Skip to content

Conversation

@talbs
Copy link
Contributor

@talbs talbs commented Oct 2, 2013

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

@talbs
Copy link
Contributor Author

talbs commented Oct 2, 2013

  • @frrrances and @marcotuts, mind taking a look from a FED/Design point of view - please proof all of the LMS views you know of to make sure we're not regressing.
  • @symbolist / @gwprice, can one of you mind taking a look from an app/infrastructure perspective?
  • @cpennington and @feanil, mind looking through this to make sure the changes to the Django pipeline won't cause you folks any issues in architecture or production-land?

if anyone has any better ideas to help split the rendered CSS into manageable chunks for IE9 while addressing the concerns above, I'm all ears.

@talbs
Copy link
Contributor Author

talbs commented Oct 2, 2013

Also, pinging @jzoldak formally on this one too.

Copy link

Choose a reason for hiding this comment

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

This file should not be committed.

@marcotuts
Copy link
Contributor

will look through the rest tomorrow but I'm going through active pull requests to remind people to add entries to the Change Log file.

@symbolist
Copy link
Contributor

I did a quick search and found two css spliters: css_splitter (Rails Asset Pipeline) and bless.js. (Node.js). They are both a couple of hundred lines each so porting wouldn't take more than an afternoon but neither is 100% reliable. So I am not sure about doing this.

However, we can probably create a test (either in js or python) which counts the number of selectors in the css files and logs an error if in any of the files the count is above 3500. So someone will not have to manually do a check every release.

Copy link
Contributor

Choose a reason for hiding this comment

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

Does this page not need any of the extend* files? Or was that an oversight?

@cpennington
Copy link
Contributor

I don't see any issues w/ the way that the pipeline stuff is defined.

@feanil
Copy link
Contributor

feanil commented Oct 3, 2013

yea, pipeline stuff looks good, verified that it built correctly in my aws sandbox just to be sure.

@talbs
Copy link
Contributor Author

talbs commented Oct 3, 2013

Thanks for the thoughts and review, folks.

@symbolist, you reminded us of some good points. After talking with @frrrances, I want to take a step back and approach resolving this bug in the following steps:

  1. Separate out vendor and context-specific styling (app vs. courseware) appropriately (completed with this existing work).
  2. Find the produced CSS that is still having any rule-count issues and run with one of the more automated ways of splitting the problem files up (Bless, http://blesscss.com, is something I've used on personal projects before) as part of our production and local dev environment pipelines.

Going this route will help avoid the very valid concern of manually checking for rule issues with every CSS delta to our codebase. It will also make it easier to decouple our Sass/CSS output architecture from the temporary solution needed for IE9 (which will hopefully not need to be supported in the future)

With that all said, I'm going to refactor my work here to not manually split up our troublesome CSS files but will need help implementing Bless into our pipeline workflow for all of our apps and our local/production envs. Who's best to help with that?

@cpennington
Copy link
Contributor

My suggestion for putting bless into everything would be to put it in as part of assets.rake, as preprocessor, rather than putting it into the django-pipeline infrastructure.

@talbs
Copy link
Contributor Author

talbs commented Oct 3, 2013

I've reverted this work back to just producing one lms-style-app.css file from one main Sass file (application.scss).

@cpennington, thanks for the recommendation of placing Bless into assets.rake. I tried to take a look into placing that in and its way beyond my wheelhouse. I'd like to get this resolved quickly as styling is broken for parts of the LMS in production now on IE9. Is there any chance you can help 1) install Bless and 2) add it into our rake.assets file?

If possible, I'd like it to run for all of our CSS files (and not just the problem ones now).

Thanks for any help you can provide in resolving this.

@talbs
Copy link
Contributor Author

talbs commented Oct 9, 2013

After discussing possible solutions with @singingwolfboy and others, I think we'd ideally like a solution that's less manual and more pipeline-centric. The optimal option proposed involves adding BlessCSS (http://blesscss.com/) to or around our Django Pipeline workflow when rendering our production-ready css files per app.

I'm closing this with that spirit in mind. @mikigoyal, since this relates to a cat-1 bug, can we get some prioritized resources ( @singingwolfboy, @cpennington, or devops ) familiar with the pipeline on the suggested approach?

@talbs talbs closed this Oct 9, 2013
@talbs talbs reopened this Oct 11, 2013
@talbs
Copy link
Contributor Author

talbs commented Oct 15, 2013

Closing this PR down, as there is now a separate one being worked on for a hotfix - https://github.com/edx/edx-platform/pull/1320

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.

7 participants