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

Fix bug with setting theme in compatability mode resulting in a noop #9617

Closed
joshblack opened this issue Sep 7, 2021 · 8 comments · Fixed by #9677
Closed

Fix bug with setting theme in compatability mode resulting in a noop #9617

joshblack opened this issue Sep 7, 2021 · 8 comments · Fixed by #9677
Assignees

Comments

@joshblack
Copy link
Contributor

joshblack commented Sep 7, 2021

This issue was originally reported by @janhassel where the compat theme was not able to have its value changed.

Code to reproduce:

@use '@carbon/react/scss/reset';
@use '@carbon/react/scss/compat/themes';
@use '@carbon/react/scss/compat/theme' with (
  $theme: themes.$g90
);

body {
  background: theme.$background;
  color: theme.$text-primary;
}

Here, we expected the tokens to use values from the g90 theme but they use the value from the white theme instead. It seems like this is related to the issue Jan reported in that the theme would not allow you to correctly retrieve interactive-01 as a token because the themes were not being merged

@joshblack joshblack changed the title Fix bug with setting theme in compatability mode resulting in a noop @tw15egan Fix bug with setting theme in compatability mode resulting in a noop Sep 7, 2021
@joshblack
Copy link
Contributor Author

@tw15egan let me know if you need any more steps to reproduce!

@janhassel let me know if I misframed your original issue, I know the wording/example changed a bit in this ticket and wanted to double-check if I was incorrectly assuming it was the same issue as what you were running into.

@janhassel
Copy link
Member

@joshblack The issue I ran into was the following: I wanted to import all Carbon styles in g100 theme. I tried these two approaches based on the docs:

@use '@carbon/styles';
@use '@carbon/styles/scss/themes';
@use '@carbon/styles/scss/theme' with (
  $theme: themes.$g100,
);
Error
./src/index.scss (./node_modules/css-loader/dist/cjs.js??ref--5-oneOf-6-1!./node_modules/postcss-loader/src??postcss!./node_modules/resolve-url-loader??ref--5-oneOf-6-3!./node_modules/sass-loader/dist/cjs.js??ref--5-oneOf-6-4!./src/index.scss)
SassError: This module was already loaded, so it can't be configured using "with".
   ┌──> src/index.scss
3  │ ┌ @use '@carbon/styles/scss/theme' with (
4  │ │   $theme: themes.$g100,
5  │ │ );
   │ └─^ new load
   ╵
   ┌──> node_modules/@carbon/styles/index.scss
15 │   @forward 'scss/theme';
   │   ━━━━━━━━━━━━━━━━━━━━━ original load
   ╵
  src/index.scss 3:1  root stylesheet
@use '@carbon/styles/scss/themes';
@use '@carbon/styles/scss/theme' with (
  $theme: themes.$g100,
);
@use '@carbon/styles';
Error
./src/index.scss (./node_modules/css-loader/dist/cjs.js??ref--5-oneOf-6-1!./node_modules/postcss-loader/src??postcss!./node_modules/resolve-url-loader??ref--5-oneOf-6-3!./node_modules/sass-loader/dist/cjs.js??ref--5-oneOf-6-4!./src/index.scss)
SassError: "Unable to find token: interactive-01 in current $theme"
   ╷
18 │     @return var(--#{config.$prefix}-#{$token}, #{theme.get($token)});
   │                                                  ^^^^^^^^^^^^^^^^^
   ╵
  node_modules/@carbon/themes/scss/compat/generated/_tokens.scss 18:50              -get()
  node_modules/@carbon/themes/scss/compat/generated/_tokens.scss 23:18              @forward
  node_modules/@carbon/themes/scss/compat/_tokens.scss 8:1                          @forward
  node_modules/@carbon/styles/scss/compat/_theme.scss 9:1                           @use
  node_modules/@carbon/styles/scss/components/code-snippet/_code-snippet.scss 11:1  @forward
  node_modules/@carbon/styles/scss/components/code-snippet/_index.scss 8:1          @use
  node_modules/@carbon/styles/scss/components/_index.scss 12:1                      @use
  node_modules/@carbon/styles/index.scss 18:1                                       @use
  src/index.scss 5:1                                                                root stylesheet

@joshblack
Copy link
Contributor Author

@janhassel thanks!

Side-note: I think you'll have to always use the second form just as a heads up otherwise you'll get the error where a module has been used before it's being configured.

@joshblack joshblack assigned joshblack and unassigned tw15egan Sep 14, 2021
@joshblack
Copy link
Contributor Author

joshblack commented Sep 15, 2021

@janhassel I think what you'll need to end up doing is:

@use '@carbon/styles/scss/compat/themes' as compat;
@use '@carbon/styles/scss/themes';
@use '@carbon/styles/scss/theme' with (
  // Make sure new tokens are in the correct theme (by default they are themes.$white)
  $fallback: themes.$g100,
  // Add the compat tokens to the theme
  $theme: compat.$g100,
);
@use '@carbon/styles';

This should be temporary as our codebase is still internally using tokens from v10. The error you ran into basically happens when a v11 theme is provided but the component is looking for a v10 token.

Let me know if this works 👀

@joshblack
Copy link
Contributor Author

After #9677 you should be able to write what you wrote in example 2 so you can wait for that too!

@janhassel
Copy link
Member

@joshblack Your interim solutions seems to work great. As soon as the PR is merged and included in a release, I'll definitely try it out and let you know as it would be a lot cleaner.

@janhassel
Copy link
Member

@joshblack @sstrubberg Sadly it seems that even with v0.6.0 the same issue still occurs. The interim solution from Josh works, the second code snippet I shared originally throws the original error.

@joshblack
Copy link
Contributor Author

@janhassel I think until we update the code to use the new tokens we'll run into this issue since the theme is looking for tokens that don't exist. I'll make sure we will go and update them in the next release though so that you can configure the theme as expected 👍

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

Successfully merging a pull request may close this issue.

3 participants