From 5f0523253053a833b79144e7ed58ac533b20f2da Mon Sep 17 00:00:00 2001 From: Kyle McCormick Date: Tue, 27 Jun 2023 09:59:54 -0400 Subject: [PATCH] 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 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 * `/[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 `BlockPreview`, is now `BlockDisplay`. * studio_view: was `BlockStudio`, is now `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//*`: 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. https://github.com/openedx/edx-platform/pull/32018 2. https://github.com/openedx/edx-platform/issues/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: https://github.com/openedx/edx-platform/issues/32292 --- cms/djangoapps/contentstore/views/preview.py | 4 +- cms/envs/common.py | 1 - lms/envs/common.py | 1 - .../core/djangoapps/xblock/runtime/runtime.py | 2 +- openedx/core/lib/xblock_utils/__init__.py | 27 +++-- pavelib/assets.py | 69 +++++++------ xmodule/README.rst | 11 ++- xmodule/annotatable_block.py | 8 +- .../AnnotatableBlockDisplay.scss} | 0 .../AnnotatableBlockEditor.scss} | 0 .../CustomTagBlockEditor.scss} | 0 xmodule/assets/HtmlBlockDisplay.scss | 6 ++ xmodule/assets/HtmlBlockEditor.scss | 7 ++ .../LTIBlockDisplay.scss} | 0 .../PollBlockDisplay.scss} | 0 .../ProblemBlockDisplay.scss} | 0 .../ProblemBlockEditor.scss} | 0 xmodule/assets/README.rst | 99 +++++++++++++++++++ .../SequenceBlockDisplay.scss} | 0 .../VideoBlockDisplay.scss} | 0 .../VideoBlockEditor.scss} | 0 .../WordCloudBlockDisplay.scss} | 0 .../annotatable/_display.scss} | 0 .../capa/_display.scss} | 0 .../codemirror/_codemirror.scss} | 0 .../edit.scss => assets/editor/_edit.scss} | 0 .../html/_display.scss} | 0 .../html/edit.scss => assets/html/_edit.scss} | 0 .../lti/lti.scss => assets/lti/_lti.scss} | 0 .../poll/_display.scss} | 0 .../edit.scss => assets/problem/_edit.scss} | 0 .../sequence/_display.scss} | 0 .../tabs/_codemirror.scss} | 0 .../tabs/tabs.scss => assets/tabs/_tabs.scss} | 0 .../video/_accessible_menu.scss} | 0 .../video/_display.scss} | 0 .../word_cloud/_display.scss} | 0 xmodule/capa_block.py | 8 +- xmodule/conditional_block.py | 6 +- xmodule/html_block.py | 8 +- xmodule/library_content_block.py | 4 +- xmodule/lti_block.py | 7 +- xmodule/poll_block.py | 5 +- xmodule/seq_block.py | 8 +- xmodule/split_test_block.py | 4 +- xmodule/static/sass/cms/AboutBlockStudio.scss | 4 - .../sass/cms/CourseInfoBlockStudio.scss | 4 - xmodule/static/sass/cms/HtmlBlockStudio.scss | 4 - .../static/sass/cms/StaticTabBlockStudio.scss | 4 - .../static/sass/lms/AboutBlockPreview.scss | 3 - .../sass/lms/CourseInfoBlockPreview.scss | 3 - xmodule/static/sass/lms/HtmlBlockPreview.scss | 3 - .../sass/lms/StaticTabBlockPreview.scss | 3 - xmodule/template_block.py | 5 +- xmodule/tests/test_util_builtin_assets.py | 68 +++++++++++++ xmodule/util/builtin_assets.py | 54 ++++++++++ xmodule/util/xmodule_django.py | 96 ------------------ xmodule/vertical_block.py | 4 +- xmodule/video_block/video_block.py | 10 +- xmodule/word_cloud_block.py | 7 +- xmodule/x_module.py | 8 +- 61 files changed, 355 insertions(+), 210 deletions(-) rename xmodule/{static/sass/lms/AnnotatableBlockPreview.scss => assets/AnnotatableBlockDisplay.scss} (100%) rename xmodule/{static/sass/cms/AnnotatableBlockStudio.scss => assets/AnnotatableBlockEditor.scss} (100%) rename xmodule/{static/sass/cms/CustomTagBlockStudio.scss => assets/CustomTagBlockEditor.scss} (100%) create mode 100644 xmodule/assets/HtmlBlockDisplay.scss create mode 100644 xmodule/assets/HtmlBlockEditor.scss rename xmodule/{static/sass/lms/LTIBlockPreview.scss => assets/LTIBlockDisplay.scss} (100%) rename xmodule/{static/sass/lms/PollBlockPreview.scss => assets/PollBlockDisplay.scss} (100%) rename xmodule/{static/sass/lms/ProblemBlockPreview.scss => assets/ProblemBlockDisplay.scss} (100%) rename xmodule/{static/sass/cms/ProblemBlockStudio.scss => assets/ProblemBlockEditor.scss} (100%) create mode 100644 xmodule/assets/README.rst rename xmodule/{static/sass/lms/SequenceBlockPreview.scss => assets/SequenceBlockDisplay.scss} (100%) rename xmodule/{static/sass/lms/VideoBlockPreview.scss => assets/VideoBlockDisplay.scss} (100%) rename xmodule/{static/sass/cms/VideoBlockStudio.scss => assets/VideoBlockEditor.scss} (100%) rename xmodule/{static/sass/lms/WordCloudBlockPreview.scss => assets/WordCloudBlockDisplay.scss} (100%) rename xmodule/{static/sass/include/annotatable/display.scss => assets/annotatable/_display.scss} (100%) rename xmodule/{static/sass/include/capa/display.scss => assets/capa/_display.scss} (100%) rename xmodule/{static/sass/include/codemirror/codemirror.scss => assets/codemirror/_codemirror.scss} (100%) rename xmodule/{static/sass/include/editor/edit.scss => assets/editor/_edit.scss} (100%) rename xmodule/{static/sass/include/html/display.scss => assets/html/_display.scss} (100%) rename xmodule/{static/sass/include/html/edit.scss => assets/html/_edit.scss} (100%) rename xmodule/{static/sass/include/lti/lti.scss => assets/lti/_lti.scss} (100%) rename xmodule/{static/sass/include/poll/display.scss => assets/poll/_display.scss} (100%) rename xmodule/{static/sass/include/problem/edit.scss => assets/problem/_edit.scss} (100%) rename xmodule/{static/sass/include/sequence/display.scss => assets/sequence/_display.scss} (100%) rename xmodule/{static/sass/include/tabs/codemirror.scss => assets/tabs/_codemirror.scss} (100%) rename xmodule/{static/sass/include/tabs/tabs.scss => assets/tabs/_tabs.scss} (100%) rename xmodule/{static/sass/include/video/accessible_menu.scss => assets/video/_accessible_menu.scss} (100%) rename xmodule/{static/sass/include/video/display.scss => assets/video/_display.scss} (100%) rename xmodule/{static/sass/include/word_cloud/display.scss => assets/word_cloud/_display.scss} (100%) delete mode 100644 xmodule/static/sass/cms/AboutBlockStudio.scss delete mode 100644 xmodule/static/sass/cms/CourseInfoBlockStudio.scss delete mode 100644 xmodule/static/sass/cms/HtmlBlockStudio.scss delete mode 100644 xmodule/static/sass/cms/StaticTabBlockStudio.scss delete mode 100644 xmodule/static/sass/lms/AboutBlockPreview.scss delete mode 100644 xmodule/static/sass/lms/CourseInfoBlockPreview.scss delete mode 100644 xmodule/static/sass/lms/HtmlBlockPreview.scss delete mode 100644 xmodule/static/sass/lms/StaticTabBlockPreview.scss create mode 100644 xmodule/tests/test_util_builtin_assets.py create mode 100644 xmodule/util/builtin_assets.py diff --git a/cms/djangoapps/contentstore/views/preview.py b/cms/djangoapps/contentstore/views/preview.py index 2cde6b33735c..cdfe08966e3f 100644 --- a/cms/djangoapps/contentstore/views/preview.py +++ b/cms/djangoapps/contentstore/views/preview.py @@ -24,7 +24,7 @@ from xmodule.services import SettingsService, TeamsConfigurationService from xmodule.studio_editable import has_author_view from xmodule.util.sandboxing import SandboxService -from xmodule.util.xmodule_django import add_webpack_to_fragment +from xmodule.util.builtin_assets import add_webpack_js_to_fragment from xmodule.x_module import AUTHOR_VIEW, PREVIEW_VIEWS, STUDENT_VIEW, XModuleMixin from cms.djangoapps.xblock_config.models import StudioConfig from cms.djangoapps.contentstore.toggles import individualize_anonymous_user_id, ENABLE_COPY_PASTE_FEATURE @@ -319,7 +319,7 @@ def _studio_wrap_xblock(xblock, view, frag, context, display_name_only=False): 'language': getattr(course, 'language', None) } - add_webpack_to_fragment(frag, "js/factories/xblock_validation") + add_webpack_js_to_fragment(frag, "js/factories/xblock_validation") html = render_to_string('studio_xblock_wrapper.html', template_context) frag = wrap_fragment(frag, html) diff --git a/cms/envs/common.py b/cms/envs/common.py index e69609022fdb..ea01bb2efce1 100644 --- a/cms/envs/common.py +++ b/cms/envs/common.py @@ -1440,7 +1440,6 @@ 'DEFAULT': { 'BUNDLE_DIR_NAME': 'bundles/', '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/envs/common.py b/lms/envs/common.py index 2d5ffa46c993..52cb04d3e5a8 100644 --- a/lms/envs/common.py +++ b/lms/envs/common.py @@ -2715,7 +2715,6 @@ def _make_locale_paths(settings): # pylint: disable=missing-function-docstring 'DEFAULT': { 'BUNDLE_DIR_NAME': 'bundles/', '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/openedx/core/djangoapps/xblock/runtime/runtime.py b/openedx/core/djangoapps/xblock/runtime/runtime.py index 6805c2649113..c1599edaff66 100644 --- a/openedx/core/djangoapps/xblock/runtime/runtime.py +++ b/openedx/core/djangoapps/xblock/runtime/runtime.py @@ -334,7 +334,7 @@ def render(self, block, view_name, context=None): raise PermissionDenied # We also need to override this method because some XBlocks in the - # edx-platform codebase use methods like add_webpack_to_fragment() + # edx-platform codebase use methods from builtin_assets.py, # which create relative URLs (/static/studio/bundles/webpack-foo.js). # We want all resource URLs to be absolute, such as is done when # local_resource_url() is used. diff --git a/openedx/core/lib/xblock_utils/__init__.py b/openedx/core/lib/xblock_utils/__init__.py index 36440e8f6e07..bb7f78e523fc 100644 --- a/openedx/core/lib/xblock_utils/__init__.py +++ b/openedx/core/lib/xblock_utils/__init__.py @@ -11,7 +11,6 @@ import uuid import markupsafe -import webpack_loader.utils from django.conf import settings from django.contrib.auth.models import User # lint-amnesty, pylint: disable=imported-auth-user from django.contrib.staticfiles.storage import staticfiles_storage @@ -29,7 +28,6 @@ from common.djangoapps import static_replace from common.djangoapps.edxmako.shortcuts import render_to_string from xmodule.seq_block import SequenceBlock # lint-amnesty, pylint: disable=wrong-import-order -from xmodule.util.xmodule_django import add_webpack_to_fragment # lint-amnesty, pylint: disable=wrong-import-order from xmodule.vertical_block import VerticalBlock # lint-amnesty, pylint: disable=wrong-import-order from xmodule.x_module import ( # lint-amnesty, pylint: disable=wrong-import-order PREVIEW_VIEWS, @@ -116,6 +114,9 @@ def wrap_xblock( if view == STUDENT_VIEW and getattr(block, 'HIDDEN', False): css_classes.append('is-hidden') + # TODO: This special case will be removed when we update the SCSS under + # xmodule/assets to use the standard XBlock CSS classes. + # See https://github.com/openedx/edx-platform/issues/32617. if getattr(block, 'uses_xmodule_styles_setup', False): if view in PREVIEW_VIEWS: # The block is acting as an XModule @@ -444,16 +445,22 @@ def xblock_resource_pkg(block): openassessment.xblock.openassessmentblock.OpenAssessmentBlock, the value returned is 'openassessment.xblock'. - XModules are special cased because they're local to this repo and they - actually don't share their resource files when compiled out as part of the - XBlock asset pipeline. This only covers XBlocks and XModules using the - XBlock-style of asset specification. If they use the XModule bundling part - of the asset pipeline (xmodule_assets), their assets are compiled through an - entirely separate mechanism and put into lms-modules.js/css. + Built-in edx-platform XBlocks (defined under ./xmodule/) are special cases. + They currently use two different mechanisms to load assets: + 1. The `builtin_assets` utilities, which let the blocks add JS and CSS + compiled completely outside of the XBlock pipeline. Used by HtmlBlock, + ProblemBlock, and most other built-in blocks currently. Handling for these + assets does not interact with this function. + 2. The (preferred) standard XBlock runtime resource loading system, used by + LibrarySourdedBlock. Handling for these assets *does* interact with this + function. + + We hope to migrate to (2) eventually, tracked by: + https://github.com/openedx/edx-platform/issues/32618. """ - # XModules are a special case because they map to different dirs for - # sub-modules. module_name = block.__module__ + # Special handling for case (2) of the built-in XBlocks because they map to different + # dirs for sub-modules. if module_name.startswith('xmodule.'): return module_name diff --git a/pavelib/assets.py b/pavelib/assets.py index 953faf84dee2..3bf8a9ce88ae 100644 --- a/pavelib/assets.py +++ b/pavelib/assets.py @@ -170,8 +170,7 @@ 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_dir = path("xmodule") / "static" / "sass" / system - xmodule_lookup_dir = path("xmodule") / "static" / "sass" / "include" + builtin_xblock_sass = path("xmodule") / "assets" dependencies = SASS_LOOKUP_DEPENDENCIES.get(system, []) if sass_dir.isdir(): @@ -199,18 +198,6 @@ 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": [ - xmodule_lookup_dir, - *dependencies, - sass_dir / "partials", - system_sass_dir / "partials", - system_sass_dir, - ], - }) - # now compile theme sass files for certificate if system == 'lms': dirs.append({ @@ -222,6 +209,28 @@ def get_theme_sass_dirs(system, theme_dir): ], }) + # Now, finally, compile builtin XBlocks' Sass. Themes cannot override these + # Sass files directly, but they *can* modify Sass variables which will affect + # the output here. We compile all builtin XBlocks' Sass both for LMS and CMS, + # not because we expect the output to be different between LMS and CMS, but + # because only LMS/CMS-compiled Sass can be themed; common sass is not themed. + dirs.append({ + "sass_source_dir": builtin_xblock_sass, + "css_destination_dir": css_dir, + "lookup_paths": [ + # XBlock editor views may need both LMS and CMS partials. + # XBlock display views should only need LMS patials. + # In order to keep this build script simpler, though, we just + # include everything and compile everything at once. + theme_dir / "lms" / "static" / "sass" / "partials", + theme_dir / "cms" / "static" / "sass" / "partials", + path("lms") / "static" / "sass" / "partials", + path("cms") / "static" / "sass" / "partials", + path("lms") / "static" / "sass", + path("cms") / "static" / "sass", + ], + }) + return dirs @@ -237,8 +246,7 @@ def get_system_sass_dirs(system): dirs = [] sass_dir = path(system) / "static" / "sass" css_dir = path(system) / "static" / "css" - xmodule_sass_dir = path("xmodule") / "static" / "sass" / system - xmodule_lookup_dir = path("xmodule") / "static" / "sass" / "include" + builtin_xblock_sass = path("xmodule") / "assets" dependencies = SASS_LOOKUP_DEPENDENCIES.get(system, []) dirs.append({ @@ -250,17 +258,6 @@ def get_system_sass_dirs(system): ], }) - dirs.append({ - "sass_source_dir": xmodule_sass_dir, - "css_destination_dir": path("common") / "static" / "css" / "xmodule", - "lookup_paths": [ - xmodule_lookup_dir, - *dependencies, - sass_dir / "partials", - sass_dir, - ], - }) - if system == 'lms': dirs.append({ "sass_source_dir": path(system) / "static" / "certificates" / "sass", @@ -271,6 +268,18 @@ def get_system_sass_dirs(system): ], }) + # See builtin_xblock_sass compilation in get_theme_sass_dirs for details. + dirs.append({ + "sass_source_dir": builtin_xblock_sass, + "css_destination_dir": css_dir, + "lookup_paths": dependencies + [ + path("lms") / "static" / "sass" / "partials", + path("cms") / "static" / "sass" / "partials", + path("lms") / "static" / "sass", + path("cms") / "static" / "sass", + ], + }) + return dirs @@ -577,6 +586,10 @@ def _compile_sass(system, theme, debug, force, timing_info): else: sh(f"rm -rf {css_dir}/*.css") + all_lookup_paths = COMMON_LOOKUP_PATHS + lookup_paths + print(f"Compiling Sass: {sass_source_dir} -> {css_dir}") + for lookup_path in all_lookup_paths: + print(f" with Sass lookup path: {lookup_path}") if dry_run: tasks.environment.info("libsass {sass_dir}".format( sass_dir=sass_source_dir, @@ -584,7 +597,7 @@ def _compile_sass(system, theme, debug, force, timing_info): else: sass.compile( dirname=(sass_source_dir, css_dir), - include_paths=COMMON_LOOKUP_PATHS + lookup_paths, + include_paths=all_lookup_paths, source_comments=source_comments, output_style=output_style, ) diff --git a/xmodule/README.rst b/xmodule/README.rst index e4f9876cc36e..73aeca96cc7c 100644 --- a/xmodule/README.rst +++ b/xmodule/README.rst @@ -6,15 +6,18 @@ The ``xmodule`` folder contains a variety of old-yet-important functionality cor * the ModuleStore, edx-platform's "Version 1" learning content storage backend; * an XBlock Runtime implementation for ModuleStore-backed content; * the "partitions" framework for differentiated XBlock content; -* implementations for the "stuctural" XBlocks: ``course``, ``chapter``, and ``sequential``; and -* the implementations of several different content-level XBlocks, such as ``problem`` and ``html``. +* implementations for the "stuctural" XBlocks: ``course``, ``chapter``, and ``sequential``; +* the implementations of several different built-in content-level XBlocks, such as ``problem`` and ``html``; and +* `assets for those built-in XBlocks`_. + +.. _assets for those built-in XBlocks: https://github.com/openedx/edx-platform/tree/master/assets Historical Context ****************** "XModule" was the original content framework for edx-platform, which gives the folder its current name. XModules rendered specific course run content types to users for both authoring and learning. -For instance, there was an XModule for Videos, another for HTML snippets, and another for Sequences. +For instance, there was an XModule for Videos, another for HTML snippets, and another for Sequences. XModule was succeeded in ~2013 by the "XBlock" framework, which served the same purpose, but put additional focus into flexibility and modularity. XBlock allows new content types to be created by anyone, completely external the edx-platform repository. @@ -47,7 +50,7 @@ To help with this direction, please **do not add new functionality to this direc .. _Blockstore: https://github.com/openedx/blockstore/ .. _edx-platform XBlock runtime: https://github.com/openedx/edx-platform/tree/master/openedx/core/djangoapps/xblock -.. _openedx-learning: https://github.com/openedx/openedx-learning +.. _openedx-learning: https://github.com/openedx/openedx-learning .. _xblock-drag-and-drop-v2: https://github.com/openedx/xblock-drag-and-drop-v2 .. _the forums: https://discuss.openedx.org diff --git a/xmodule/annotatable_block.py b/xmodule/annotatable_block.py index e2c6a7a2c554..3c78a3b7c0e7 100644 --- a/xmodule/annotatable_block.py +++ b/xmodule/annotatable_block.py @@ -12,7 +12,7 @@ from openedx.core.djangolib.markup import HTML, Text from xmodule.editing_block import EditingMixin from xmodule.raw_block import RawMixin -from xmodule.util.xmodule_django import add_webpack_to_fragment +from xmodule.util.builtin_assets import add_webpack_js_to_fragment, add_sass_to_fragment from xmodule.xml_block import XmlMixin from xmodule.x_module import ( HTMLSnippet, @@ -199,7 +199,8 @@ def student_view(self, context): # lint-amnesty, pylint: disable=unused-argumen """ fragment = Fragment() fragment.add_content(self.get_html()) - add_webpack_to_fragment(fragment, 'AnnotatableBlockPreview') + add_sass_to_fragment(fragment, 'AnnotatableBlockDisplay.scss') + add_webpack_js_to_fragment(fragment, 'AnnotatableBlockDisplay') shim_xmodule_js(fragment, 'Annotatable') return fragment @@ -211,6 +212,7 @@ def studio_view(self, _context): fragment = Fragment( self.runtime.service(self, 'mako').render_template(self.mako_template, self.get_context()) ) - add_webpack_to_fragment(fragment, 'AnnotatableBlockStudio') + add_sass_to_fragment(fragment, 'AnnotatableBlockEditor.scss') + add_webpack_js_to_fragment(fragment, 'AnnotatableBlockEditor') shim_xmodule_js(fragment, self.studio_js_module_name) return fragment diff --git a/xmodule/static/sass/lms/AnnotatableBlockPreview.scss b/xmodule/assets/AnnotatableBlockDisplay.scss similarity index 100% rename from xmodule/static/sass/lms/AnnotatableBlockPreview.scss rename to xmodule/assets/AnnotatableBlockDisplay.scss diff --git a/xmodule/static/sass/cms/AnnotatableBlockStudio.scss b/xmodule/assets/AnnotatableBlockEditor.scss similarity index 100% rename from xmodule/static/sass/cms/AnnotatableBlockStudio.scss rename to xmodule/assets/AnnotatableBlockEditor.scss diff --git a/xmodule/static/sass/cms/CustomTagBlockStudio.scss b/xmodule/assets/CustomTagBlockEditor.scss similarity index 100% rename from xmodule/static/sass/cms/CustomTagBlockStudio.scss rename to xmodule/assets/CustomTagBlockEditor.scss diff --git a/xmodule/assets/HtmlBlockDisplay.scss b/xmodule/assets/HtmlBlockDisplay.scss new file mode 100644 index 000000000000..0bc49706cfff --- /dev/null +++ b/xmodule/assets/HtmlBlockDisplay.scss @@ -0,0 +1,6 @@ +.xmodule_display.xmodule_AboutBlock, +.xmodule_display.xmodule_CourseInfoBlock, +.xmodule_display.xmodule_HtmlBlock, +.xmodule_display.xmodule_StaticTabBlock { + @import "html/display.scss"; +} diff --git a/xmodule/assets/HtmlBlockEditor.scss b/xmodule/assets/HtmlBlockEditor.scss new file mode 100644 index 000000000000..15d60863e28e --- /dev/null +++ b/xmodule/assets/HtmlBlockEditor.scss @@ -0,0 +1,7 @@ +.xmodule_edit.xmodule_AboutBlock, +.xmodule_edit.xmodule_CourseInfoBlock, +.xmodule_edit.xmodule_HtmlBlock, +.xmodule_edit.xmodule_StaticTabBlock { + @import "editor/edit.scss"; + @import "html/edit.scss"; +} diff --git a/xmodule/static/sass/lms/LTIBlockPreview.scss b/xmodule/assets/LTIBlockDisplay.scss similarity index 100% rename from xmodule/static/sass/lms/LTIBlockPreview.scss rename to xmodule/assets/LTIBlockDisplay.scss diff --git a/xmodule/static/sass/lms/PollBlockPreview.scss b/xmodule/assets/PollBlockDisplay.scss similarity index 100% rename from xmodule/static/sass/lms/PollBlockPreview.scss rename to xmodule/assets/PollBlockDisplay.scss diff --git a/xmodule/static/sass/lms/ProblemBlockPreview.scss b/xmodule/assets/ProblemBlockDisplay.scss similarity index 100% rename from xmodule/static/sass/lms/ProblemBlockPreview.scss rename to xmodule/assets/ProblemBlockDisplay.scss diff --git a/xmodule/static/sass/cms/ProblemBlockStudio.scss b/xmodule/assets/ProblemBlockEditor.scss similarity index 100% rename from xmodule/static/sass/cms/ProblemBlockStudio.scss rename to xmodule/assets/ProblemBlockEditor.scss diff --git a/xmodule/assets/README.rst b/xmodule/assets/README.rst new file mode 100644 index 000000000000..c63fd5126d35 --- /dev/null +++ b/xmodule/assets/README.rst @@ -0,0 +1,99 @@ +xmodule/assets: edx-platform XBlock resources +############################################# + +This folder exists to contain resources (ie, static assets) for the XBlocks +defined in edx-platform. + +Concepts +******** + +We would like edx-platform XBlock resources to match the standard XBlock +resource strategy as much as possible, because: + +* it'll make it easier to extract the XBlocks into their own packages + eventually, and +* it makes it easier to reason about the system as a whole when + internally-defined and externally-defined blocks play by the same rules. + +Due to the legacy of the XModule system, we're not quite there yet. +However, we are proactively working towards a system where: + +* Python is not involved in the generation of static assets. +* We minimze conditionals that differentiate between "older" (aka "XModule-style") + XBlocks and newer (aka "pure") XBlocks. +* Each XBlock's assets are contained within their own folder as much as + possible. See ``./vertical`` as an example. + +Themable Sass (.scss) +********************* + +XBlock CSS for ``student_view``, ``author_view``, and ``public_view`` is compiled from the various ``./BlockDisplay.scss`` modules, such as `AnnotatableBlockDisplay.scss`_. + +XBlock CSS for ``studio_view`` is compiled from the various ``./BlockEditor.scss`` modules, such as `AnnotatableBlockEditor.scss`_. + +Those Sass modules are mostly thin wrappers around the underscore-prefixed Sass +modules within block-type-subdirectories, such as `annotatable/_display.css`. In the +future, we may `simplify things`_ by collapsing the top-level Sass modules and +just directly compiling the block-type-subdirectories' Sass. + +The CSS is compiled into the static folders of both LMS and CMS, as well as into +the corresponding folders in any enabled themes, as part of the edx-platform build. +It is collected into the static root, and then linked to from XBlock fragments by the +``add_sass_to_fragment`` function in `builtin_assets.py`_. + +.. _AnnotatableBlockDisplay: https://github.com/openedx/edx-platform/tree/master/xmodule/assets/AnnotatableBlockDisplay.scss +.. _AnnotatableBlockEditor: https://github.com/openedx/edx-platform/tree/master/xmodule/assets/AnnotatableBlockEditor.scss +.. _annotatable/_display.scss: https://github.com/openedx/edx-platform/tree/master/xmodule/assets/annotatable/_display.scss +.. _simplify things: https://github.com/openedx/edx-platform/issues/32621 + +Static CSS (.css) +***************** + +Non-themable, ready-to-seve CSS may also be added to the any block type's +subdirectory. For example, see `library_source_block/style.css`_. + +JavaScript (.js) +**************** + +Currently, edx-platform XBlock JS is defined both here in `xmodule/assets`_ and outside in `xmodule/js`_. Different JS resources are processed differently: + +* For many older blocks, their JS is: + + * copied to ``common/static/xmodule`` by `static_content.py`_ (aka ``xmodule_assets``), + * then bundled using a generated Webpack config at ``common/static/xmodule/webpack.xmodule.config.js``, + * which is included into `webpack.common.config.js`_, + * allowing it to be included into XBlock fragments using ``add_webpack_js_to_fragment`` from `builtin_assets.py`_. + + Example blocks using this setup: + + * `ProblemBlock`_ + * `HtmlBlock`_ + * `AnnotatableBlock`_ + +* For other "purer" blocks, the JS is used as a standard XBlock fragment. Example blocks: + + * `VerticalBlock`_ + * `LibrarySourcedBlock`_ + +* Some XBlock JS is also processed through Django Pipeline and used in a couple specific legacy places. + +As part of an `active build refactoring`_: + +* We update the older builtin XBlocks to reference their JS directly rather than using copies of it. +* We will move ``webpack.xmodule.config.js`` here instead of generating it. +* We will consolidate all edx-platform XBlock JS here in `xmodule/assets`_. +* We will remove XBlock JS from Django Pipeline. +* We will delete the ``xmodule_assets`` script. + +.. _xmodule/assets: https://github.com/openedx/edx-platform/tree/master/xmodule/assets +.. _xmodule/js: https://github.com/openedx/edx-platform/tree/master/xmodule/js +.. _ProblemBlock: https://github.com/openedx/edx-platform/blob/master/xmodule/capa_block.py +.. _HtmlBlock: https://github.com/openedx/edx-platform/blob/master/xmodule/html_block.py +.. _AnnotatableBlock: https://github.com/openedx/edx-platform/blob/master/xmodule/annotatable_block.py +.. _VerticalBlock: https://github.com/openedx/edx-platform/blob/master/xmodule/vertical_block.py +.. _LibrarySourcedBlock: https://github.com/openedx/edx-platform/blob/master/xmodule/library_sourced_block.py +.. _active build refactoring: https://github.com/openedx/edx-platform/issues/31624 +.. _builtin_assets.py: https://github.com/openedx/edx-platform/tree/master/xmodule/util/builtin_assets.py +.. _static_content.py: https://github.com/openedx/edx-platform/blob/master/xmodule/static_content.py +.. _library_source_block/style.css: https://github.com/openedx/edx-platform/blob/master/xmodule/assets/library_source_block/style.css +.. _webpack.common.config.js: https://github.com/openedx/edx-platform/blob/master/webpack.common.config.js diff --git a/xmodule/static/sass/lms/SequenceBlockPreview.scss b/xmodule/assets/SequenceBlockDisplay.scss similarity index 100% rename from xmodule/static/sass/lms/SequenceBlockPreview.scss rename to xmodule/assets/SequenceBlockDisplay.scss diff --git a/xmodule/static/sass/lms/VideoBlockPreview.scss b/xmodule/assets/VideoBlockDisplay.scss similarity index 100% rename from xmodule/static/sass/lms/VideoBlockPreview.scss rename to xmodule/assets/VideoBlockDisplay.scss diff --git a/xmodule/static/sass/cms/VideoBlockStudio.scss b/xmodule/assets/VideoBlockEditor.scss similarity index 100% rename from xmodule/static/sass/cms/VideoBlockStudio.scss rename to xmodule/assets/VideoBlockEditor.scss diff --git a/xmodule/static/sass/lms/WordCloudBlockPreview.scss b/xmodule/assets/WordCloudBlockDisplay.scss similarity index 100% rename from xmodule/static/sass/lms/WordCloudBlockPreview.scss rename to xmodule/assets/WordCloudBlockDisplay.scss diff --git a/xmodule/static/sass/include/annotatable/display.scss b/xmodule/assets/annotatable/_display.scss similarity index 100% rename from xmodule/static/sass/include/annotatable/display.scss rename to xmodule/assets/annotatable/_display.scss diff --git a/xmodule/static/sass/include/capa/display.scss b/xmodule/assets/capa/_display.scss similarity index 100% rename from xmodule/static/sass/include/capa/display.scss rename to xmodule/assets/capa/_display.scss diff --git a/xmodule/static/sass/include/codemirror/codemirror.scss b/xmodule/assets/codemirror/_codemirror.scss similarity index 100% rename from xmodule/static/sass/include/codemirror/codemirror.scss rename to xmodule/assets/codemirror/_codemirror.scss diff --git a/xmodule/static/sass/include/editor/edit.scss b/xmodule/assets/editor/_edit.scss similarity index 100% rename from xmodule/static/sass/include/editor/edit.scss rename to xmodule/assets/editor/_edit.scss diff --git a/xmodule/static/sass/include/html/display.scss b/xmodule/assets/html/_display.scss similarity index 100% rename from xmodule/static/sass/include/html/display.scss rename to xmodule/assets/html/_display.scss diff --git a/xmodule/static/sass/include/html/edit.scss b/xmodule/assets/html/_edit.scss similarity index 100% rename from xmodule/static/sass/include/html/edit.scss rename to xmodule/assets/html/_edit.scss diff --git a/xmodule/static/sass/include/lti/lti.scss b/xmodule/assets/lti/_lti.scss similarity index 100% rename from xmodule/static/sass/include/lti/lti.scss rename to xmodule/assets/lti/_lti.scss diff --git a/xmodule/static/sass/include/poll/display.scss b/xmodule/assets/poll/_display.scss similarity index 100% rename from xmodule/static/sass/include/poll/display.scss rename to xmodule/assets/poll/_display.scss diff --git a/xmodule/static/sass/include/problem/edit.scss b/xmodule/assets/problem/_edit.scss similarity index 100% rename from xmodule/static/sass/include/problem/edit.scss rename to xmodule/assets/problem/_edit.scss diff --git a/xmodule/static/sass/include/sequence/display.scss b/xmodule/assets/sequence/_display.scss similarity index 100% rename from xmodule/static/sass/include/sequence/display.scss rename to xmodule/assets/sequence/_display.scss diff --git a/xmodule/static/sass/include/tabs/codemirror.scss b/xmodule/assets/tabs/_codemirror.scss similarity index 100% rename from xmodule/static/sass/include/tabs/codemirror.scss rename to xmodule/assets/tabs/_codemirror.scss diff --git a/xmodule/static/sass/include/tabs/tabs.scss b/xmodule/assets/tabs/_tabs.scss similarity index 100% rename from xmodule/static/sass/include/tabs/tabs.scss rename to xmodule/assets/tabs/_tabs.scss diff --git a/xmodule/static/sass/include/video/accessible_menu.scss b/xmodule/assets/video/_accessible_menu.scss similarity index 100% rename from xmodule/static/sass/include/video/accessible_menu.scss rename to xmodule/assets/video/_accessible_menu.scss diff --git a/xmodule/static/sass/include/video/display.scss b/xmodule/assets/video/_display.scss similarity index 100% rename from xmodule/static/sass/include/video/display.scss rename to xmodule/assets/video/_display.scss diff --git a/xmodule/static/sass/include/word_cloud/display.scss b/xmodule/assets/word_cloud/_display.scss similarity index 100% rename from xmodule/static/sass/include/word_cloud/display.scss rename to xmodule/assets/word_cloud/_display.scss diff --git a/xmodule/capa_block.py b/xmodule/capa_block.py index 910007712770..43b5bb3efa42 100644 --- a/xmodule/capa_block.py +++ b/xmodule/capa_block.py @@ -37,7 +37,7 @@ from xmodule.graders import ShowCorrectness from xmodule.raw_block import RawMixin from xmodule.util.sandboxing import SandboxService -from xmodule.util.xmodule_django import add_webpack_to_fragment +from xmodule.util.builtin_assets import add_webpack_js_to_fragment, add_sass_to_fragment from xmodule.x_module import ( HTMLSnippet, ResourceTemplates, @@ -347,7 +347,8 @@ def student_view(self, _context, show_detailed_errors=False): else: html = self.get_html() fragment = Fragment(html) - add_webpack_to_fragment(fragment, 'ProblemBlockPreview') + add_sass_to_fragment(fragment, "ProblemBlockDisplay.scss") + add_webpack_js_to_fragment(fragment, 'ProblemBlockDisplay') shim_xmodule_js(fragment, 'Problem') return fragment @@ -378,7 +379,8 @@ def studio_view(self, _context): fragment = Fragment( self.runtime.service(self, 'mako').render_template(self.mako_template, self.get_context()) ) - add_webpack_to_fragment(fragment, 'ProblemBlockStudio') + add_sass_to_fragment(fragment, 'ProblemBlockEditor.scss') + add_webpack_js_to_fragment(fragment, 'ProblemBlockEditor') shim_xmodule_js(fragment, 'MarkdownEditingDescriptor') return fragment diff --git a/xmodule/conditional_block.py b/xmodule/conditional_block.py index 23ffbae43afa..5f484050d438 100644 --- a/xmodule/conditional_block.py +++ b/xmodule/conditional_block.py @@ -19,7 +19,7 @@ from xmodule.modulestore.exceptions import ItemNotFoundError from xmodule.seq_block import SequenceMixin from xmodule.studio_editable import StudioEditableBlock -from xmodule.util.xmodule_django import add_webpack_to_fragment +from xmodule.util.builtin_assets import add_webpack_js_to_fragment from xmodule.validation import StudioValidation, StudioValidationMessage from xmodule.xml_block import XmlMixin from xmodule.x_module import ( @@ -233,7 +233,7 @@ def student_view(self, _context): """ fragment = Fragment() fragment.add_content(self.get_html()) - add_webpack_to_fragment(fragment, 'ConditionalBlockPreview') + add_webpack_js_to_fragment(fragment, 'ConditionalBlockDisplay') shim_xmodule_js(fragment, 'Conditional') return fragment @@ -267,7 +267,7 @@ def studio_view(self, _context): fragment = Fragment( self.runtime.service(self, 'mako').render_template(self.mako_template, self.get_context()) ) - add_webpack_to_fragment(fragment, 'ConditionalBlockStudio') + add_webpack_js_to_fragment(fragment, 'ConditionalBlockEditor') shim_xmodule_js(fragment, self.studio_js_module_name) return fragment diff --git a/xmodule/html_block.py b/xmodule/html_block.py index c9f3ddbf4d69..3027c24b2ba8 100644 --- a/xmodule/html_block.py +++ b/xmodule/html_block.py @@ -25,7 +25,7 @@ from xmodule.html_checker import check_html from xmodule.stringify import stringify_children from xmodule.util.misc import escape_html_characters -from xmodule.util.xmodule_django import add_webpack_to_fragment +from xmodule.util.builtin_assets import add_webpack_js_to_fragment, add_sass_to_fragment from xmodule.x_module import ( HTMLSnippet, ResourceTemplates, @@ -93,7 +93,8 @@ def student_view(self, _context): Return a fragment that contains the html for the student view """ fragment = Fragment(self.get_html()) - add_webpack_to_fragment(fragment, 'HtmlBlockPreview') + add_sass_to_fragment(fragment, 'HtmlBlockDisplay.scss') + add_webpack_js_to_fragment(fragment, 'HtmlBlockDisplay') shim_xmodule_js(fragment, 'HTMLModule') return fragment @@ -138,7 +139,8 @@ def studio_view(self, _context): fragment = Fragment( self.runtime.service(self, 'mako').render_template(self.mako_template, self.get_context()) ) - add_webpack_to_fragment(fragment, 'HtmlBlockStudio') + add_sass_to_fragment(fragment, 'HtmlBlockEditor.scss') + add_webpack_js_to_fragment(fragment, 'HtmlBlockEditor') shim_xmodule_js(fragment, 'HTMLEditingDescriptor') return fragment diff --git a/xmodule/library_content_block.py b/xmodule/library_content_block.py index 2e405371d017..74ab77630626 100644 --- a/xmodule/library_content_block.py +++ b/xmodule/library_content_block.py @@ -27,7 +27,7 @@ from xmodule.capa.responsetypes import registry from xmodule.mako_block import MakoTemplateBlockBase from xmodule.studio_editable import StudioEditableBlock -from xmodule.util.xmodule_django import add_webpack_to_fragment +from xmodule.util.builtin_assets import add_webpack_js_to_fragment from xmodule.validation import StudioValidation, StudioValidationMessage from xmodule.xml_block import XmlMixin from xmodule.x_module import ( @@ -458,7 +458,7 @@ def studio_view(self, _context): fragment = Fragment( self.runtime.service(self, 'mako').render_template(self.mako_template, self.get_context()) ) - add_webpack_to_fragment(fragment, 'LibraryContentBlockStudio') + add_webpack_js_to_fragment(fragment, 'LibraryContentBlockEditor') shim_xmodule_js(fragment, self.studio_js_module_name) return fragment diff --git a/xmodule/lti_block.py b/xmodule/lti_block.py index 66bb4e2049c7..80afed7a0887 100644 --- a/xmodule/lti_block.py +++ b/xmodule/lti_block.py @@ -85,7 +85,7 @@ ) from xmodule.lti_2_util import LTI20BlockMixin, LTIError from xmodule.raw_block import EmptyDataRawMixin -from xmodule.util.xmodule_django import add_webpack_to_fragment +from xmodule.util.builtin_assets import add_webpack_js_to_fragment, add_sass_to_fragment from xmodule.xml_block import XmlMixin from xmodule.x_module import ( HTMLSnippet, @@ -399,7 +399,8 @@ def studio_view(self, _context): fragment = Fragment( self.runtime.service(self, 'mako').render_template(self.mako_template, context) ) - add_webpack_to_fragment(fragment, 'LTIBlockStudio') + add_sass_to_fragment(fragment, 'LTIBlockEditor.scss') + add_webpack_js_to_fragment(fragment, 'LTIBlockEditor') shim_xmodule_js(fragment, self.studio_js_module_name) return fragment @@ -514,7 +515,7 @@ def student_view(self, _context): """ fragment = Fragment() fragment.add_content(self.runtime.service(self, 'mako').render_template('lti.html', self.get_context())) - add_webpack_to_fragment(fragment, 'LTIBlockPreview') + add_webpack_js_to_fragment(fragment, 'LTIBlockDisplay') shim_xmodule_js(fragment, 'LTI') return fragment diff --git a/xmodule/poll_block.py b/xmodule/poll_block.py index 3141c7b71984..6093237fa3c5 100644 --- a/xmodule/poll_block.py +++ b/xmodule/poll_block.py @@ -22,7 +22,7 @@ from openedx.core.djangolib.markup import Text, HTML from xmodule.mako_block import MakoTemplateBlockBase from xmodule.stringify import stringify_children -from xmodule.util.xmodule_django import add_webpack_to_fragment +from xmodule.util.builtin_assets import add_webpack_js_to_fragment, add_sass_to_fragment from xmodule.x_module import ( HTMLSnippet, ResourceTemplates, @@ -155,7 +155,8 @@ def student_view(self, _context): 'configuration_json': self.dump_poll(), } fragment.add_content(self.runtime.service(self, 'mako').render_template('poll.html', params)) - add_webpack_to_fragment(fragment, 'PollBlockPreview') + add_sass_to_fragment(fragment, 'PollBlockDisplay.scss') + add_webpack_js_to_fragment(fragment, 'PollBlockDisplay') shim_xmodule_js(fragment, 'Poll') return fragment diff --git a/xmodule/seq_block.py b/xmodule/seq_block.py index c46bb97e0fee..b0a2789fd273 100644 --- a/xmodule/seq_block.py +++ b/xmodule/seq_block.py @@ -23,7 +23,7 @@ from xblock.fields import Boolean, Integer, List, Scope, String from edx_toggles.toggles import WaffleFlag, SettingDictToggle -from xmodule.util.xmodule_django import add_webpack_to_fragment +from xmodule.util.builtin_assets import add_webpack_js_to_fragment, add_sass_to_fragment from xmodule.x_module import ( HTMLSnippet, ResourceTemplates, @@ -469,7 +469,8 @@ def student_view(self, context): banner_text, special_html = special_html_view if special_html and not masquerading_as_specific_student: fragment = Fragment(special_html) - add_webpack_to_fragment(fragment, 'SequenceBlockPreview') + add_sass_to_fragment(fragment, 'SequenceBlockDisplay.scss') + add_webpack_js_to_fragment(fragment, 'SequenceBlockDisplay') shim_xmodule_js(fragment, 'Sequence') return fragment @@ -610,7 +611,8 @@ def _student_or_public_view(self, context, prereq_met, prereq_meta_info, banner_ self._capture_full_seq_item_metrics(children) self._capture_current_unit_metrics(children) - add_webpack_to_fragment(fragment, 'SequenceBlockPreview') + add_sass_to_fragment(fragment, 'SequenceBlockDisplay.scss') + add_webpack_js_to_fragment(fragment, 'SequenceBlockDisplay') shim_xmodule_js(fragment, 'Sequence') return fragment diff --git a/xmodule/split_test_block.py b/xmodule/split_test_block.py index 229578caef0c..5984ae531efc 100644 --- a/xmodule/split_test_block.py +++ b/xmodule/split_test_block.py @@ -22,7 +22,7 @@ from xmodule.progress import Progress from xmodule.seq_block import ProctoringFields, SequenceMixin from xmodule.studio_editable import StudioEditableBlock -from xmodule.util.xmodule_django import add_webpack_to_fragment +from xmodule.util.builtin_assets import add_webpack_js_to_fragment from xmodule.validation import StudioValidation, StudioValidationMessage from xmodule.xml_block import XmlMixin from xmodule.x_module import ( @@ -362,7 +362,7 @@ def studio_view(self, context): fragment = Fragment( self.runtime.service(self, 'mako').render_template(self.mako_template, self.get_context()) ) - add_webpack_to_fragment(fragment, 'SplitTestBlockStudio') + add_webpack_js_to_fragment(fragment, 'SplitTestBlockEditor') shim_xmodule_js(fragment, self.studio_js_module_name) return fragment diff --git a/xmodule/static/sass/cms/AboutBlockStudio.scss b/xmodule/static/sass/cms/AboutBlockStudio.scss deleted file mode 100644 index 0af42e47ff02..000000000000 --- a/xmodule/static/sass/cms/AboutBlockStudio.scss +++ /dev/null @@ -1,4 +0,0 @@ -.xmodule_edit.xmodule_AboutBlock { - @import "editor/edit.scss"; - @import "html/edit.scss"; -} diff --git a/xmodule/static/sass/cms/CourseInfoBlockStudio.scss b/xmodule/static/sass/cms/CourseInfoBlockStudio.scss deleted file mode 100644 index df1af8311546..000000000000 --- a/xmodule/static/sass/cms/CourseInfoBlockStudio.scss +++ /dev/null @@ -1,4 +0,0 @@ -.xmodule_edit.xmodule_CourseInfoBlock { - @import "editor/edit.scss"; - @import "html/edit.scss"; -} diff --git a/xmodule/static/sass/cms/HtmlBlockStudio.scss b/xmodule/static/sass/cms/HtmlBlockStudio.scss deleted file mode 100644 index 350a80cc5d84..000000000000 --- a/xmodule/static/sass/cms/HtmlBlockStudio.scss +++ /dev/null @@ -1,4 +0,0 @@ -.xmodule_edit.xmodule_HtmlBlock { - @import "editor/edit.scss"; - @import "html/edit.scss"; -} diff --git a/xmodule/static/sass/cms/StaticTabBlockStudio.scss b/xmodule/static/sass/cms/StaticTabBlockStudio.scss deleted file mode 100644 index 37fa9ca36030..000000000000 --- a/xmodule/static/sass/cms/StaticTabBlockStudio.scss +++ /dev/null @@ -1,4 +0,0 @@ -.xmodule_edit.xmodule_StaticTabBlock { - @import "editor/edit.scss"; - @import "html/edit.scss"; -} diff --git a/xmodule/static/sass/lms/AboutBlockPreview.scss b/xmodule/static/sass/lms/AboutBlockPreview.scss deleted file mode 100644 index 1df920bc232b..000000000000 --- a/xmodule/static/sass/lms/AboutBlockPreview.scss +++ /dev/null @@ -1,3 +0,0 @@ -.xmodule_display.xmodule_AboutBlock { - @import "html/display.scss"; -} diff --git a/xmodule/static/sass/lms/CourseInfoBlockPreview.scss b/xmodule/static/sass/lms/CourseInfoBlockPreview.scss deleted file mode 100644 index ff6353df74e2..000000000000 --- a/xmodule/static/sass/lms/CourseInfoBlockPreview.scss +++ /dev/null @@ -1,3 +0,0 @@ -.xmodule_display.xmodule_CourseInfoBlock { - @import "html/display.scss"; -} diff --git a/xmodule/static/sass/lms/HtmlBlockPreview.scss b/xmodule/static/sass/lms/HtmlBlockPreview.scss deleted file mode 100644 index 09610448fed5..000000000000 --- a/xmodule/static/sass/lms/HtmlBlockPreview.scss +++ /dev/null @@ -1,3 +0,0 @@ -.xmodule_display.xmodule_HtmlBlock { - @import "html/display.scss"; -} diff --git a/xmodule/static/sass/lms/StaticTabBlockPreview.scss b/xmodule/static/sass/lms/StaticTabBlockPreview.scss deleted file mode 100644 index 5c044e659b28..000000000000 --- a/xmodule/static/sass/lms/StaticTabBlockPreview.scss +++ /dev/null @@ -1,3 +0,0 @@ -.xmodule_display.xmodule_StaticTabBlock { - @import "html/display.scss"; -} diff --git a/xmodule/template_block.py b/xmodule/template_block.py index abf1f2c725a9..a27b92cb2c7e 100644 --- a/xmodule/template_block.py +++ b/xmodule/template_block.py @@ -10,7 +10,7 @@ from web_fragments.fragment import Fragment from xmodule.editing_block import EditingMixin from xmodule.raw_block import RawMixin -from xmodule.util.xmodule_django import add_webpack_to_fragment +from xmodule.util.builtin_assets import add_webpack_js_to_fragment, add_sass_to_fragment from xmodule.x_module import ( HTMLSnippet, ResourceTemplates, @@ -81,7 +81,8 @@ def studio_view(self, _context): fragment = Fragment( self.runtime.service(self, 'mako').render_template(self.mako_template, self.get_context()) ) - add_webpack_to_fragment(fragment, 'CustomTagBlockStudio') + add_sass_to_fragment(fragment, 'CustomTagBlockEditor.scss') + add_webpack_js_to_fragment(fragment, 'CustomTagBlockEditor') shim_xmodule_js(fragment, 'XMLEditingDescriptor') return fragment diff --git a/xmodule/tests/test_util_builtin_assets.py b/xmodule/tests/test_util_builtin_assets.py new file mode 100644 index 000000000000..bc26bed3d6be --- /dev/null +++ b/xmodule/tests/test_util_builtin_assets.py @@ -0,0 +1,68 @@ +""" +Tests for methods defined in builtin_assets.py +""" +from unittest import TestCase +from unittest.mock import patch + +from django.core.exceptions import ImproperlyConfigured +from web_fragments.fragment import Fragment, FragmentResource + +from xmodule.util import builtin_assets + + +class AddSassToFragmentTests(TestCase): + """ + Tests for add_sass_to_fragment. + + We would have liked to also test two additional cases: + * When a theme is enabled, and add_sass_to_fragment is called with a + theme-overriden Sass file, then a URL to themed CSS is added. + * When a theme is enabled, but add_sass_to_fragment is called with a Sass + file that the theme doesn't override, then a URL to the original (unthemed) + CSS is added. + Unfortunately, under edx-platform tests, settings.STATICFILES_STORAGE does not + include the ThemeStorage class, so themed URL generation doesn't work. + """ + + def test_absolute_path_raises_value_error(self): + fragment = Fragment() + with self.assertRaises(ValueError): + builtin_assets.add_sass_to_fragment( + fragment, + "/openedx/edx-platform/xmodule/assets/ProblemBlockDisplay.scss", + ) + + def test_not_scss_raises_value_error(self): + fragment = Fragment() + with self.assertRaises(ValueError): + builtin_assets.add_sass_to_fragment( + fragment, + "vertical/public/js/vertical_student_view.js" + ) + + def test_misspelled_path_raises_not_found(self): + fragment = Fragment() + with self.assertRaises(FileNotFoundError): + builtin_assets.add_sass_to_fragment( + fragment, + "ProblemBlockDisplayyyy.scss", + ) + + def test_static_file_missing_raises_improperly_configured(self): + fragment = Fragment() + with patch.object(builtin_assets, 'get_static_file_url', lambda _path: None): + with self.assertRaises(ImproperlyConfigured): + builtin_assets.add_sass_to_fragment( + fragment, + "ProblemBlockDisplay.scss", + ) + + def test_happy_path(self): + fragment = Fragment() + builtin_assets.add_sass_to_fragment(fragment, "ProblemBlockDisplay.scss") + assert fragment.resources[0] == FragmentResource( + kind='url', + data='/static/css/ProblemBlockDisplay.css', + mimetype='text/css', + placement='head', + ) diff --git a/xmodule/util/builtin_assets.py b/xmodule/util/builtin_assets.py new file mode 100644 index 000000000000..aaedc44cc649 --- /dev/null +++ b/xmodule/util/builtin_assets.py @@ -0,0 +1,54 @@ +""" +Utilities for adding edx-platform assets to built-in XBlocks. + +These should not be used to support any XBlocks outside of edx-platform. +""" +from pathlib import Path + +import webpack_loader +from django.conf import settings +from django.core.exceptions import ImproperlyConfigured + +from openedx.core.djangoapps.theming.helpers_static import get_static_file_url + + +def add_sass_to_fragment(fragment, sass_relative_path): + """ + Given a Sass path relative to xmodule/assets, add a compiled CSS URL to the fragment. + + Raises: + * ValueError if {sass_relative_path} is absolute or does not end in '.scss'. + * FileNotFoundError if edx-platform/xmodule/assets/{sass_relative_path} is missing. + * ImproperlyConfigured if the lookup of the static CSS URL fails. This could happen + if Sass wasn't compiled, CSS wasn't collected, or the staticfiles app is misconfigured. + + Notes: + * This function is theme-aware. That is: If a theme is enabled which provides a compiled + CSS file of the same name, then that CSS file will be used instead. + """ + if not isinstance(sass_relative_path, Path): + sass_relative_path = Path(sass_relative_path) + if sass_relative_path.is_absolute(): + raise ValueError(f"sass_relative_path should be relative; is absolute: {sass_relative_path}") + if sass_relative_path.suffix != '.scss': + raise ValueError(f"sass_relative_path should be .scss file; is: {sass_relative_path}") + sass_absolute_path = Path(settings.REPO_ROOT) / "xmodule" / "assets" / sass_relative_path + if not sass_absolute_path.is_file(): + raise FileNotFoundError(f"Sass not found: {sass_absolute_path}") + css_static_path = Path('css') / sass_relative_path.with_suffix('.css') + css_url = get_static_file_url(str(css_static_path)) # get_static_file_url is theme-aware. + if not css_url: + raise ImproperlyConfigured( + f"Did not find CSS file {css_static_path} (compiled from {sass_absolute_path}) " + f"in staticfiles storage. Perhaps it wasn't collected?" + ) + fragment.add_css_url(css_url) + + +def add_webpack_js_to_fragment(fragment, bundle_name): + """ + Add all JS webpack chunks to the supplied fragment. + """ + for chunk in webpack_loader.utils.get_files(bundle_name, None, 'DEFAULT'): + if chunk['name'].endswith(('.js', '.js.gz')): + fragment.add_javascript_url(chunk['url']) diff --git a/xmodule/util/xmodule_django.py b/xmodule/util/xmodule_django.py index 1d9a858ea495..2ce721ace114 100644 --- a/xmodule/util/xmodule_django.py +++ b/xmodule/util/xmodule_django.py @@ -1,93 +1,8 @@ """ Exposes Django utilities for consumption in the xmodule library -NOTE: This file should only be imported into 'django-safe' code, i.e. known that this code runs int the Django -runtime environment with the djangoapps in common configured to load """ - - -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.css', - 'path': 'common/static/css/xmodule/AnnotatableBlockPreview.css', - 'publicPath': '/static/css/xmodule/AnnotatableBlockPreview.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(): @@ -100,14 +15,3 @@ def get_current_request_hostname(): hostname = request.META.get('HTTP_HOST') return hostname - - -def add_webpack_to_fragment(fragment, bundle_name, extension=None, config='DEFAULT'): - """ - Add all webpack chunks to the supplied fragment as the appropriate resource type. - """ - for chunk in webpack_loader.utils.get_files(bundle_name, extension, config): - if chunk['name'].endswith(('.js', '.js.gz')): - fragment.add_javascript_url(chunk['url']) - elif chunk['name'].endswith(('.css', '.css.gz')): - fragment.add_css_url(chunk['url']) diff --git a/xmodule/vertical_block.py b/xmodule/vertical_block.py index f66e90203679..11904cfdb3da 100644 --- a/xmodule/vertical_block.py +++ b/xmodule/vertical_block.py @@ -19,7 +19,7 @@ from xmodule.seq_block import SequenceFields from xmodule.studio_editable import StudioEditableBlock from xmodule.util.misc import is_xblock_an_assignment -from xmodule.util.xmodule_django import add_webpack_to_fragment +from xmodule.util.builtin_assets import add_webpack_js_to_fragment from xmodule.x_module import PUBLIC_VIEW, STUDENT_VIEW, XModuleFields from xmodule.xml_block import XmlMixin @@ -163,7 +163,7 @@ def _student_or_public_view(self, context, view): # lint-amnesty, pylint: disab fragment.add_content(self.runtime.service(self, 'mako').render_template('vert_module.html', fragment_context)) - add_webpack_to_fragment(fragment, 'VerticalStudentView') + add_webpack_js_to_fragment(fragment, 'VerticalStudentView') fragment.initialize_js('VerticalStudentView') try: diff --git a/xmodule/video_block/video_block.py b/xmodule/video_block/video_block.py index aa83b5401c0f..dc5fb87c3c2e 100644 --- a/xmodule/video_block/video_block.py +++ b/xmodule/video_block/video_block.py @@ -48,7 +48,7 @@ from xmodule.modulestore.inheritance import InheritanceKeyValueStore, own_metadata from xmodule.raw_block import EmptyDataRawMixin from xmodule.validation import StudioValidation, StudioValidationMessage -from xmodule.util.xmodule_django import add_webpack_to_fragment +from xmodule.util.builtin_assets import add_webpack_js_to_fragment, add_sass_to_fragment from xmodule.video_block import manage_video_subtitles_save from xmodule.x_module import ( PUBLIC_VIEW, STUDENT_VIEW, @@ -242,7 +242,8 @@ def student_view(self, _context): Return the student view. """ fragment = Fragment(self.get_html()) - add_webpack_to_fragment(fragment, 'VideoBlockPreview') + add_sass_to_fragment(fragment, 'VideoBlockDisplay.scss') + add_webpack_js_to_fragment(fragment, 'VideoBlockDisplay') shim_xmodule_js(fragment, 'Video') return fragment @@ -259,7 +260,8 @@ def studio_view(self, _context): fragment = Fragment( self.runtime.service(self, 'mako').render_template(self.mako_template, self.get_context()) ) - add_webpack_to_fragment(fragment, 'VideoBlockStudio') + add_sass_to_fragment(fragment, 'VideoBlockEditor.scss') + add_webpack_js_to_fragment(fragment, 'VideoBlockEditor') shim_xmodule_js(fragment, 'TabsEditingDescriptor') return fragment @@ -274,7 +276,7 @@ def public_view(self, context): return self.student_view(context) fragment = Fragment(self.get_html(view=PUBLIC_VIEW, context=context)) - add_webpack_to_fragment(fragment, 'VideoBlockPreview') + add_webpack_js_to_fragment(fragment, 'VideoBlockPreview') shim_xmodule_js(fragment, 'Video') return fragment diff --git a/xmodule/word_cloud_block.py b/xmodule/word_cloud_block.py index 8fe741b881db..8be08242dbae 100644 --- a/xmodule/word_cloud_block.py +++ b/xmodule/word_cloud_block.py @@ -17,7 +17,7 @@ from xblock.fields import Boolean, Dict, Integer, List, Scope, String from xmodule.editing_block import EditingMixin from xmodule.raw_block import EmptyDataRawMixin -from xmodule.util.xmodule_django import add_webpack_to_fragment +from xmodule.util.builtin_assets import add_webpack_js_to_fragment, add_sass_to_fragment from xmodule.xml_block import XmlMixin from xmodule.x_module import ( HTMLSnippet, @@ -279,7 +279,8 @@ def student_view(self, context): # lint-amnesty, pylint: disable=unused-argumen 'num_inputs': self.num_inputs, 'submitted': self.submitted, })) - add_webpack_to_fragment(fragment, 'WordCloudBlockPreview') + add_sass_to_fragment(fragment, 'WordCloudBlockDisplay.scss') + add_webpack_js_to_fragment(fragment, 'WordCloudBlockDisplay') shim_xmodule_js(fragment, 'WordCloud') return fragment @@ -297,7 +298,7 @@ def studio_view(self, _context): fragment = Fragment( self.runtime.service(self, 'mako').render_template(self.mako_template, self.get_context()) ) - add_webpack_to_fragment(fragment, 'WordCloudBlockStudio') + add_webpack_js_to_fragment(fragment, 'WordCloudBlockEditor') shim_xmodule_js(fragment, self.studio_js_module_name) return fragment diff --git a/xmodule/x_module.py b/xmodule/x_module.py index 8694948dd040..1c44b2cc0b8d 100644 --- a/xmodule/x_module.py +++ b/xmodule/x_module.py @@ -36,7 +36,7 @@ from xmodule import block_metadata_utils from xmodule.fields import RelativeTime from xmodule.modulestore.exceptions import ItemNotFoundError -from xmodule.util.xmodule_django import add_webpack_to_fragment +from xmodule.util.builtin_assets import add_webpack_js_to_fragment from openedx.core.djangolib.markup import HTML from common.djangoapps.xblock_django.constants import ( @@ -221,7 +221,7 @@ def get_preview_view_js(cls): @classmethod def get_preview_view_js_bundle_name(cls): - return cls.__name__ + 'Preview' + return cls.__name__ + 'Display' @classmethod def get_studio_view_js(cls): @@ -229,7 +229,7 @@ def get_studio_view_js(cls): @classmethod def get_studio_view_js_bundle_name(cls): - return cls.__name__ + 'Studio' + return cls.__name__ + 'Editor' def get_html(self): """ @@ -252,7 +252,7 @@ def shim_xmodule_js(fragment, js_module_name): fragment.initialize_js('XBlockToXModuleShim') fragment.json_init_args = {'xmodule-type': js_module_name} - add_webpack_to_fragment(fragment, 'XModuleShim') + add_webpack_js_to_fragment(fragment, 'XModuleShim') class XModuleFields: