feat: move general imports to specific scss files

[andrey-canon]

Description

This pull request change the way how the scss xmodule imports are made, currently this line adds bourbun and lms/theme/variables as general imports at the top of the file _module-styles.scss that is generated after running the command openedx-assets xmodule, example

@import 'bourbon/bourbon';
@import 'lms/theme/variables';
.xmodule_edit.xmodule_AboutBlock {
  @import "000-cd23cf32f86efa1c4179df0201722e55.scss";
  @import "001-a10fc3e0fd6aca63426a89e75fe69c31.scss";
}
...
.xmodule_edit.xmodule_VideoBlock {
  @import "000-e5d1b330372214d8a7b774e826640db5.scss";
}

This makes harder to know which file really needs of that import to compile and it will generate css files bigger after decoupling process as this pr suggest.

Results and conclusions

For the lms(http://local.overhang.io:8000/static/css/lms-course.css), the difference was just two lines

That was due to I removed "@import 'lms/theme/variables';", however that line is redundant that import is made 4 times in the master branch with this it's just made two times

For cms(http://studio.local.overhang.io:8001/static/studio/css/studio-main-v1.css), when I just remove the general import the difference was bigger the results were the following:

1. Same case as lms
   

2. font weight was set to the selector .xmodule_edit.xmodule_VideoBlock .component-tab .blue-button
   

3. more attributes were set for the selector .xmodule_edit.xmodule_VideoBlock .component-tab .blue-button
   

4 Multiple changes all related with `.xmodule_edit.xmodule_VideoBlock .component-tab .blue-button

That was happening because the import order had changed and an specific button mixin was not overridden by bourbon, the file that generates that difference is this one, that use the blue-button mixin, and that mixin use inside the button mixin in this line, when the bourbon import is made in the file openedx-assets xmodule that override the button mixin with this however when I applied this changes that uses this mixin

I consider those changes doesn't make any difference in the visual resul since currently that class is not present in the studio editor template

Therefore the visual result is the same and I also made a visual comparison to verify that, for that reason I think the best option is to remove that class from the style file and the final result for cms is the next

Testing instructions

To test this a comparison is required, so follow the next instructions with first the master branch and then with this.(Those instructions are base on a tutor environment)

1. Go inside your docker image docker exec -it tutor_nightly_dev-lms-1 /bin/bash
2. Run this command openedx-assets xmodule && openedx-assets common. (probably with master branch this is not necessary since this is part of the provision process)
3. Go to http://local.overhang.io:8000/static/css/lms-course.css and copy the content and save it in a temporary file
4. Go to http://studio.local.overhang.io:8001/static/studio/css/studio-main-v1.css and copy the content and save it in a temporary file

Repeat with and/move_general_imports branch

After that you will have to compare the result of the master branch and the pr branch, I used this page

If you don't want to compare the compiled filed, you can use a online beautifier like this https://www.cleancss.com/css-beautify/ and compare that result

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

[kdmccormick]

I think this will be a good change, but I have some questions to make sure that I understand it.

[kdmccormick]

Is it true that none of the XModules use lms/theme/variables?

[andrey-canon]

no, that's false, probably one of them uses lms/theme/variables, however in this context it's hard to know which one, because the file _module-styles.scss is imported in a bigger file (_build-course.scss or _build-v1.scss) that already import that.

if you check this list after decoupling you will find that more of one import is necessary

[kdmccormick]

Got it. Do you want to merge this PR first? If so, when you rebase the decoupling PR, do you think you will put @import 'lms/theme/variables'; back for some files?

[andrey-canon]

yes, I'd like to merge this one first, in that way I can add just the required imports in the specific files in this PR

[kdmccormick]

Sorry, I am having trouble understanding what you mean here:

That was happening because the import order had changed and an specific button mixin was not overridden by bourbon, the file that generates that difference is this one, that use the blue-button mixin, and that mixin use inside the button mixin in this line, when the bourbon import is made in the file openedx-assets xmodule that override the button mixin with this however when I applied this changes that uses this mixin

As I understand it, you are removing this block because it generates extra CSS in the video editor (which is the only thing that uses tabs.scss), but that CSS has no visual impact on the video editor, right?

[andrey-canon]

yes, that is the conclusion, that has no visual impact, the comment is probably a bad explanation of why the blue-button style change when I change the import place, but at the end it doesn't matter since that class doesn't exist in the current video editor

[kdmccormick]

Sounds good to me 👍🏻

[kdmccormick]

@andrey-canon Nice work, this looks good to me. Please just rebase & add some extra details to your commit message as well as a link back to the parent issue (#31624). I should be able to merge this as early as tomorrow.

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