Decouple XModule styles from LMS/Studio styles (attempt 3)

[andrey-canon]

Description

Addressing this issue #31624

This PR is the third attempt of this #32018 but this now include some fixes to make this compatible with custom comprehensive themes

Testing instructions

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

For theme testing (Using tutor)

1. Copy the theme folder inside /end/build/openedx/themes
2. Run tutor dev start [--mount=./some path if you want to set a edx-platform volume] and wait
3. Go inside the container docker exec -it tutor_nightly_dev-lms-1 /bin/bash
4. Run openedx-assets themes --theme-dirs /openedx/themes /openedx/themes/parent-folder-of -your-theme --themes name-of-your-theme

[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.

[andrey-canon]

@kdmccormick :)

[kdmccormick]

Thanks @andrey-canon . Let me make sure I understand this correctly.

from Slack:

as you can see edx.org-next doesn't import edx-bootstrap, other themes also override that file but they include that import, labxchange includes in its file @import 'edx-bootstrap/sass/edx/theme'; and mitx-theme includes @import 'lms/static/sass/partials/lms/theme/variables.scss'; I think at this point the solution should be to add the import in the lms/theme/variables file

So, the base lms/theme/variables imports edx-bootstrap, but edx.org-next's override of that file doesn't. That causes edx.org-next's build to fail because it is missing bootstrap variables such as $pink. That makes sense 👍🏻 A couple questions:

• Do you know why the build worked before?
• Are you recommending that 2U add the edx-bootstrap import to edx.org-next's lms/theme/variables`?

however I consider how those xblocks-styles are built currently, xmodule files are imported here, line 29 and 60, line 7 imports 'base/build'; and inside base/build lms/theme/variables is imported and then bootstrap/variables is imported, so the content of lms/theme/variables is overridden by bootstrap/variables . Conclusion the content of lms/theme/variables doesn't matter since that is overridden but my changes should ensure that bootstrap/variables overrides that, so I added that in this commit

I don't think I completely follow this, a couple more questions:

• Is this related to the edx.org-next issue above, or is it separate?
• Are you saying that bootstrap/variables completely overrides lms/theme/variables; in other words, importing lms/theme/variables is a no-op?

[andrey-canon]

@kdmccormick

• Do you know why the build worked before?

the edx.org-next build worked previous to my changes because the xmodule-styles were compiled inside the context of build-course, I will try to clarify this with the whole flow, but I will focus just in one file not the whole compilation

when someone runs the command openedx-assets themes or openedx-assets common the flow for lms-course.scss will be the next.

1. lms-course.scss will import everything, the third import is @import 'build-course' so that will try to compile build-course
2. build-course will import @import 'base/build'; in its first line, that is defined in the base/build file
3. base/build will import first lms/theme/variables and then will import bootstrap/variables. At this point the content of lms/theme/variables was overridden by bootstrap.
4. There are more steps but for this answer it's not necessary to explain

So these decouple changes move the xmodule styles to its own files, that means that the styles won't be compiled inside lms-course.scss instead of that we will have a new main file for each xblock in this list, so each xblock will run independently therefore we have to ensure that every file has the whole context

Short answer is: that worked because the xmodule styles where compiled after the step 3 base/build

• Are you recommending that 2U add the edx-bootstrap import to edx.org-next's lms/theme/variables`?

Nop, probably in another context but here I just want to preserve the current styles but in a different files, so if pink is defined by bootstrap/variables I want the same pink inside the independent xblock files

• Is this related to the edx.org-next issue above, or is it separate?

it's the same

• Are you saying that bootstrap/variables completely overrides lms/theme/variables; in other words, importing lms/theme/variables is a no-op?

bootstrap/variables completely overrides the content that is present in both, if there are a variable called dog inside lms/theme/variables that will be the same value since probably bootstrap/variables doesn't have that

[kdmccormick]

I think that makes sense @andrey-canon . Just to make sure I understand correctly, I've tried to summarize. Let me know if I am mistaken.

bootstrap/scss/variables need to be included when compiling xmodule styles. Running through each possible scenario:

• Base theme, before decoupling:

  • Bootstrap vars were included first: build-course -> base/build -> lms/theme/variables -> edx-bootstrap -> ... -> bootstrap/scss/variables.
  • Then, the xmodule styles were included: build-course -> xmodule-styles ✅

• edX theme, before decoupling:

  • The theme replaces lms/theme/variables, but bootstrap vars were still included first: build-course -> base/build -> bootstrap/scss/variables.
  • Then, xmodule styles were included: build-course -> xmodule-styles ✅

• Base theme, after decoupling:

  • The xmodule styles are now compiled directly, not through build-course.
  • However, the xmodules still include lms/theme/variables, so we get bootstrap vars through lms/theme/variables -> edx-bootstrap -> ... -> bootstrap/scss/variables. ✅

• edX theme, after decoupling:

  • The xmodule style sheets are now compiled directly, not through build-course.
  • The theme replaces lms/theme/variables, so bootstrap/scss/variables is never included.
  • So the build failed in one of the xmodule style sheets. ❌
  • You fix this by importing bootstrap/scss/variables in every xmodule style sheet. ✅

[andrey-canon]

I think that makes sense @andrey-canon . Just to make sure I understand correctly, I've tried to summarize. Let me know if I am mistaken.

bootstrap/scss/variables need to be included when compiling xmodule styles. Running through each possible scenario:

• Base theme, before decoupling:

  • Bootstrap vars were included first: build-course -> base/build -> lms/theme/variables -> edx-bootstrap -> ... -> bootstrap/scss/variables.
  • Then, the xmodule styles were included: build-course -> xmodule-styles white_check_mark

• edX theme, before decoupling:

  • The theme replaces lms/theme/variables, but bootstrap vars were still included first: build-course -> base/build -> bootstrap/scss/variables.
  • Then, xmodule styles were included: build-course -> xmodule-styles white_check_mark

• Base theme, after decoupling:

  • The xmodule styles are now compiled directly, not through build-course.
  • However, the xmodules still include lms/theme/variables, so we get bootstrap vars through lms/theme/variables -> edx-bootstrap -> ... -> bootstrap/scss/variables. white_check_mark

• edX theme, after decoupling:

  • The xmodule style sheets are now compiled directly, not through build-course.
  • The theme replaces lms/theme/variables, so bootstrap/scss/variables is never included.
  • So the build failed in one of the xmodule style sheets. x
  • You fix this by importing bootstrap/scss/variables in every xmodule style sheet. white_check_mark

@kdmccormick that's right

[kdmccormick]

Great! I'm testing it out manually now. Assuming all goes well, I'll merge tomorrow morning.

[kdmccormick]

Tested it with edx.org-next and labxchange themes, looks good 👍🏻

[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.

[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.

[edx-pipeline-bot]

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

[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.

[edx-pipeline-bot]

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

[muhammad-ammar]

@kdmccormick @andrey-canon Looks like devstack(lms+studio) is broken due to this change. Any idea how can this be fixed without spinning up a new devstack?

[kdmccormick]

@muhammad-ammar Others have ben able to fix this with:

make dev.remove-containers
make dev.pull.lms
make dev.start.lms

Let us know if that doesn't resolve the issue.