-
Notifications
You must be signed in to change notification settings - Fork 3.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
XModule Style Decoupling: Follow-up fixes & refactoring #32592
XModule Style Decoupling: Follow-up fixes & refactoring #32592
Conversation
9e5edf0
to
59115e1
Compare
bd1933a
to
6954a3b
Compare
8c2a57a
to
d90ae76
Compare
46384b3
to
5f8db1b
Compare
FYI @andrey-canon -- here's some more XModule assets insanity if you are interested :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a couple of tiny nits in the docs portion.
934ab21
to
25c8c69
Compare
In ~Palm and earlier, all built-in XBlock Sass was included into LMS and CMS styles before being compiled. The generated CSS was coupled together with broader LMS/CMS CSS. This means that comprehensive themes have been able to modify built-in XBlock appearance by setting certain Sass variables. We say that built-in XBlock Sass was, and is expected to be, "theme-aware". Shortly after Palm, we decoupled XBlock Sass from LMS and CMS Sass [1]. Each built-in block's Sass is now compiled into two separate CSS targets, one for block editing and one for block display. The CSS, now located at `common/static/css/xmodule`, is injected into the running Webpack context with the new `XModuleWebpackLoader`. Built-in XBlocks already used `add_webpack_to_fragment` in order to add JS Webpack bundles to their view fragments, so when CSS was added to Webpack, it Just Worked. This unlocked a slieu of simplifications for static asset processing [2]; however, it accidentally made XBlock Sass theme-*unaware*, or perhaps theme-confused, since the CSS was targeted at `common/static/css/xmodule` regardless of the theme. The result of this is that **built-in XBlock views will use CSS based on the Sass variables _last theme to be compiled._** Sass variables are only used in a handful of places in XBlocks, so the bug is subtle, but it is there for those running off of master. For example, using edX.org's theme on master, we can see that there is a default blue underline in the Studio sequence nav [3]. With this bugfix, it becomes the standard edX.org greenish-black [4]. This commit makes several changes, firstly to fix the bug, and secondly to leave ourselves with a more comprehensible asset setup in the `xmodule/` directory. * We remove the `XModuleWebpackLoader`, thus taking built-in XBlock Sass back out of Webpack. * We compile XBlock Sass not to `common/static/css/xmodule`, but to: * `[lms|cms]/static/css` for the default theme, and * `<THEME_ROOT>/[lms|cms]/static/css`, for any custom theme. This is where the comprehensive theming system expects to find themable assets. Unfortunately, this does mean that the Sass is compiled twice, both for LMS and CMS. We would have liked to compile it once to somewhere in the `common/`, but comprehensive theming does not consider `common/` assets to be themable. * We split `add_webpack_to_fragment` into two more specialized functions: * `add_webpack_js_to_fragment` , for adding *just* JS from a Webpack bundle, and * `add_sass_to_fragment`, for adding static links to CSS compiled themable Sass (not Webpack). Both these functions are moved to a new module `xmodule/util/builtin_assets.py`, since the original module (`xmodule/util/xmodule_django.py`) didn't make a ton of sense. * In an orthogonal bugfix, we merge Sass `CourseInfoBlock`, `StaticTabBlock`, `AboutBlock` into the `HtmlBlock` Sass files. The first three were never used, as their styling was handled by `HtmlBlock` (their shared parent class). * As a refactoring, we change Webpack bundle names and Sass module names to be less misleading: * student_view, public_view, and author_view: was `<Name>BlockPreview`, is now `<Name>BlockDisplay`. * studio_view: was `<Name>BlockStudio`, is now `<Name>BlockEditor`. * As a refactoring, we move the contents of `xmodule/static` into the existing `xmodule/assets` directory, and adopt its simper structure. We now have: * `xmodule/assets/*.scss`: Top-level compiled Sass modules. These could be collapsed away in a future refactoring. * `xmodule/assets/<blocktype>/*`: Resources for each block, including both JS modules and Sass includes (underscore-prefixed so that they aren't compiled). This structure maps closely with what externally-defined XBlocks do. * `xmodule/js` still exists, but it will soon be folded into the `xmodule/assets`. * We add a new README [4] to explain the new structure, and also update a docstring in `openedx/lib/xblock/utils` which had fallen out of date with reality. * Side note: We avoid the term "XModule" in all of this, because that's (thankfully) become a much less useful/accurate way to describe these blocks. Instead, we say "built-in XBlocks". Refs: 1. openedx#32018 2. openedx#32292 3. https://github.com/openedx/edx-platform/assets/3628148/8b44545d-0f71-4357-9385-69d6e1cca86f 4. https://github.com/openedx/edx-platform/assets/3628148/d0b7b309-b8a4-4697-920a-8a520e903e06 5. https://github.com/openedx/edx-platform/tree/master/xmodule/assets#readme Part of: openedx#32292
In ~Palm and earlier, all built-in XBlock Sass was included into CMS (and LMS) styles before being compiled. So, if a site theme was meant to affect built-in XBlock styling, those changes would be manifested directly in the base CMS CSS that is included into every single Studio page. When the user provided the `?site_theme` querystring parameter, which is intended to allow devs & admins to view Studio through a given theme, CMS would look up the given theme and serve the corresponding base CMS CSS, which would affect the built-in XBlocks views (as expected). After ~Palm, built-in XBlocks styles are handled more similarly to to pure XBlock styles, in that they are only requested when CMS tries to render the block. In Studio, blocks are not rendered by the original request, but by a subsequent AJAX request to the `/container_preview` enpoint. Thus, passing the `?site_theme` query parameter to the original request will apply the given theme to Studio's chrome, but the theme will _not_ apply to built-in XBlock views, whose CSS is now loaded via async request. To fix this, we simply pass Studio's querystring parameters (including `?site_theme`) along to the `/container_view` AJAX request. This will cause CMS to correctly serve the built-in XBlock CSS from the theme specified by `?site_theme`, rather than whatever the current theme is. Part of: openedx#32292
25c8c69
to
b9df5cd
Compare
2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production. |
2U Release Notice: This PR has been deployed to the edX production environment. |
#32592 changed some webpack loading, and renamed the video display bundle from VideoBlockPreview to VideoBlockDisplay but the code for the public view did not include the updated name.
lti_block has Sass for its display, but not for its editor. During the `add_sass_to_fragment` refactoring, I mixed this up: I added a non-existent scss file to the studio_view but didn't add the actual scss file to the student_view. Course authors using the (deprecated) lti_block saw: We're having trouble rendering your component Students will not be able to access this component. Re-edit your component to fix the error. Error: Sass not found: /edx/app/edxapp/edx-platform/xmodule/assets/LTIBlockEditor.scss as a result of this bug. Original PR: openedx#32592 Private-ref: https://2u-internal.atlassian.net/browse/TNL-11029
There exists a Sass file for lti_block's display, but not for its editor. However, during the `add_sass_to_fragment` refactoring, I mixed that up: I added a non-existent scss file to the studio_view but didn't add the actual scss file to the student_view. Course authors using the (deprecated) lti_block saw: We're having trouble rendering your component Students will not be able to access this component. Re-edit your component to fix the error. Error: Sass not found: /edx/app/edxapp/edx-platform/xmodule/assets/LTIBlockEditor.scss as a result of this bug. Original PR: #32592 Private-ref: https://2u-internal.atlassian.net/browse/TNL-11029
tl;dr
Comprehensive theming for built-in XBlocks was subtly broken since we decoupled their Sass from LMS/CMS Sass. Specifically, XBlock Sass from all themes was being compiled to the same CSS file, with each theme clobbering the previous one.
This PR fixes that issue, plus some cleanup that'll help with the final phase of #31624
Description and Supporting Info
Two commits, starting with the more interesting & significant one:
fix: make built-in XBlock Sass theme-aware again
In ~Palm and earlier, all built-in XBlock Sass was included into LMS and CMS styles before being compiled. The generated CSS was coupled together with broader LMS/CMS CSS. This means that comprehensive themes have been able to modify built-in XBlock appearance by setting certain Sass variables. We say that built-in XBlock Sass was, and is expected to be, "theme-aware".
Shortly after Palm, we decoupled XBlock Sass from LMS and CMS Sass [1]. Each built-in block's Sass is now compiled into two separate CSS targets, one for block editing and one for block display. The CSS, now located at
common/static/css/xmodule
, is injected into the running Webpack context with the newXModuleWebpackLoader
. Built-in XBlocks already usedadd_webpack_to_fragment
in order to add JS Webpack bundles to their view fragments, so when CSS was added to Webpack, it Just Worked.This unlocked a slieu of simplifications for static asset processing [2]; however, it accidentally made XBlock Sass theme-unaware, or perhaps theme-confused, since the CSS was targeted at
common/static/css/xmodule
regardless of the theme. The result of this is that built-in XBlock views will use CSS based on the Sass variables last theme to be compiled. Sass variables are only used in a handful of places in XBlocks, so the bug is subtle, but it is there for those running off of master. For example, using edX.org's theme on master, we can see that there is a default blue underline in the Studio sequence nav [3]. With this bugfix, it becomes the standard edX.org greenish-black [4].This commit makes several changes, firstly to fix the bug, and secondly to leave ourselves with a more comprehensible asset setup in the
xmodule/
directory.We remove the
XModuleWebpackLoader
, thus taking built-in XBlock Sass back out of Webpack.We compile XBlock Sass not to
common/static/css/xmodule
, but to:[lms|cms]/static/css
for the default theme, and<THEME_ROOT>/[lms|cms]/static/css
, for any custom theme.This is where the comprehensive theming system expects to find themable assets. Unfortunately, this does mean that the Sass is compiled twice, both for LMS and CMS. We would have liked to compile it once to somewhere in the
common/
, but comprehensive theming does not considercommon/
assets to be themable.We split
add_webpack_to_fragment
into two more specialized functions:add_webpack_js_to_fragment
, for adding just JS from a Webpack bundle, andadd_sass_to_fragment
, for adding static links to CSS compiled themable Sass (not Webpack).Both these functions are moved to a new module
xmodule/util/builtin_assets.py
, since the original module (xmodule/util/xmodule_django.py
) didn't make a ton of sense.In an orthogonal bugfix, we merge Sass
CourseInfoBlock
,StaticTabBlock
,AboutBlock
into theHtmlBlock
Sass files. The first three were never used, as their styling was handled byHtmlBlock
(their shared parent class).As a refactoring, we change Webpack bundle names and Sass module names to be less misleading:
<Name>BlockPreview
, is now<Name>BlockDisplay
.<Name>BlockStudio
, is now<Name>BlockEditor
.As a refactoring, we move the contents of
xmodule/static
into the existingxmodule/assets
directory, and adopt its simper structure. We now have:xmodule/assets/*.scss
: Top-level compiled Sass modules. These could be collapsed away in a future refactoring.xmodule/assets/<blocktype>/*
: Resources for each block, including both JS modules and Sass includes (underscore-prefixed so that they aren't compiled). This structure maps closely with what externally-defined XBlocks do.xmodule/js
still exists, but it will soon be folded into thexmodule/assets
.We add a new README [4] to explain the new structure, and also update a docstring in
openedx/lib/xblock/utils
which had fallen out of date with reality.Side note: We avoid the term "XModule" in all of this, because that's (thankfully) become a much less useful/accurate way to describe these blocks. Instead, we say "built-in XBlocks".
Refs:
Part of: #32292
fix: flow ?site_theme down through Studio container preview
In ~Palm and earlier, all built-in XBlock Sass was included into CMS (and LMS) styles before being compiled. So, if a site theme was meant to affect built-in XBlock styling, those changes would be manifested directly in the base CMS CSS that is included into every single Studio page. When the user provided the
?site_theme
querystring parameter, which is intended to allow devs & admins to view Studio through a given theme, CMS would look up the given theme and serve the corresponding base CMS CSS, which would affect the built-in XBlocks views (as expected).After ~Palm, built-in XBlocks styles are handled more similarly to to pure XBlock styles, in that they are only requested when CMS tries to render the block. In Studio, blocks are not rendered by the original request, but by a subsequent AJAX request to the
/container_preview
enpoint. Thus, passing the?site_theme
query parameter to the original request will apply the given theme to Studio's chrome, but the theme will not apply to built-in XBlock views, whose CSS is now loaded via async request.To fix this, we simply pass Studio's querystring parameters (including
?site_theme
) along to the/container_view
AJAX request. This will cause CMS to correctly serve the built-in XBlock CSS from the theme specified by?site_theme
, rather than whatever the current theme is.Part of: #32292
Testing
Setup
First: Set up Tutor Nightly if you haven't already, with at least one user created. Ensure images are recently pulled.
Then:
Bug Reproduction
tutor config save --set EDX_PLATFORM_REPOSITORY=https://github.com/openedx/edx-platform tutor config save --set EDX_PLATFORM_VERSION=master tutor images build openedx tutor local start -d
Then, navigate to this problem in the demo course, log in.
"Show Answer" is nice and orange, as specified by our god awful theming choices:
But when you hover over "Show Answer", notice that when you hover, it turns RED. As in, red-theme red! 😲
Open the network inspector and filter for CSS. Refresh the page. Notice where
ProblemBlockPreview.css
is coming from: it's http://local.overhang.io/static/css/xmodule/ProblemBlockPreview.css. Notice that there's no/indigo
anywhere in that path. Both indigo and red-theme have compiled to that same location, and because red-theme is second alphabetically, it won.This problem is barely noticeable, because the ProblemBlock mostly just inherits cascading CMS CSS rules... but link hover is one thing that for which it does look at the compiled XBlock CSS.
Fix Reproduction
Again, navigate to this same problem in the demo course. Hover over "Show Answer". No red, just orange 😌
Open the network inspector and filter for CSS. Refresh the page. Now, notice where
ProblemBlockDisplay.css
is coming from: it's http://local.overhang.io/static/indigo/css/xmodule/ProblemBlockDisplay.ee14053c39ab.css, which is specific to indigo.Don't worry, if we run
tutor local do settheme red-theme
, refresh, and check the network log again, we see red-theme's version of the file is safe over here: http://local.overhang.io/static/red-theme/css/xmodule/ProblemBlockDisplay.bdc1f9f16129.css, yielding: