Skip to content

Hotfix - LMS: Revised CSS Architecture (resolves LMS_1020) - #1320

Closed
talbs wants to merge 5 commits into
releasefrom
talbs/lms-cssarch
Closed

Hotfix - LMS: Revised CSS Architecture (resolves LMS_1020)#1320
talbs wants to merge 5 commits into
releasefrom
talbs/lms-cssarch

Conversation

@talbs

@talbs talbs commented Oct 11, 2013

Copy link
Copy Markdown
Contributor

This is a copy of an original pull request made against master - https://github.com/edx/edx-platform/pull/1320

This LMS work helps to:

  • resolve a known IE9 CSS Selector limit bug (LMS-1020- https://edx-wiki.atlassian.net/browse/LMS-1020).
  • provides better, more consistent names to our outputted CSS files
  • move vendor-related CSS to be loaded when needed (overall vs. in-courseware)
  • move IE-specific styling (largely left untouched) into using standard body element classes
  • FYI - My editor also stripped out some whitespace as part of my editing

This work brought up large concerns:

  1. This resolves the bug noted above, but is far from ideal. Given that the LMS Sass architecture (the individual files, the inheritance/dependency within them, and the unorganized styling across the app), I could not find a logical/semantic way to segment the application.scss (generated by application.scss.mako for alpha-level theming reasons). Thus the separate CSS files are suffixed with -extend1 and -extend2.
  2. Also, there is a ton of redundancy where each application(-extend*).css file is calling the same Sass set-up files, generating redundant styling rules that are already cached in its sibling files (essentially, we're making the browser load/parse a subset of the same rules in two separate stylesheets - not great at all).

These two points really concern me and I highly recommend a very deep revamp/cleaning of the Sass architecture shortly. I've tested the display of many pages locally, but each page's display should really be stress-tested to ensure there are no visual regressions. If you want to test the number of rules being used, crack open your console and run this gist in a view - https://gist.github.com/psebborn/1885511

Per Rob/Miki, its been decided to merge this change into the release branch.

@talbs

talbs commented Oct 11, 2013

Copy link
Copy Markdown
Contributor Author
  • @frrrances and @marcotuts, mind reviewing this from a design/UX perspective? we're looking for all LMS views to render as they normally would
  • @cpennington, mind giving this the thumbs up from an architecture perspective again?
  • @jzoldak or @wedaly, I'll check to make sure tests are passing, but would you fellas approving this formally as well?
  • @cahrens, if you have any feedback or need anything else to be prepped for a merge, let me know.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indentation looks wrong here

@cpennington

Copy link
Copy Markdown
Contributor

The only issues I'm seeing are around whitespace, and are optional to fix for this hotfix.

@frrrances

Copy link
Copy Markdown
Contributor

@talbs looks good. I'd like to check it once more on staging once it's up since I can't load it on my own local.

@talbs

talbs commented Oct 11, 2013

Copy link
Copy Markdown
Contributor Author

After much finagling (thanks to @singingwolfboy and @jarv), I've been able to proof these new production-level css files on a sandbox.

I ran https://gist.github.com/psebborn/1885511#file-countcssrules-js in the console on a course view (which contains all of our produced CSS). Here is a screengrab of the results (note that the gist's warning message syntax is broken - our rules are under the limit here - http://blogs.msdn.com/b/ieinternals/archive/2011/05/14/10164546.aspx). the counts per sheet above the warning are what matter

screen shot 2013-10-11 at 11 48 00 am

I'll re-test this once its good to be placed on staging.

@talbs

talbs commented Oct 11, 2013

Copy link
Copy Markdown
Contributor Author

@cpennington and @frrrances, thanks for the help. I've addressed the indentation notes.

@cahrens

cahrens commented Oct 11, 2013

Copy link
Copy Markdown

This commit is on staging. We have decided to wait until Tuesday to release it. But please go ahead and test it on staging and give your thumbs up or thumbs down for this PR.

@frrrances

Copy link
Copy Markdown
Contributor

@talbs thanks for tackling this! you've done a heck of a job getting this to done. I noticed a few things in our review:

  1. on the marketing iframe button, there is an extra carrot:
    extra-carrot
  2. the browser warning is missing when you're in a course:
    browser-warning
  3. The background on the current course nav is transparent, when it should be dark:
    incorrect-nav-highlight (stage)
    correct-nav-highlight (prod)

I'm going to keep digging in so I will let you know if I come across anything else.

@frrrances

Copy link
Copy Markdown
Contributor

@talbs: One last question if you have the pipeline working on your local, have you checked the old front-end/marketing site?

Once you've tackled all the issues I mentioned, this is good to go. 👍 Thanks again for taking this beast on!

@talbs

talbs commented Oct 16, 2013

Copy link
Copy Markdown
Contributor Author

Closing this PR as we're trying to get this work into the current release planned for 10-17. A new PR for that effort has been made - https://github.com/edx/edx-platform/pull/1365

@talbs talbs closed this Oct 16, 2013
giovannicimolin pushed a commit to open-craft/openedx-platform that referenced this pull request Dec 20, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants