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.

Sign up for GitHub

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

build: Remove unneccessary built-in XBlock Sass built steps #35833

Merged
merged 1 commit into from
Nov 14, 2024
Merged

build: Remove unneccessary built-in XBlock Sass built steps #35833

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 0 additions & 52 deletions scripts/compile_sass.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -352,18 +352,6 @@ def compile_sass_dir(
repo / "lms" / "static" / "sass",
],
)
compile_sass_dir(
"Compiling built-in XBlock Sass for default LMS",
repo / "xmodule" / "assets",
repo / "lms" / "static" / "css",
includes=[
*common_includes,
repo / "lms" / "static" / "sass" / "partials",
repo / "cms" / "static" / "sass" / "partials",
repo / "lms" / "static" / "sass",
repo / "cms" / "static" / "sass",
],
)
if not skip_cms:
compile_sass_dir(
"Compiling default CMS Sass",
Expand All @@ -376,18 +364,6 @@ def compile_sass_dir(
repo / "cms" / "static" / "sass",
],
)
compile_sass_dir(
"Compiling built-in XBlock Sass for default CMS",
repo / "xmodule" / "assets",
repo / "cms" / "static" / "css",
includes=[
*common_includes,
repo / "lms" / "static" / "sass" / "partials",
repo / "cms" / "static" / "sass" / "partials",
repo / "lms" / "static" / "sass",
repo / "cms" / "static" / "sass",
],
)
click.secho(f"Done compiling default Sass!", fg="cyan", bold=True)
click.echo()

Expand Down Expand Up @@ -429,20 +405,6 @@ def compile_sass_dir(
],
tolerate_missing=True,
)
compile_sass_dir(
"Compiling built-in XBlock Sass for themed LMS",
repo / "xmodule" / "assets",
theme / "lms" / "static" / "css",
includes=[
*common_includes,
theme / "lms" / "static" / "sass" / "partials",
theme / "cms" / "static" / "sass" / "partials",
repo / "lms" / "static" / "sass" / "partials",
repo / "cms" / "static" / "sass" / "partials",
repo / "lms" / "static" / "sass",
repo / "cms" / "static" / "sass",
],
)
if not skip_cms:
compile_sass_dir(
"Compiling default CMS Sass with themed partials",
Expand Down Expand Up @@ -470,20 +432,6 @@ def compile_sass_dir(
],
tolerate_missing=True,
)
compile_sass_dir(
"Compiling built-in XBlock Sass for themed CMS",
repo / "xmodule" / "assets",
theme / "cms" / "static" / "css",
includes=[
*common_includes,
theme / "lms" / "static" / "sass" / "partials",
theme / "cms" / "static" / "sass" / "partials",
repo / "lms" / "static" / "sass" / "partials",
repo / "cms" / "static" / "sass" / "partials",
repo / "lms" / "static" / "sass",
repo / "cms" / "static" / "sass",
],
)
click.secho(f"Done compiling Sass for theme at {theme}!", fg="cyan", bold=True)
click.echo()

Expand Down
28 changes: 9 additions & 19 deletions xmodule/assets/README.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,25 +27,15 @@ However, we are proactively working towards a system where:
Themable Sass (.scss)
*********************

XBlock CSS for ``student_view``, ``author_view``, and ``public_view`` is compiled from the various ``./<ClassName>BlockDisplay.scss`` modules, such as `AnnotatableBlockDisplay.scss`_.

XBlock CSS for ``studio_view`` is compiled from the various ``./<ClassName>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.scss: https://github.com/openedx/edx-platform/tree/master/xmodule/assets/AnnotatableBlockDisplay.scss
.. _AnnotatableBlockEditor.scss: 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

Formerly, built-in XBlock CSS for ``student_view``, ``author_view``, and
``public_view`` was compiled from the various
``./<ClassName>BlockDisplay.scss`` modules, and ``studio_view`` CSS was
compiled from the various ``./<ClassName>BlockEditor.scss`` modules.

As of November 2024, all that built-in XBlock Sass was been permanently
compiled into CSS, stored at ``../static/css-builtin-blocks/``.
The theme-overridable Sass variables are injected into CSS variables via
``../../common/static/sass/_builtin-block-variables.scss``.

JavaScript (.js)
****************
Expand Down
35 changes: 1 addition & 34 deletions xmodule/util/builtin_assets.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,47 +32,14 @@ def add_css_to_fragment(fragment, css_relative_path):
if not css_absolute_path.is_file():
raise FileNotFoundError(f"css file not found: {css_absolute_path}")
css_static_path = Path('css-builtin-blocks') / css_relative_path.with_suffix('.css')
css_url = get_static_file_url(str(css_static_path)) # get_static_file_url is theme-aware.
Copy link
Member Author

@kdmccormick kdmccormick Nov 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note for reviewers: I removed this comment because this step is not theme aware any more. It will always return the same CSS file regardless of theme. Theme customizations will instead be injected via the CSS variables defined in common/static/sass/_builtin-block-variables.

css_url = get_static_file_url(str(css_static_path))
if not css_url:
raise ImproperlyConfigured(
f"Did not find static CSS file {css_static_path} in staticfiles storage. Perhaps it wasn't collected?"
)
fragment.add_css_url(css_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.
Expand Down
Loading