Decouple XModule styles from LMS/Studio styles

[andrey-canon]

Description

Addressing this issue #31624

This basically changes how the xmodule static files are generated and consumed in order to separate the Xblock styles from general style files.

Generation

Run command openedx-assets xmodule

Current generation basically takes the preview_view_css or studio_view_css value from the XBlock class and creates hashed files in /common/static/xmodule/{descriptors or modules}/css folder then those files are imported in a common file called _module-styles.scss in the same folder

Example of _module-styles.scss

@import 'bourbon/bourbon';
@import 'lms/theme/variables';
.xmodule_edit.xmodule_AboutBlock {
  @import "000-cd23cf32f86efa1c4179df0201722e55.scss";
  @import "001-a10fc3e0fd6aca63426a89e75fe69c31.scss";
}
.xmodule_edit.xmodule_AnnotatableBlock {
  @import "000-d41921b4c5d45188759ef3d04fd9a78a.scss";
}
.xmodule_edit.xmodule_CourseInfoBlock {
  @import "000-cd23cf32f86efa1c4179df0201722e55.scss";
  @import "001-a10fc3e0fd6aca63426a89e75fe69c31.scss";
}
.xmodule_edit.xmodule_CustomTagBlock {
  @import "000-d41921b4c5d45188759ef3d04fd9a78a.scss";
}
.xmodule_edit.xmodule_HtmlBlock {
  @import "000-cd23cf32f86efa1c4179df0201722e55.scss";
  @import "001-a10fc3e0fd6aca63426a89e75fe69c31.scss";
}
.xmodule_edit.xmodule_ProblemBlock {
  @import "000-cd23cf32f86efa1c4179df0201722e55.scss";
  @import "001-901b985e5ea2dea2a89cce747cf4307d.scss";
}
.xmodule_edit.xmodule_StaticTabBlock {
  @import "000-cd23cf32f86efa1c4179df0201722e55.scss";
  @import "001-a10fc3e0fd6aca63426a89e75fe69c31.scss";
}
.xmodule_edit.xmodule_VideoBlock {
  @import "000-e5d1b330372214d8a7b774e826640db5.scss";
}

This pr changes that in the following way

1. the destination folder will be scss instead of css
2. hashed files are kept
3. _module-styles.scss is removed and an independent file is created per Xblock
4. Required scss files are imported

Example

Compilation

Run command openedx-assets common

Previously module-styles.scss files were imported in lms and cms scss files, after running the openedx-assets common command that generated the lms-course.css and studio-main-v1.css files with the xblocks styles

This pr changes that in the following way:

1. module-styles.scss files were removed from lms and cms, therefore lms-course.css and studio-main-v1.css won't contain the Xblock styles anymore
2. Xmodule paths were added (common/static/xmodule/{modules or descriptors}/scss) to the compilation dirs. This will generate a compiled css file for each scss file in those folders
3. Compiled file are store in common/static/css/xmodule/ that's why studio needs access to the lms styles in the preview

Example

Consumption

Currently every xblock loads its js by using this function, that functions also allows to load CSS, so this pr adds a custom webpack loader in order to include the CSS files generated previously. XmoduleWebpackLoader will append the CSS files in common/stactic/css/xmodule to assets found by the standard webpack loader

Results

Testing instructions

This instruction are just valid in a tutor environment

1. Checkout this branch
2. go inside the lms docker image, example, docker exec -it tutor_nightly_dev-lms-1 /bin/bash
3. Run openedx-assets xmodule and check the generation part
4. Run openedx-assets common and check the compilation part
5. Finally go to studio or lms and check Xblock styles. keep in mind that this is just for the native xblock in this list

Other information

More details about generation and compilation static files could be found here

[openedx-webhooks]

Thanks for the pull request, @andrey-canon! Please note that it may take us up to several weeks or months to complete a review and merge your PR.

Feel free to add as much of the following information to the ticket as you can:

• supporting documentation
• Open edX discussion forum threads
• timeline information ("this must be merged by XX date", and why that is)
• partner information ("this is a course on edx.org")
• any other information that can help Product understand the context for the PR

All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here.

Please let us know once your PR is ready for our review and all tests are green.

[e0d]

@andrey-canon Looks like there are a couple of check failures, can you have a look?

[andrey-canon]

Hi @kdmccormick, @e0d, this is ready for review

[kdmccormick]

Sorry @andrey-canon , Axim got together in-person last week and I was not able to do any code review. I have not forgotten about this.

[kdmccormick]

This is excellent. I am still testing it locally, but here are the only issues I found with the code itself.

Once this merges and the styles are decoupled, what do you think the next step should be?

[kdmccormick]

It's standard to capitalize the "M": XModuleWebpackLoader

[andrey-canon]

Renamed

[kdmccormick]

For docstrings, our Python style guide recommends breaking the line after the opening """:

Suggested change

	"""Custom webpack loader.This is where behavior can be customized
	"""
	Custom webpack loader.This is where behavior can be customized

[andrey-canon]

Changed

[kdmccormick]

Can you be more specific in this docstring? It is not just any old edx-platform static files which you handle here: it is specifically assets for XModule-style XBlocks.

[andrey-canon]

Please check last commit, also I improved the docstring for load_assets method

[kdmccormick]

@andrey-canon your test instructions mention:

Add WEBPACK_LOADER['DEFAULT']['LOADER_CLASS'] = "xmodule.util.xmodule_django.XmoduleWebpackLoader" to your settings

This is unnecessary, as your PR already makes this change, right?

[andrey-canon]

@andrey-canon your test instructions mention:

Add WEBPACK_LOADER['DEFAULT']['LOADER_CLASS'] = "xmodule.util.xmodule_django.XmoduleWebpackLoader" to your settings

This is unnecessary, as your PR already makes this change, right?

@kdmccormick yes, you are right that's unnecessary I added that because when I was developing I had to and I just copied some notes in the testing instructions

[kdmccormick]

@andrey-canon I tested this and couldn't find any issues, nice work!

If you rebase & apply the requested changes, then I should be able to merge this within the next couple of days.

[andrey-canon]

@andrey-canon I tested this and couldn't find any issues, nice work!

If you rebase & apply the requested changes, then I should be able to merge this within the next couple of days.

@kdmccormick Great, just let me know to squash the commits

[andrey-canon]

This is excellent. I am still testing it locally, but here are the only issues I found with the code itself.

Once this merges and the styles are decoupled, what do you think the next step should be?

@kdmccormick, IMO this requires a style refactor, personally I don't like these module_styles_lines and descriptor module_styles_lines it'd be great if both of them depend on just one standard import or the same list of imports. In a bigger picture I don't have the answer right know, probably the next step should be identify and decouple more assets in order to do the loading process lighter or something more complex like try to migrate some xblocks to react applications since there are more independent from other assets.

[kdmccormick]

Looks great! Just one suggestion. Go ahead and squash, and I'll look into when I can safely merge this.

[kdmccormick]

This is a really helpful docstring, thank you!

[kdmccormick]

Suggested change

	module_styles_lines = [
	"@import 'bourbon/bourbon';",
	"@import 'vendor/bi-app/bi-app-ltr';",
	"@import 'lms/theme/variables';",
	"@import 'cms/theme/variables-v1';",
	"@import 'mixins';",
	"@import 'mixins-inherited';",
	]
	return _write_styles(
	'.xmodule_edit',
	output_root, XBLOCK_CLASSES,
	'get_studio_view_css',
	module_styles_lines,
	descriptor_styles_lines = [
	"@import 'bourbon/bourbon';",
	"@import 'vendor/bi-app/bi-app-ltr';",
	"@import 'lms/theme/variables';",
	"@import 'cms/theme/variables-v1';",
	"@import 'mixins';",
	"@import 'mixins-inherited';",
	]
	return _write_styles(
	'.xmodule_edit',
	output_root, XBLOCK_CLASSES,
	'get_studio_view_css',
	descriptor_styles_lines,

[kdmccormick]

(I had left a comment about next steps, but I moved it over to the issue since it's beyond this PR: #31624 (comment))

[kdmccormick]

@andrey-canon I was wrong, due to some vacations at 2U and some large XModule refactorings that are in line to be merged first, we'll have to wait in order to merge this with proper 2U on-call support. I'll merge it late April or early May.

In the meantime, it looks like there is a JS test failure. Just ping me if you need approval for the tests to run at any point.

[andrey-canon]

Looks great! Just one suggestion. Go ahead and squash, and I'll look into when I can safely merge this.

Hi @kdmccormick, I have already applied the suggestion and I made the squash :)

[kdmccormick]

Thanks @andrey-canon ! Would you mind adding some details to the body of the commit message as well as a link to the issue?

[andrey-canon]

@andrey-canon I was wrong, due to some vacations at 2U and some large XModule refactorings that are in line to be merged first, we'll have to wait in order to merge this with proper 2U on-call support. I'll merge it late April or early May.

In the meantime, it looks like there is a JS test failure. Just ping me if you need approval for the tests to run at any point.

ok, I think at that time we will need a branch rebase, but no problem just let me know when to do that

Regarding the JS test I think we are facing a flaky test since I had the same failure three days ago and the solution was the same as today, make a rebase.

[andrey-canon]

Thanks @andrey-canon ! Would you mind adding some details to the body of the commit message as well as a link to the issue?

Done

[kdmccormick]

Code looks great. I'm just testing this locally one last time. BTW, I re-named the PR so it'll be easier for folks who aren't on this project to understand.

[jristau1984]

@kdmccormick have you confirmed this change fully supports the existing requirements for global CSS of a course? If so, then this is able to be merged at your convenience. Thanks!

[kdmccormick]

Good call out @jristau1984 , I'm testing that now.

[kdmccormick]

@jristau1984 I was able to confirm that course-wide styling still works. Here was my test process:

• Spin up two deployments, one running master and one running this branch.

• In the Demo course of each deployment, upload course-wide-test.css:

  h1 {
  	color: red !important;
  }
  h2 {
  	color: orange !important;
  }
  h3 {
  	color: green !important;
  }
  h4 {
  	color: cyan !important;
  }
  h5 {
  	color: magenta !important:
  }

• Copy the Web URL for the CSS and add to Advanced Settings > Custom CSS. Save.

• Browse the demo course and confirm that heading colors are changed.

• Using the browser dev tools, inspect a heading, and compare the rules CSS listed for both master and this branch. Confirm that resolution order of the custom CSS rules has not changed, even though on this branch, block-specific rules are defined in via their own SCSS file rather than lms-course.scss

I'll merge this ~8AM EST tomorrow.

[openedx-webhooks]

@andrey-canon 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future.

[kdmccormick]

This is being reverted in #32183, as red-theme compilation failed when building for deployment to edx.org.

[edx-pipeline-bot]

EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production.

[edx-pipeline-bot]

EdX Release Notice: This PR has been deployed to the production environment.

[kdmccormick]

Will re-merge in: #32188