Studio: Revised CSS Architecture (resolves STUD-806) - #1189
Conversation
There was a problem hiding this comment.
CONTEXT: We don't need this file (as its not referenced anywhere in our CSS/Sass imports and calls and it looks pretty sketchy)
There was a problem hiding this comment.
yeah it seems like this was for a studio prototype over a year ago. :)
|
@frrrances and @marcotuts, mind taking a look from a FED/Design point of view - please proof Studio views to make sure we're not regressing. @cahrens and @singingwolfboy, 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? |
|
Also, pinging @jzoldak formally on this one too. |
There was a problem hiding this comment.
There's something odd here. xmodule/* is referenced here, and also inside of the old base-style.scss file. Looks like the ones referenced here were old and wrong?
|
Thanks for the thoughts and review, folks. After talking with @frrrances and going through a similar review for the LMS' CSS, I want to take a step back and approach resolving this bug in the following steps:
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? |
|
I've reverted this work back to just producing one lms-style-app.css file from one main Sass file (style-app.scss). I'm waiting for help in integrating Bless into our assets.rake. I've pinged @cpennington on the mirror work being done on the LMS side. Hopefully the same solution can be used here. |
|
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? |
There was a problem hiding this comment.
@talbs I'm pretty sure this line is causing the collectstatic failures in Jenkins. When I add the / back in, I'm able to run collectstatic successfully.
Can't claim I understand the cryptic error message though :)
There was a problem hiding this comment.
@wedaly, D'oh! That's definitely a syntax error I should have caught without you having to triage all of this (thx for that by the way). Three cheers for tests on Jenkins catching this :)
…zation, reduced redundancy, and to handle IE9 selector limits
|
@frrrances, mind taking this branch for a spin and reviewing? Specifically, I'd love a review of all xmodule Sass/CSS (testing problems, videos, etc.). I've proofed locally and while styles look fine, some thing is weird with the Video Unit UI (compared to studio.stage.edx.org currently). |
|
Looks good to me. I think the differences you are seeing in the video module have to do with new changes and/or old video modules, so I think it's okay, though we should certainly check that on stage once it's up. I wasn't able to find anything else amiss. 👍 I would like to review this once it's on stage since I can't check the pipeline/css breakdown locally. |
|
@frrrances you can check out the pipeline breakdown by deploying this onto an AWS test box, using @jarv's ansible scripts. Do you know how to do that? If not, I'm sure that either @jarv or I can come by and help you out. |
|
Thanks for the review and feedback, all. I agree that we should review this on staging once its up to double-check the pipeline is behaving as we want. For now, I'm merging this in. |
Studio: Revised CSS Architecture (resolves STUD-806)
feat: upgrade mysql to 8.0
This Studio work helps to:
There is a known trade-off. Currently, the xmodule Sass files (xmodule/descriptors/css/module-styles.scss and xmodule/modules/css/module-styles.scss) are referencing Sass variables, mixins, and extends we have defined elsewhere in Studio. Because of this, style-xmodule.scss has to re-import these needed assets, which causes redundancy in the compiled style definitions that both style-app.css and style-xmodule.css (essentially, we're making the browser load/parse a small subset of the same rules in two separate stylesheets - not great at all).
Hopefully with xblocks on the horizon, this redundancy can be removed and corrected. If that's not the case, we will need to discuss a permanent solution more.
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