Skip to content
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

Stop dynamically generating XModule SCSS #32292

Closed
7 tasks done
Tracked by #31624 ...
kdmccormick opened this issue May 23, 2023 · 2 comments
Closed
7 tasks done
Tracked by #31624 ...

Stop dynamically generating XModule SCSS #32292

kdmccormick opened this issue May 23, 2023 · 2 comments
Assignees

Comments

@kdmccormick
Copy link
Member

kdmccormick commented May 23, 2023

Background

Goal

All XModule SCSS should be committed to the repository rather than generated as part of the build process. Benefits:

  • Simplify the build process, partially unblocking us from removing Python from the build process.
  • Make XModule assets more similar to standard XBlock assets.
  • Make it so XModule SCSS can be read by a human being; no tooling or special context necessary.
  • Make the next step (stop dynamically generating XModule JS) easier & safer by getting SCSS out of the equation.

Sum of the changes

The PRs do a better job of describing each atomic change, but as whole, here's what's changing.

Before the first PR...

XModule CSS is:

  • defined by various SCSS files under xmodule/css;
  • synthesized together for each XModule under its studio_view_css and preview_view_css class variables;
  • processed by the xmodules_assets console script (defined in xmodule/static_content.py), which:
    • for each XModule,
      • copies each file in the XModule's studio_view_css from
        • xmodule/css/<blah>/<yada>.scss to
        • common/static/xmodule/descriptors/css/_001-<md5-hash-of-contents-28f7a87e12f>.scss
      • generates an entrypoint SCSS file named like common/static/xmodule/descriptors/css/AnnotatableBlockStudio.be69909d83985d31e206fad272906958.scss, with contents like:
        .xmodule_edit.xmodule_AboutBlock {
          @import "9bdcda00f046f78be79aca7791e1d4fb.scss";
          @import "a10fc3e0fd6aca63426a89e75fe69c31.scss";
        }
      • copies each file in the XModule's preview_view_css from
        • xmodule/css/<blah>/<yada>.scss to
        • common/static/xmodule/modules/js/_001-<md5-hash-of-contents-28f7a87e12f>.scss
      • generates an entrypoint SCSS file named like common/static/xmodule/modules/css/AnnotatableBlockPreview.7e95b106aa0a61824f4290da1374960d.scss;
  • compiled from each SCSS entrypoint file a CSS file, eg, common/static/css/xmodule/AnnotatableBlockPreview.7e95b106aa0a61824f4290da1374960d.css, which are
  • included into Webpack via xmodule/util/xmodule_django.py:XModuleWebpackLoader; and
  • rendered to XModule via the xmodule/util/xmodule_django.py:add_webpack_to_fragment .

After the last PR...

XModule SCSS will be:

  • defined by various SCSS files under xmodule/static/sass/include;
  • synthesized together for each XModule by SCSS files named:
    • xmodule/static/sass/cms/AnnotatableBlockStudio.scss (editing views)
    • xmodule/static/sass/lms/AnnotatableBlockPreview.scss (display views)
  • compiled from xmodule/static/sass/(lms|cms)/*.scss to CSS files, eg, xmodule/static/css/AnnotatableBlockPreview.css, which are
  • included into Webpack via xmodule/util/xmodule_django.py:XModuleWebpackLoader (no change); and
  • rendered to XModule via the xmodule/util/xmodule_django.py:add_webpack_to_fragment (no change).
@kdmccormick kdmccormick self-assigned this May 23, 2023
@kdmccormick kdmccormick changed the title Stop dynamic generating XModule SCSS Stop dynamically generating XModule SCSS May 23, 2023
kdmccormick added a commit that referenced this issue May 25, 2023
…2286)

For the XBlocks types that use legacy XModule-style assets (specifically, those that
inherit from `HTMLSnippet`), this is small refactor that brings them a bit closer to being like
standard XBlocks.

Given these class attributes:

    class SomeXModuleLikeBlock(..., HTMLSnippet, ...):
        ...
        studio_view_css = { ... }
        preview_view_css = { ... }
        studio_view_js = { ... }
        preview_view_js = { ... }
        ...

we make it so their values are *paths to the resources*
rather than *the actual content of the resources*.
This is a no-op change, but it'll enable future XModule
asset refactorings which require us to operate on asset
paths rather than contents.

Part of: #32292
kdmccormick added a commit that referenced this issue Jun 5, 2023
The `xmodule_assets` command copies SCSS files from
xmodule/css to common/static/xmodule/{modules|descriptors}/scss.
It renames the files to the format:

     _{INDEX}-{HASH}.scss

where an XModule's first SCSS resource will have INDEX==0,
the next will have INDEX==1, ...and that's it because no
XModule has more than two SCSS resources.
The output looks like this:

    common/static/xmodule/descriptors/scss:
      _000-808fcbb4c5109c5156ae3c0c9729c8be.scss
      ...
      _001-a10fc3e0fd6aca63426a89e75fe69c31.scss
    common/static/xmodule/modules/scss:
      _000-1ad2f05db822d3176affd203d70319c0.scss
	  ...
      _001-482ebc752ab6e41946651ceb0f3e7f55.scss

These indexes serve no purpose. Reading the comments
and git-blame in xmodule/static_content.py, one can glean
that the indexes might have been intended to enforce
dependency relationships between the assets, but
this is unnecessary, because the ordering of the copied
SCSS is *already preserved* by the order which they're
included into the `{BLOCK_NAME}{Studio|Preivew}.{HASH}.scss`
SCSS entrypoint files. I have to assume that this is an
unnecessary relic from the time when the XModule system
was more heavily utilized, rather than just a legacy corner
of the XBlock framework as it is today.
So, we remove the indexes, which lets us simplify the logic
of xmodule/static_content.py. This is a minor refactoring, but it'll
make it easier for the next steps on our way to deleting
xmodule/static_content.py entirely. The new output looks like this:

    common/static/xmodule/descriptors/scss:
      _808fcbb4c5109c5156ae3c0c9729c8be.scss
	  ...
      _d41921b4c5d45188759ef3d04fd9a78a.scss
    common/static/xmodule/modules/scss:
      _1ad2f05db822d3176affd203d70319c0.scss
	  ...
      _b80300e1a5f290f6a850e35874068427.scss

Part of: #32292
kdmccormick added a commit to kdmccormick/edx-platform that referenced this issue Jun 6, 2023
Similar to openedx#32287,
this change removes another unnecessary step from the
`xmodule_assets` script. The script, which generates XModule
SCSS "entrypoint" files (synthesizing one or more "source" SCSS
resources), was appending MD5 hashes to each SCSS entrypoint filename:

    common/static/xmodule/descriptors/scss:
       AboutBlockStudio.768623f4d8d73dfb637fc94583adb990.scss
       ...
       WordCloudBlockStudio.d41d8cd98f00b204e9800998ecf8427e.scss
    common/static/xmodule/modules/scss:
       AboutBlockPreview.05a6cbd5c10100a245fa2cbf151b9770.scss
       ...
       WordCloudBlockPreview.7b899a56a70d29c58cf14b7e1888a0ec.scss

It's unclear why these MD5 hashes needed to be appended.
A comment in xmodule/static_content.py hints that it might have
something to do with de-duplication, but that doesn't make any sense,
because each XModule has exactly two SCSS entrypoint files (one for
studio_view and one for other student/author_views) and none of those
entrypoint files can possibly be shared between XModules.

Soon, as part of deleting the `xmodule_assets` script,
we would like to just check these SCSS files into version control
rather than generating them. In order to do that, we will need to
drop the hashes. This commit does that.
The new output looks like this:

    common/static/xmodule/descriptors:
       AboutBlockStudio.scss
       ...
       WordCloudBlockStudio.scss
    common/static/xmodule/modules:
       AboutBlockPreview.scss
       ...
       WordCloudBlockPreview.scss

Part of: openedx#32292
kdmccormick added a commit that referenced this issue Jun 7, 2023
Similar to #32287,
this change removes another unnecessary step from the
`xmodule_assets` script. The script, which generates XModule
SCSS "entrypoint" files (synthesizing one or more "source" SCSS
resources), was appending MD5 hashes to each SCSS entrypoint filename:

    common/static/xmodule/descriptors/scss:
       AboutBlockStudio.768623f4d8d73dfb637fc94583adb990.scss
       ...
       WordCloudBlockStudio.d41d8cd98f00b204e9800998ecf8427e.scss
    common/static/xmodule/modules/scss:
       AboutBlockPreview.05a6cbd5c10100a245fa2cbf151b9770.scss
       ...
       WordCloudBlockPreview.7b899a56a70d29c58cf14b7e1888a0ec.scss

It's unclear why these MD5 hashes needed to be appended.
A comment in xmodule/static_content.py hints that it might have
something to do with de-duplication, but that doesn't make any sense,
because each XModule has exactly two SCSS entrypoint files (one for
studio_view and one for other student/author_views) and none of those
entrypoint files can possibly be shared between XModules.

Soon, as part of deleting the `xmodule_assets` script,
we would like to just check these SCSS files into version control
rather than generating them. In order to do that, we will need to
drop the hashes. This commit does that.
The new output looks like this:

    common/static/xmodule/descriptors:
       AboutBlockStudio.scss
       ...
       WordCloudBlockStudio.scss
    common/static/xmodule/modules:
       AboutBlockPreview.scss
       ...
       WordCloudBlockPreview.scss

Part of: #32292
Yagnesh1998 pushed a commit to ManpraXSoftware/edx-platform that referenced this issue Jun 8, 2023
The `xmodule_assets` command copies SCSS files from
xmodule/css to common/static/xmodule/{modules|descriptors}/scss.
It renames the files to the format:

     _{INDEX}-{HASH}.scss

where an XModule's first SCSS resource will have INDEX==0,
the next will have INDEX==1, ...and that's it because no
XModule has more than two SCSS resources.
The output looks like this:

    common/static/xmodule/descriptors/scss:
      _000-808fcbb4c5109c5156ae3c0c9729c8be.scss
      ...
      _001-a10fc3e0fd6aca63426a89e75fe69c31.scss
    common/static/xmodule/modules/scss:
      _000-1ad2f05db822d3176affd203d70319c0.scss
	  ...
      _001-482ebc752ab6e41946651ceb0f3e7f55.scss

These indexes serve no purpose. Reading the comments
and git-blame in xmodule/static_content.py, one can glean
that the indexes might have been intended to enforce
dependency relationships between the assets, but
this is unnecessary, because the ordering of the copied
SCSS is *already preserved* by the order which they're
included into the `{BLOCK_NAME}{Studio|Preivew}.{HASH}.scss`
SCSS entrypoint files. I have to assume that this is an
unnecessary relic from the time when the XModule system
was more heavily utilized, rather than just a legacy corner
of the XBlock framework as it is today.
So, we remove the indexes, which lets us simplify the logic
of xmodule/static_content.py. This is a minor refactoring, but it'll
make it easier for the next steps on our way to deleting
xmodule/static_content.py entirely. The new output looks like this:

    common/static/xmodule/descriptors/scss:
      _808fcbb4c5109c5156ae3c0c9729c8be.scss
	  ...
      _d41921b4c5d45188759ef3d04fd9a78a.scss
    common/static/xmodule/modules/scss:
      _1ad2f05db822d3176affd203d70319c0.scss
	  ...
      _b80300e1a5f290f6a850e35874068427.scss

Part of: openedx#32292
Yagnesh1998 pushed a commit to ManpraXSoftware/edx-platform that referenced this issue Jun 8, 2023
Similar to openedx#32287,
this change removes another unnecessary step from the
`xmodule_assets` script. The script, which generates XModule
SCSS "entrypoint" files (synthesizing one or more "source" SCSS
resources), was appending MD5 hashes to each SCSS entrypoint filename:

    common/static/xmodule/descriptors/scss:
       AboutBlockStudio.768623f4d8d73dfb637fc94583adb990.scss
       ...
       WordCloudBlockStudio.d41d8cd98f00b204e9800998ecf8427e.scss
    common/static/xmodule/modules/scss:
       AboutBlockPreview.05a6cbd5c10100a245fa2cbf151b9770.scss
       ...
       WordCloudBlockPreview.7b899a56a70d29c58cf14b7e1888a0ec.scss

It's unclear why these MD5 hashes needed to be appended.
A comment in xmodule/static_content.py hints that it might have
something to do with de-duplication, but that doesn't make any sense,
because each XModule has exactly two SCSS entrypoint files (one for
studio_view and one for other student/author_views) and none of those
entrypoint files can possibly be shared between XModules.

Soon, as part of deleting the `xmodule_assets` script,
we would like to just check these SCSS files into version control
rather than generating them. In order to do that, we will need to
drop the hashes. This commit does that.
The new output looks like this:

    common/static/xmodule/descriptors:
       AboutBlockStudio.scss
       ...
       WordCloudBlockStudio.scss
    common/static/xmodule/modules:
       AboutBlockPreview.scss
       ...
       WordCloudBlockPreview.scss

Part of: openedx#32292
Yagnesh1998 pushed a commit to ManpraXSoftware/edx-platform that referenced this issue Jun 8, 2023
The `xmodule_assets` command copies SCSS files from
xmodule/css to common/static/xmodule/{modules|descriptors}/scss.
It renames the files to the format:

     _{INDEX}-{HASH}.scss

where an XModule's first SCSS resource will have INDEX==0,
the next will have INDEX==1, ...and that's it because no
XModule has more than two SCSS resources.
The output looks like this:

    common/static/xmodule/descriptors/scss:
      _000-808fcbb4c5109c5156ae3c0c9729c8be.scss
      ...
      _001-a10fc3e0fd6aca63426a89e75fe69c31.scss
    common/static/xmodule/modules/scss:
      _000-1ad2f05db822d3176affd203d70319c0.scss
	  ...
      _001-482ebc752ab6e41946651ceb0f3e7f55.scss

These indexes serve no purpose. Reading the comments
and git-blame in xmodule/static_content.py, one can glean
that the indexes might have been intended to enforce
dependency relationships between the assets, but
this is unnecessary, because the ordering of the copied
SCSS is *already preserved* by the order which they're
included into the `{BLOCK_NAME}{Studio|Preivew}.{HASH}.scss`
SCSS entrypoint files. I have to assume that this is an
unnecessary relic from the time when the XModule system
was more heavily utilized, rather than just a legacy corner
of the XBlock framework as it is today.
So, we remove the indexes, which lets us simplify the logic
of xmodule/static_content.py. This is a minor refactoring, but it'll
make it easier for the next steps on our way to deleting
xmodule/static_content.py entirely. The new output looks like this:

    common/static/xmodule/descriptors/scss:
      _808fcbb4c5109c5156ae3c0c9729c8be.scss
	  ...
      _d41921b4c5d45188759ef3d04fd9a78a.scss
    common/static/xmodule/modules/scss:
      _1ad2f05db822d3176affd203d70319c0.scss
	  ...
      _b80300e1a5f290f6a850e35874068427.scss

Part of: openedx#32292
Yagnesh1998 pushed a commit to ManpraXSoftware/edx-platform that referenced this issue Jun 8, 2023
Similar to openedx#32287,
this change removes another unnecessary step from the
`xmodule_assets` script. The script, which generates XModule
SCSS "entrypoint" files (synthesizing one or more "source" SCSS
resources), was appending MD5 hashes to each SCSS entrypoint filename:

    common/static/xmodule/descriptors/scss:
       AboutBlockStudio.768623f4d8d73dfb637fc94583adb990.scss
       ...
       WordCloudBlockStudio.d41d8cd98f00b204e9800998ecf8427e.scss
    common/static/xmodule/modules/scss:
       AboutBlockPreview.05a6cbd5c10100a245fa2cbf151b9770.scss
       ...
       WordCloudBlockPreview.7b899a56a70d29c58cf14b7e1888a0ec.scss

It's unclear why these MD5 hashes needed to be appended.
A comment in xmodule/static_content.py hints that it might have
something to do with de-duplication, but that doesn't make any sense,
because each XModule has exactly two SCSS entrypoint files (one for
studio_view and one for other student/author_views) and none of those
entrypoint files can possibly be shared between XModules.

Soon, as part of deleting the `xmodule_assets` script,
we would like to just check these SCSS files into version control
rather than generating them. In order to do that, we will need to
drop the hashes. This commit does that.
The new output looks like this:

    common/static/xmodule/descriptors:
       AboutBlockStudio.scss
       ...
       WordCloudBlockStudio.scss
    common/static/xmodule/modules:
       AboutBlockPreview.scss
       ...
       WordCloudBlockPreview.scss

Part of: openedx#32292
kdmccormick added a commit to kdmccormick/edx-platform that referenced this issue Jun 14, 2023
The `xmodule_assets` command copies SCSS source files from
xmodule/css to common/static/xmodule/scss, renaming them
to `{MD5_HASH}.scss` in order to "remove duplicates".
The copied files are then included into the generated
SCSS entrypoint files (eg AnnotatableBlockStudio.scss).

The "de-deplication" is completely unnecessary: there are
only a couple dozen SCSS files, and none of them are duplicates.
This copying process is confusing, it complicates our
build process, and it makes our SCSS harder to understand.

So, in the generated SCSS entrypoint files, we
stop importing the *copied* SCSS sources, and just
import the *original* SCSS sources instead.
For example, common/static/xmodule/descriptors/scss/AboutBlockStudio.scss
is changed from:

    .xmodule_edit.xmodule_AboutBlock {
      @import "9bdcda00f046f78be79aca7791e1d4fb.scss";
      @import "a10fc3e0fd6aca63426a89e75fe69c31.scss";
    }

to:

    .xmodule_edit.xmodule_AboutBlock {
      @import "editor/edit.scss";
      @import "html/edit.scss";
    }

In order to make the `@import` lines work, we add xmodule/css to the list
of lookup dirs for XModule SCSS compilation. We also remove the
copying logic from `xmodule_assets`, as it is no longer needed.

Part of: openedx#32292
kdmccormick added a commit that referenced this issue Jun 14, 2023
The `xmodule_assets` command copies SCSS source files from
xmodule/css to common/static/xmodule/scss, renaming them
to `{MD5_HASH}.scss` in order to "remove duplicates".
The copied files are then included into the generated
SCSS entrypoint files (eg AnnotatableBlockStudio.scss).

The "de-deplication" is completely unnecessary: there are
only a couple dozen SCSS files, and none of them are duplicates.
This copying process is confusing, it complicates our
build process, and it makes our SCSS harder to understand.

So, in the generated SCSS entrypoint files, we
stop importing the *copied* SCSS sources, and just
import the *original* SCSS sources instead.
For example, common/static/xmodule/descriptors/scss/AboutBlockStudio.scss
is changed from:

    .xmodule_edit.xmodule_AboutBlock {
      @import "9bdcda00f046f78be79aca7791e1d4fb.scss";
      @import "a10fc3e0fd6aca63426a89e75fe69c31.scss";
    }

to:

    .xmodule_edit.xmodule_AboutBlock {
      @import "editor/edit.scss";
      @import "html/edit.scss";
    }

In order to make the `@import` lines work, we add xmodule/css to the list
of lookup dirs for XModule SCSS compilation. We also remove the
copying logic from `xmodule_assets`, as it is no longer needed.

Part of: #32292
kdmccormick added a commit to kdmccormick/edx-platform that referenced this issue Jun 16, 2023
Now that all XModule SCSS is located in xmodule/static/sass,
it would make sense to co-locate the CSS there as well.

We also add a README to explain the purpose of this new folder.

In the future, we will move xmodule/js and xmodule/assets
into xmodule/static as well.

Part of: openedx#32292
kdmccormick added a commit that referenced this issue Jun 20, 2023
Now that all XModule SCSS is located in xmodule/static/sass,
it would make sense to co-locate the CSS there as well.

We also add a README to explain the purpose of this new folder.

In the future, we will move xmodule/js and xmodule/assets
into xmodule/static as well.

Part of: #32292
@kdmccormick
Copy link
Member Author

This is done!

kdmccormick added a commit that referenced this issue Jun 21, 2023
…" (#32526)

This reverts commit 5671dab.

The original PR is causing styling issues due to broken CSS references
on studio.edx.org.

See #32292 for follow-up.
@kdmccormick kdmccormick reopened this Jun 22, 2023
aht007 pushed a commit that referenced this issue Jul 4, 2023
…" (#32526)

This reverts commit 5671dab.

The original PR is causing styling issues due to broken CSS references
on studio.edx.org.

See #32292 for follow-up.
kdmccormick added a commit to kdmccormick/edx-platform that referenced this issue Jul 6, 2023
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
kdmccormick added a commit to kdmccormick/edx-platform that referenced this issue Jul 6, 2023
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
kdmccormick added a commit that referenced this issue Jul 6, 2023
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. #32018
2. #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: #32292
kdmccormick added a commit that referenced this issue Jul 6, 2023
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
@kdmccormick
Copy link
Member Author

Closing, now that #32592 is merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

No branches or pull requests

1 participant