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

Sass configuration refactor #154

Open
wants to merge 42 commits into
base: main
Choose a base branch
from

Conversation

whiplashwebb
Copy link
Contributor

The Problem

Theme Bulma works just fine until you try to configure the SCSS. This will work fine:

@use '@oruga-ui/theme-bulma/dist/scss/bulma-build';

but if you add a config value like this

@use '@oruga-ui/theme-bulma/dist/scss/bulma-build' with (
    $red: #f00,
    $table-sticky-header-height: 299px,
);

you'll get an error [sass] This module was already loaded, so it can't be configured using "with". $red is a Bulma variable and $table-sticky-header-height is a Theme Bulma variable. Either will cause the error. Interestingly if you only pass $primary the error will not occur.

Confusing, huh? The error complains about the order of the code, but the order of the code isn't changing, we're only passing config values. How can this even be possible? The answer appears to be hoisting. I can't find any sass documentation for this but it seems that when you have a defaulted sass variable like $example: #f00 !default; which is referenced by with (...) syntax, the file which contains $example is hoisted to the top of the pile of sass files being processed. This isn't always a problem but if the hoisted file references Bulma with an @use statement that @use can be shifted to an invalid position relative to other code. Typically this breaks src/assets/scss/components/utils/_variables.scss where the theme configures Bulma to pass secondary/custom color.

This problem is compounded by the fact that Theme Bulma needs to support two use cases: using Bulma as part of the theme or separate from it. The two code pathways can break differently in some situations.

The Solution

The solution is reorganizing Theme Bulma code to ensure that the primary bulma @use with (...) statement always comes first.

  • All !default variables have been collected in two files mirroring the internal structure of Bulma.
    • _initial-defaults.scss collects all variables which do not reference Bulma. These variables have been placed before the main Bulma @use.
    • _derrived-defaults.scss collects all variables which do reference Bulma. These variables have been placed after the main Bulma @use.
  • The primary Bulma @use has been moved to bulma-build.scss so its order can be easily controlled.
  • src/assets/scss/components/utils/_variables.scss has been rewritten. Its purpose is the same, allowing you to pull Bulma variables into any theme component that needs them, but it is now hardened against hoisting errors and able to provide ALL variables from Bulma and the theme rather than just Bulma's derived variables. This is important because...
  • All component files have been refactored to reference variables via _variables.scss. This provides a consistent pathway to variables where order can be preserved, even when the theme is being used separately from Bulma. References to Bulma mixins etc have been preserved as-is, they never error.
  • component-only-build.scss has been added as the entrypoint for the separate bulma use-case. bulma.scss is unchanged but is now intended to be used internally as an entrypoint for component sass.
  • Readme has been updated to reflect the changes and covers both theme use-cases.

I also made some secondary fixes to the theme development environment to make dev and testing easier.

  • The main sass entrypoint has been moved from src/plugin/theme.ts to the new build.ts. This solves an issue where the style import in theme.ts was creating duplicate css based on the default values for color etc, making it impossible to see the changes made in the scss files.
  • Added main-combined.scss and main-separated.scss to represent the two use cases so we can easily test either one. Combined is the default.
  • Added pack:lib npm command and gitignore for the packed tarball. Very handy if you want to test in an external codebase.

src/assets/scss/components/utils/_variables.scss Outdated Show resolved Hide resolved
src/assets/scss/component-only-build.scss Outdated Show resolved Hide resolved
src/assets/scss/_initial-defaults.scss Outdated Show resolved Hide resolved
package.json Show resolved Hide resolved
@whiplashwebb
Copy link
Contributor Author

@mlmoravek have another look at this PR when you get a sec. I refactored how the component defaults work to use the structure you proposed. I think this works a lot better now. I've created oruga-ui/oruga#1172 to update the docs scripting to handle the changes and tested it with new and old code. Everything appears to be working well.

src/assets/scss/components/utils/_variables.scss Outdated Show resolved Hide resolved
@forward "derived-defaults";
@forward "component-defaults";

// Theme component scss
@forward "bulma";
Copy link
Member

Choose a reason for hiding this comment

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

Looks good so far. Could you explain to me again why we have to have the component variables in their own files in the "component-defaults" folder and can't have them in the "components" folder files?
I'm a bit confused because the @forward "component-defaults"; comes just before the @forward "bulma"; not only in this file, but also in the "component-only-build.scss" file...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's the hoisting problem. if you put the defaults in the main component files then if the default is used the whole file gets hoisted. this isn't a problem for the defaults generally, but it will hoist other use or forward statements in the file, which typically shifts a Bulma @use before the primary Bulma @use with (...). There are a few files which don't use Bulma like that where in theory we could get away with putting them together, but that's the exception rather than the rule, and i think the mixing of patterns will just confuse future coders. trust me, this pattern feel inelegant to me too, but it seems to be unavoidable.

"large": vars.$size-large,
) !default;
@forward "../../initial-defaults";
@forward "bulma/sass/utilities/initial-variables";
Copy link
Member

@mlmoravek mlmoravek Feb 2, 2025

Choose a reason for hiding this comment

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

I think you can get rid of this _variables file completely. Then you just have to @use the specific variable files in the other files when needed. But then you can also leave the components as they are and you don't have the need to have a the component-default file.
Maybe try it out. I think got a working version by this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At first I kept this file because I was trying to minimize how much i was changing the existing code. As I iterated I found that it was actually really convenient to be able to bring in all the variables with a single @use. It's convenient for the coder because you don't have to think too hard about where the var is coming from. Is it Bulma or Oruga? Initial or derived? Usually that's a pretty easy question to answer, but it still takes time and requires more understanding of the underlying system. I think this has value since it makes the project more accessible to new contributors. It was also very nice when I refactored all the defaults, as I was able to change the whole file structure with no impact to usage.

To turn it around, what harm do you see in keeping this as-is?

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.

2 participants