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

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

Merged
merged 4 commits into from
May 18, 2023

Conversation

andrey-canon
Copy link
Contributor

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 openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label May 15, 2023
@openedx-webhooks
Copy link

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.

This basically changes how the xmodule static files are generated and consumed in order to separate the Xblock styles from general style files.

Addressing the issue openedx#31624
@andrey-canon andrey-canon force-pushed the and/decople_xmodule_styles branch from 1c6ad4c to b9417ec Compare May 15, 2023 13:28
@andrey-canon
Copy link
Contributor Author

@kdmccormick :)

@kdmccormick
Copy link
Member

kdmccormick commented May 15, 2023

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
Copy link
Contributor Author

@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
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Member

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

Copy link
Member

@kdmccormick kdmccormick left a comment

Choose a reason for hiding this comment

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

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

@kdmccormick kdmccormick merged commit 516eff0 into openedx:master May 18, 2023
@openedx-webhooks
Copy link

@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
Copy link
Contributor

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

@edx-pipeline-bot
Copy link
Contributor

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

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

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

@edx-pipeline-bot
Copy link
Contributor

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

@edx-pipeline-bot
Copy link
Contributor

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

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

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

@muhammad-ammar
Copy link
Contributor

muhammad-ammar commented May 31, 2023

@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?

Screenshot 2023-05-31 at 12 21 58 PM

@kdmccormick
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
open-source-contribution PR author is not from Axim or 2U
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants