Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged

Conversation

kdmccormick
Copy link
Member

@kdmccormick kdmccormick commented Nov 13, 2024

Description

Since all built-in block Sass is gone, we remove two final pieces of code:

  • the steps in npm run compile-sass which compiled xmodule/assets, and
  • the now-unused add_sass_to_fragment function.

This should speed up the edx-platform assets build a little bit.
This commit also includes some minor dev doc updates.

Supporting info

#35300

Testing instructions

  • Run npm run build-dev. Ensure there are no errors logged.
  • Load a ProblemBlock in the LMS. Make sure to hard-refresh so you get the latest assets.

@kdmccormick kdmccormick requested a review from farhan November 13, 2024 17:00
@@ -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

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.

Since all built-in block Sass is gone, we remove two final pieces of code:

* the steps in `npm run compile-sass` which compiled `xmodule/assets`, and
* the now-unused `add_sass_to_fragment` function.

This should speed up the edx-platform assets build a little bit.
This commit also includes some minor dev doc updates.

Part of: openedx#35300
@kdmccormick kdmccormick force-pushed the kdmccormick/xmodule-sass-final-cleanup branch from 9f4cccb to b323c2a Compare November 14, 2024 13:44
@kdmccormick kdmccormick enabled auto-merge (squash) November 14, 2024 13:45
@kdmccormick kdmccormick merged commit 2769a00 into openedx:master Nov 14, 2024
49 of 50 checks passed
@kdmccormick kdmccormick deleted the kdmccormick/xmodule-sass-final-cleanup branch November 14, 2024 14:05
@edx-pipeline-bot
Copy link
Contributor

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

@edx-pipeline-bot
Copy link
Contributor

2U Release Notice: This PR has been deployed to the edX production environment.

1 similar comment
@edx-pipeline-bot
Copy link
Contributor

2U Release Notice: This PR has been deployed to the edX production environment.

@edx-pipeline-bot
Copy link
Contributor

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

@edx-pipeline-bot
Copy link
Contributor

2U Release Notice: This PR has been deployed to the edX production environment.

1 similar comment
@edx-pipeline-bot
Copy link
Contributor

2U Release Notice: This PR has been deployed to the edX production environment.

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

Successfully merging this pull request may close these issues.

3 participants