From 516eff0633f36c2deb7cae3ff153917185de1c60 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andrey=20Ca=C3=B1on?= Date: Thu, 18 May 2023 08:00:44 -0500 Subject: [PATCH] Decouple XModule styles from LMS/Studio styles (attempt 3) (#32237) This basically changes how the xmodule static files are generated and consumed in order to separate the Xblock styles from general style files. Includes: * build: decople XModule style assets by using a custom webpack loader * build: move scss imports to its specific file * build: fix: add system dirs to theme lookup paths. (fixes attempt 1) * build: fix: use bootstrap variables instead of lms variables (fixes attempt 2) This is an amendment to #32188, which itself was an amendment to #32018. Addressing the issue https://github.com/openedx/edx-platform/issues/31624 --- cms/envs/common.py | 3 +- cms/static/sass/_build-v1.scss | 2 - lms/envs/common.py | 3 +- lms/static/sass/_build-course.scss | 2 - pavelib/assets.py | 23 ++++++++ xmodule/css/annotatable/display.scss | 5 ++ xmodule/css/capa/display.scss | 4 ++ xmodule/css/editor/edit.scss | 7 +++ xmodule/css/html/display.scss | 4 ++ xmodule/css/lti/lti.scss | 6 ++ xmodule/css/poll/display.scss | 5 ++ xmodule/css/sequence/display.scss | 7 +++ xmodule/css/tabs/tabs.scss | 6 ++ xmodule/css/video/accessible_menu.scss | 2 + xmodule/css/video/display.scss | 5 ++ xmodule/css/word_cloud/display.scss | 5 ++ xmodule/static_content.py | 17 +++--- xmodule/util/xmodule_django.py | 80 ++++++++++++++++++++++++++ 18 files changed, 172 insertions(+), 14 deletions(-) diff --git a/cms/envs/common.py b/cms/envs/common.py index 4b7282059499..a01c42517420 100644 --- a/cms/envs/common.py +++ b/cms/envs/common.py @@ -1439,7 +1439,8 @@ WEBPACK_LOADER = { 'DEFAULT': { 'BUNDLE_DIR_NAME': 'bundles/', - 'STATS_FILE': os.path.join(STATIC_ROOT, 'webpack-stats.json') + 'STATS_FILE': os.path.join(STATIC_ROOT, 'webpack-stats.json'), + 'LOADER_CLASS': 'xmodule.util.xmodule_django.XModuleWebpackLoader', }, 'WORKERS': { 'BUNDLE_DIR_NAME': 'bundles/', diff --git a/cms/static/sass/_build-v1.scss b/cms/static/sass/_build-v1.scss index 2a32c688e5a1..fefd095e21a7 100644 --- a/cms/static/sass/_build-v1.scss +++ b/cms/static/sass/_build-v1.scss @@ -84,8 +84,6 @@ // +Xmodule // ==================== -@import 'xmodule/modules/css/module-styles.scss'; -@import 'xmodule/descriptors/css/module-styles.scss'; @import 'xmodule/headings'; @import 'elements/xmodules'; // styling for Studio-specific contexts @import 'developer'; // used for any developer-created scss that needs further polish/refactoring diff --git a/lms/envs/common.py b/lms/envs/common.py index b4b7ae1794d9..89c9bb6d1c14 100644 --- a/lms/envs/common.py +++ b/lms/envs/common.py @@ -2695,7 +2695,8 @@ def _make_locale_paths(settings): # pylint: disable=missing-function-docstring WEBPACK_LOADER = { 'DEFAULT': { 'BUNDLE_DIR_NAME': 'bundles/', - 'STATS_FILE': os.path.join(STATIC_ROOT, 'webpack-stats.json') + 'STATS_FILE': os.path.join(STATIC_ROOT, 'webpack-stats.json'), + 'LOADER_CLASS': 'xmodule.util.xmodule_django.XModuleWebpackLoader', }, 'WORKERS': { 'BUNDLE_DIR_NAME': 'bundles/', diff --git a/lms/static/sass/_build-course.scss b/lms/static/sass/_build-course.scss index 103e299b2e93..732eb2caf888 100644 --- a/lms/static/sass/_build-course.scss +++ b/lms/static/sass/_build-course.scss @@ -26,7 +26,6 @@ @import 'course/base/mixins'; @import 'course/base/base'; @import 'course/base/extends'; -@import 'xmodule/modules/css/module-styles.scss'; @import 'course/courseware/courseware'; @import 'course/courseware/sidebar'; @import 'course/courseware/amplifier'; @@ -57,7 +56,6 @@ @import "course/gradebook"; @import "course/instructor/instructor_2"; @import "course/instructor/email"; -@import "xmodule/descriptors/css/module-styles.scss"; // course - ccx_coach @import "course/ccx_coach/dashboard"; diff --git a/pavelib/assets.py b/pavelib/assets.py index 003c76487585..991fb7f914b1 100644 --- a/pavelib/assets.py +++ b/pavelib/assets.py @@ -170,6 +170,8 @@ def get_theme_sass_dirs(system, theme_dir): css_dir = theme_dir / system / "static" / "css" certs_sass_dir = theme_dir / system / "static" / "certificates" / "sass" certs_css_dir = theme_dir / system / "static" / "certificates" / "css" + xmodule_sass_folder = "modules" if system == 'lms' else "descriptors" + xmodule_sass_dir = path("common") / "static" / "xmodule" / xmodule_sass_folder / "scss" dependencies = SASS_LOOKUP_DEPENDENCIES.get(system, []) if sass_dir.isdir(): @@ -197,6 +199,16 @@ def get_theme_sass_dirs(system, theme_dir): ], }) + dirs.append({ + "sass_source_dir": xmodule_sass_dir, + "css_destination_dir": path("common") / "static" / "css" / "xmodule", + "lookup_paths": dependencies + [ + sass_dir / "partials", + system_sass_dir / "partials", + system_sass_dir, + ], + }) + # now compile theme sass files for certificate if system == 'lms': dirs.append({ @@ -223,6 +235,8 @@ def get_system_sass_dirs(system): dirs = [] sass_dir = path(system) / "static" / "sass" css_dir = path(system) / "static" / "css" + xmodule_sass_folder = "modules" if system == 'lms' else "descriptors" + xmodule_sass_dir = path("common") / "static" / "xmodule" / xmodule_sass_folder / "scss" dependencies = SASS_LOOKUP_DEPENDENCIES.get(system, []) dirs.append({ @@ -234,6 +248,15 @@ def get_system_sass_dirs(system): ], }) + dirs.append({ + "sass_source_dir": xmodule_sass_dir, + "css_destination_dir": path("common") / "static" / "css" / "xmodule", + "lookup_paths": dependencies + [ + sass_dir / "partials", + sass_dir, + ], + }) + if system == 'lms': dirs.append({ "sass_source_dir": path(system) / "static" / "certificates" / "sass", diff --git a/xmodule/css/annotatable/display.scss b/xmodule/css/annotatable/display.scss index f35a16035639..124011e2bd2b 100644 --- a/xmodule/css/annotatable/display.scss +++ b/xmodule/css/annotatable/display.scss @@ -4,6 +4,11 @@ * that the LMS did, so the quick fix was to localize the LMS variables not shared by the CMS. * -Abarrett and Vshnayder */ +@import 'bourbon/bourbon'; +@import 'lms/theme/variables'; +@import 'bootstrap/scss/variables'; +@import 'lms/theme/variables-v1'; + $annotatable--border-color: $gray-l3; $annotatable--body-font-size: em(14); diff --git a/xmodule/css/capa/display.scss b/xmodule/css/capa/display.scss index b8536a40d159..990ba28b2ab2 100644 --- a/xmodule/css/capa/display.scss +++ b/xmodule/css/capa/display.scss @@ -18,7 +18,11 @@ // * +Problem - Choice Text Group // * +Problem - Image Input Overrides // * +Problem - Annotation Problem Overrides +@import 'vendor/bi-app/bi-app-ltr'; @import 'bourbon/bourbon'; +@import 'lms/theme/variables'; +@import 'bootstrap/scss/variables'; +@import 'lms/theme/variables-v1'; // +Variables - Capa // ==================== diff --git a/xmodule/css/editor/edit.scss b/xmodule/css/editor/edit.scss index cd740c41171c..71699776522d 100644 --- a/xmodule/css/editor/edit.scss +++ b/xmodule/css/editor/edit.scss @@ -1,3 +1,10 @@ +@import 'vendor/bi-app/bi-app-ltr'; +@import 'bourbon/bourbon'; +@import 'lms/theme/variables'; +@import 'bootstrap/scss/variables'; +@import 'cms/theme/variables-v1'; +@import 'mixins'; + // This is shared CSS between the xmodule problem editor and the xmodule HTML editor. .editor { position: relative; diff --git a/xmodule/css/html/display.scss b/xmodule/css/html/display.scss index d9e123e6d5e0..25e2ce4fbd64 100644 --- a/xmodule/css/html/display.scss +++ b/xmodule/css/html/display.scss @@ -1,4 +1,8 @@ +@import 'vendor/bi-app/bi-app-ltr'; @import 'bourbon/bourbon'; +@import 'lms/theme/variables'; +@import 'bootstrap/scss/variables'; +@import 'lms/theme/variables-v1'; // HTML component display: * { diff --git a/xmodule/css/lti/lti.scss b/xmodule/css/lti/lti.scss index 88540aee09e9..4bd2c41317f5 100644 --- a/xmodule/css/lti/lti.scss +++ b/xmodule/css/lti/lti.scss @@ -1,3 +1,9 @@ +@import 'bourbon/bourbon'; +@import 'lms/theme/variables'; +@import 'bootstrap/scss/variables'; +@import 'lms/theme/variables-v1'; +@import 'base/mixins'; + h2.problem-header { display: inline-block; } diff --git a/xmodule/css/poll/display.scss b/xmodule/css/poll/display.scss index cf46fcf3bf4d..7c07f89376b2 100644 --- a/xmodule/css/poll/display.scss +++ b/xmodule/css/poll/display.scss @@ -1,3 +1,8 @@ +@import 'bourbon/bourbon'; +@import 'lms/theme/variables'; +@import 'bootstrap/scss/variables'; +@import 'lms/theme/variables-v1'; + div.poll_question { @media print { display: block; diff --git a/xmodule/css/sequence/display.scss b/xmodule/css/sequence/display.scss index 55a24eadad85..3ddda8b37d09 100644 --- a/xmodule/css/sequence/display.scss +++ b/xmodule/css/sequence/display.scss @@ -1,3 +1,10 @@ +@import 'vendor/bi-app/bi-app-ltr'; +@import 'bourbon/bourbon'; +@import 'lms/theme/variables'; +@import 'bootstrap/scss/variables'; +@import 'bootstrap/scss/mixins/breakpoints'; +@import 'lms/theme/variables-v1'; + $seq-nav-border-color: $border-color !default; $seq-nav-hover-color: rgb(245, 245, 245) !default; $seq-nav-link-color: $link-color !default; diff --git a/xmodule/css/tabs/tabs.scss b/xmodule/css/tabs/tabs.scss index e8b025442de4..ad47d915a230 100644 --- a/xmodule/css/tabs/tabs.scss +++ b/xmodule/css/tabs/tabs.scss @@ -1,4 +1,10 @@ // styles duped from _unit.scss - Edit Header (Component Name, Mode-Editor, Mode-Settings) +@import 'vendor/bi-app/bi-app-ltr'; +@import 'bourbon/bourbon'; +@import 'lms/theme/variables'; +@import 'bootstrap/scss/variables'; +@import 'cms/theme/variables-v1'; +@import 'mixins'; .tabs-wrapper { diff --git a/xmodule/css/video/accessible_menu.scss b/xmodule/css/video/accessible_menu.scss index c9b56604c994..f7153fa98429 100644 --- a/xmodule/css/video/accessible_menu.scss +++ b/xmodule/css/video/accessible_menu.scss @@ -1,3 +1,5 @@ +@import 'base/mixins'; + $a11y--gray: rgb(127, 127, 127); $a11y--blue: rgb(0, 159, 230); $a11y--gray-d1: shade($gray, 20%); diff --git a/xmodule/css/video/display.scss b/xmodule/css/video/display.scss index 80292f1f230c..467c5960751e 100644 --- a/xmodule/css/video/display.scss +++ b/xmodule/css/video/display.scss @@ -8,7 +8,12 @@ // -------- // Defaults: what displays if the icon font doesn't load. // -------- +@import 'vendor/bi-app/bi-app-ltr'; @import 'bourbon/bourbon'; +@import 'lms/theme/variables'; +@import 'bootstrap/scss/variables'; +@import 'bootstrap/scss/mixins/breakpoints'; +@import 'lms/theme/variables-v1'; // the html target is necessary for xblocks and xmodules, but works across the board diff --git a/xmodule/css/word_cloud/display.scss b/xmodule/css/word_cloud/display.scss index 154dde27c331..0b8940ab9abe 100644 --- a/xmodule/css/word_cloud/display.scss +++ b/xmodule/css/word_cloud/display.scss @@ -1,3 +1,8 @@ +@import 'bourbon/bourbon'; +@import 'lms/theme/variables'; +@import 'bootstrap/scss/variables'; +@import 'lms/theme/variables-v1'; + .input-cloud { margin: ($baseline/4); } diff --git a/xmodule/static_content.py b/xmodule/static_content.py index de705320de51..2447347da90f 100755 --- a/xmodule/static_content.py +++ b/xmodule/static_content.py @@ -91,7 +91,7 @@ class VideoBlock(HTMLSnippet): # lint-amnesty, pylint: disable=abstract-method def write_module_styles(output_root): """Write all registered XModule css, sass, and scss files to output root.""" - return _write_styles('.xmodule_display', output_root, XBLOCK_CLASSES, 'get_preview_view_css') + return _write_styles('.xmodule_display', output_root, XBLOCK_CLASSES, 'get_preview_view_css', 'Preview') def write_module_js(output_root): @@ -101,7 +101,7 @@ def write_module_js(output_root): def write_descriptor_styles(output_root): """Write all registered XModuleDescriptor css, sass, and scss files to output root.""" - return _write_styles('.xmodule_edit', output_root, XBLOCK_CLASSES, 'get_studio_view_css') + return _write_styles('.xmodule_edit', output_root, XBLOCK_CLASSES, 'get_studio_view_css', 'Studio') def write_descriptor_js(output_root): @@ -120,7 +120,7 @@ def _ensure_dir(directory): raise -def _write_styles(selector, output_root, classes, css_attribute): +def _write_styles(selector, output_root, classes, css_attribute, suffix): """ Write the css fragments from all XModules in `classes` into `output_root` as individual files, hashed by the contents to remove @@ -147,17 +147,18 @@ def _write_styles(selector, output_root, classes, css_attribute): for class_ in classes: css_imports[class_].add(fragment_name) - module_styles_lines = [] - for class_, fragment_names in sorted(css_imports.items()): + module_styles_lines = [] + fragment_names = sorted(fragment_names) module_styles_lines.append("""{selector}.xmodule_{class_} {{""".format( class_=class_, selector=selector )) module_styles_lines.extend(f' @import "{name}";' for name in fragment_names) module_styles_lines.append('}') + file_hash = hashlib.md5("".join(fragment_names).encode('ascii')).hexdigest() - contents['_module-styles.scss'] = '\n'.join(module_styles_lines) + contents[f"{class_}{suffix}.{file_hash}.scss"] = '\n'.join(module_styles_lines) _write_files(output_root, contents) @@ -305,9 +306,9 @@ def main(): root = path(args['']) descriptor_files = write_descriptor_js(root / 'descriptors/js') - write_descriptor_styles(root / 'descriptors/css') + write_descriptor_styles(root / 'descriptors/scss') module_files = write_module_js(root / 'modules/js') - write_module_styles(root / 'modules/css') + write_module_styles(root / 'modules/scss') write_webpack(root / 'webpack.xmodule.config.js', module_files, descriptor_files) diff --git a/xmodule/util/xmodule_django.py b/xmodule/util/xmodule_django.py index e09ea3a6e981..248e10bf981a 100644 --- a/xmodule/util/xmodule_django.py +++ b/xmodule/util/xmodule_django.py @@ -5,9 +5,89 @@ """ +from os import listdir + import webpack_loader # NOTE: we are importing this method so that any module that imports us has access to get_current_request from crum import get_current_request +from django.conf import settings +from webpack_loader.loader import WebpackLoader + + +class XModuleWebpackLoader(WebpackLoader): + """ + Custom webpack loader that consumes the output generated by webpack-bundle-tracker + and the files from XModule style generation stage. Briefly, this allows to use the + generated js bundles and compiled assets for XModule-style during Xblocks rendering. + """ + + def load_assets(self): + """ + This function will append XModule css files to the standard load_assets results. + + The standard WebpackLoader load_assets method returns a dictionary like the following: + + { + 'status': 'done', + 'chunks': { + 'AnnotatableBlockPreview': [ + { + 'name': 'AnnotatableBlockPreview.js', + 'path': '/openedx/edx-platform/common/static/bundles/AnnotatableBlockPreview.js' + }, + { + 'name': 'AnnotatableBlockPreview.js.map', + 'path': '/openedx/edx-platform/common/static/bundles/AnnotatableBlockPreview.js.map' + } + ], + ... + } + } + + Chunks key contains the data for every file in /openedx/edx-platform/common/static/bundles/, that + is a folder created during the compilation theme, this method will append the listed files in + common/static/css/xmodule to the correspondent key, the result will be the following: + + { + 'status': 'done', + 'chunks': { + 'AnnotatableBlockPreview': [ + { + 'name': 'AnnotatableBlockPreview.js', + 'path': '/openedx/edx-platform/common/static/bundles/AnnotatableBlockPreview.js' + }, + { + 'name': 'AnnotatableBlockPreview.js.map', + 'path': '/openedx/edx-platform/common/static/bundles/AnnotatableBlockPreview.js.map' + }, + { + 'name': 'AnnotatableBlockPreview.85745121.css', + 'path': 'common/static/css/xmodule/AnnotatableBlockPreview.85745121.css', + 'publicPath': '/static/css/xmodule/AnnotatableBlockPreview.85745121.css' + } + ], + ... + } + } + + Returns: + dict: Assets dictionary as described above. + """ + assets = super().load_assets() + + css_path = "common/static/css/xmodule" + css_files = listdir(css_path) + + for css_file in css_files: + name = css_file.split(".")[0] + css_chunk = { + "name": css_file, + "path": f"{css_path}/{css_file}", + "publicPath": f"{settings.STATIC_URL}css/xmodule/{css_file}", + } + assets["chunks"][name].append(css_chunk) + + return assets def get_current_request_hostname():