saleem-latif/WL-299: SCSS Overrides for Comprehensive Theming - #11476
saleem-latif/WL-299: SCSS Overrides for Comprehensive Theming#11476saleem-latif wants to merge 1 commit into
Conversation
1e69083 to
98dd913
Compare
|
👍 LGTM @saleem-latif -- would be great if we could get one more thumb from the guys you've tagged -- @ziafazal is out for the rest of the week -- perhaps @mtyaka or @andy-armstrong can take a look? |
|
@AlasdairSwan I'm not sure if you're doing much with the Sass pipeline, but if so maybe you could take a look as well |
| if lms_sass.isdir(): | ||
| lms_css.mkdir_p() | ||
| THEME_SASS_DIRECTORIES.append(lms_sass) | ||
| THEME_SASS_DIRECTORIES.append(("lms/static/sass", theme_dir / "lms" / "static" / "css")) |
There was a problem hiding this comment.
Nit: You can replace theme_dir / "lms" / "static" / "css" with lms_css, which is already defined a couple of lines above.
|
@saleem-latif Thanks for this, it looks great. In addition to the comments I made inline:
|
|
@mtyaka I tried moving |
| path("common/static/sass"), | ||
| (path("common/static/sass"), path("common/static/css")) | ||
| ] | ||
| COMMON_SASS_LOOKUP_DIRECTORIES = [ |
There was a problem hiding this comment.
I think we need more commentary in the code describing the function of the lookup directories.
|
This PR needs tests. |
|
I'm a little concerned with how reliant this approach is to execution order. Can there be a way to do this that is more deterministic? |
| css_dir=css_dir or sass_dir.parent / "css", | ||
| )) | ||
| else: | ||
| sh("rm -rf {css_dir}/*.css".format(css_dir=css_dir)) |
There was a problem hiding this comment.
Doesn't this need to handle css_dir being falsey, like the dry_run case does?
|
I agree with @benpatterson that this needs tests. Look at test_assets.py and extend it to demonstrate the behavior when a comprehensive theme is in play. |
|
I'm a little unsure about the idea of overriding partial files. It seems brittle to me, in that it is depending upon the naming conventions used to break up the SASS. It means that going forward, platform developers can't rename anything or change load order without the risk of breaking themes. It also means that the theme author has to know exactly how the platform author laid out all of their partials. If we do go down this path then we will have to be very careful to document what can be overridden by theme authors, and what therefore can't be changed by platform developers. We have essentially made the files on the disk into a contract that can't be changed. There is also a risk of accidental overrides if a theme author has a new SASS partial, and then later the platform author happens to use the same filename. In this situation, the platform author won't know that they've broken the theme. Having said this, it does seem very powerful too. It makes it much easier to change theme colors, fonts etc in a comprehensive way, without having to manually override every single CSS file with a theme version. If we are very explicit about the files that a theme author is allowed to override then maybe that's okay. One option worth considering is to have a named directory for the partials that are allowed to be overridden, to avoid any risk of accidental collisions. |
|
Thinking this through some more, I'm confused as to how this will work (this could well be because I'm not understanding things correctly). Say you have three installed themes: red, green and blue. Each one of these overrides variables.scss to define the brand colors to be appropriate to their theme. In order to support multi-tenancy, the result we want is that each theme directory has an lms-main-ltr.css (for example) that is compiled with its brand colors applied. When a request asks for the blue theme, the static file loader will look for the CSS file first in the blue theme directory and will pick it up if it is there. If it isn't, then it will instead load the default version of the CSS file. If I'm understanding this PR correctly, what it does is put all three of these theme directories on the lookup path at once. This means that only the first listed theme will get to override variables.scss, and that this theme's values will be used to generate the default lms-main-ltr.css. This won't support multi-tenancy where we can dynamically switch themes per request. Is my understanding correct here? To reiterate @benpatterson's point, it is crucial that we have tests that demonstrate that the desired use cases will work. Similarly, I recommend that we implement a URL parameter so that developers can easily switch themes, like we can with |
|
@andy-armstrong This PR only deals with SASS compilation, not with django static asset lookup path. It currently assumes a single theme, but it wouldn't be hard to extend this to support multiple themes; sass compilation would just have to be run separately for each installed theme. Multi-tenancy and multi-theme support are being worked on in a separate PR: https://github.com/edx/edx-platform/pull/11480 |
|
@andy-armstrong I understand your concerns about themes being brittle if we allow overriding arbitrary sass partials. The same could be said of templates and static files (images, etc), though. We already support overriding arbitrary django/mako templates and static files from a theme, but that doesn't mean we can't change or rename any template in edx-platform. As a theme author, I understand overriding templates is brittle, but I still do it sometimes when there's no better solution, and I am grateful I can do that in a custom theme instead of having to change templates directly in a fork of edx-platform. Themes almost always have to be adapted for new major releases of edx-platform due to changes in HTML structures and CSS rules, so theme authors expect to have to do some work when upgrading. Still, I don't think overriding partials other than a few selected and documented ones (ie. |
|
@mtyaka Thanks for the well thought out replies. It is good to hear that you don't think it would be hard to add support for multiple themes. To my mind, I wouldn't want to merge this without it, because we know that's what we need. I think the key is that the theme's SASS overrides should only apply to the theme's own CSS files, which as you suggest seems that there should be a separate compilation step performed per theme. Regarding overrides, I think we're in agreement. Overriding most partials will be risky and error prone, so we should just make it clear which ones make sense, i.e. I'm on vacation all next week, so if possible it would be ideal to hold off merging until I get back. If you need this sooner then I recommend getting @AlasdairSwan's opinion. Thanks. |
25f0eb5 to
3c9a97b
Compare
|
@AlasdairSwan, it would be great if you could take a look at this PR -- thanks! |
172ce86 to
430bdf1
Compare
|
jenkins run python |
|
jenkins run lettuce |
|
I'm about half-way through a pass on this. However, this still seems fairly dependent on the order of operations. Is there a smarter way to do it? |
2aba17d to
c5767c5
Compare
|
@benpatterson, it appears that @saleem-latif has updated this PR with a new commit -- can you take another look and let us know what else you'd like to see? |
|
@saleem-latif did you make additional changes? It looks like a squash and/or rebase to me... |
|
@benpatterson yes I made the changes that you suggested in the feedback. |
c5767c5 to
17aefba
Compare
|
jenkins run lettuce |
|
@benpatterson I have divided test cases into multiple tests, kindly take a look. |
mattdrayer
changed the title
| """ | ||
| Disable comprehensive theme and clear changes made for comprehensive themes. | ||
| """ | ||
| patch("pavelib.assets.Env.env_tokens", {'COMPREHENSIVE_THEME_DIR': ""}) |
There was a problem hiding this comment.
Why is this line necessary when the env_tokens is only updated within a with clause that should clean up?
|
@saleem-latif Thanks for addressing all of my feedback. The code looks very clear now, and the tests are excellent. Unfortunately I'm still uncomfortable with the approach, i.e. that the configured theme changes the way that the non-themed CSS files are generated. This seems like it will be confusing to folks who try to change their theme but don't think to regenerate their CSS. It will be important to document this clearly in the comprehensive theming documentation. My bigger concern is that this approach doesn't work for multi-tenancy, and we know that's where we want to go. I looked at the multi-tenancy PR (https://github.com/edx/edx-platform/pull/11480) but that doesn't seem to address SASS compilation as yet. I don't see that there's enough benefit to the theme developer with this change, when they will have to change everything they do when we support multi-tenancy. Is the thinking that this approach will continue to work for folks who don't need multi-tenancy even when we add full support for it for ourselves? How are you imagining that this will work? I'd like to see a clear architectural design for multi-tenant theming and understand how this work fits into it before approving this PR. @mattdrayer can we discuss this the next time you're in the office so that we can both be on the same page. FYI @efagin @nedbat. |
|
@andy-armstrong & devs: wouldn't it be better to allow overriding of just the base colors (i.e. $black, $red, $blue, etc...) and then let all the other vars just automagically generated themselves? |
|
@mattdrayer & @saleem-latif, so you guys get a ping about ^ |
|
@andy-armstrong PR #11480 is an older attempt at multi-tenant theming that has since been closed -- instead please reference #11613 |
|
Closing this PR as the development of the feature has shifted to #11613 |
6 participants
Hi @mattdrayer , @andy-armstrong, @mtyaka , @ziafazal , @benpatterson
Kindly review this PR and share your thoughts on these changes for WL-299.
Note:
Since, sass first looks at its current path to resolve
@importso, We decided to move sass files, that comprehensive theme will be able to override, to partials directory. I have only movedbase/_variables.scssto partials dir, if this change set looks good than also share your thoughts on what other sass files should be moved to partials dir.Description of WL-299:
Currently the behavior of static files and templates overriding in comprehensive theming is that if we place a file named same as it is placed in lms it will override the lms static/template file.
We want to apply the same override pattern for sass files, see the discussion here and here for some insight.
Acceptance Criteria:
for more information regarding implementation details look at the comments on WL-299