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(component-tokens): only emit static values in emit-component-tokens #7801

Merged
merged 2 commits into from
Feb 16, 2021

Conversation

janhassel
Copy link
Member

Closes #7800

The emit-component-tokens mixin currently emits all tokens as css custom properties if the feature flag is enabled, resulting in the actual css variable never declared and the colors always falling back to the default, white theme.

The workaround so far was to include the component's styles again, like so:

:root {
  @include carbon--theme($carbon--theme--g100, true) {
    @include emit-component-tokens($tag-colors);
    @include tags;
  }
}

This results in a lot of unnecessary css being generated and can create specificity issues.

Changelog

New

  • Added an optional parameter $force-static-values to get-token-value mixin.

Changed

  • Pass true to $force-static-values from emit-component-tokens mixin.

Testing / Reviewing

Before, this code should not result in the correct tag colors:

:root {
    @include carbon--theme($carbon--theme--g100, true) {
      @include emit-component-tokens($tag-colors);
    }
  }

With the changes in this PR, it should result in the correct tag colors.

In the inspector, all tag colors should be shown as static values:
image

compared to before
image

@netlify
Copy link

netlify bot commented Feb 15, 2021

Deploy preview for carbon-elements ready!

Built with commit ac30e17

https://deploy-preview-7801--carbon-elements.netlify.app

@netlify
Copy link

netlify bot commented Feb 15, 2021

Deploy preview for carbon-components-react ready!

Built without sensitive environment variables with commit ac30e17

https://deploy-preview-7801--carbon-components-react.netlify.app

@joshblack
Copy link
Contributor

@janhassel would we want this to be the default behavior (forcing static values)? This specific mixin is private IIRC so we can totally update it if it should be that behavior by default.

@janhassel
Copy link
Member Author

@joshblack I was thinking since the mixin is called far more often without the force-static-values parameter than with it, it would make more sense to have it default to false.

.#{$prefix}--tag--red {
@include tag-theme(
get-token-value($tag-colors, 'tag-background-red'),
get-token-value($tag-colors, 'tag-color-red'),
get-token-value($tag-colors, 'tag-hover-red')
);
}
.#{$prefix}--tag--magenta {
@include tag-theme(
get-token-value($tag-colors, 'tag-background-magenta'),
get-token-value($tag-colors, 'tag-color-magenta'),
get-token-value($tag-colors, 'tag-hover-magenta')
);
}
.#{$prefix}--tag--purple {
@include tag-theme(
get-token-value($tag-colors, 'tag-background-purple'),
get-token-value($tag-colors, 'tag-color-purple'),
get-token-value($tag-colors, 'tag-hover-purple')
);
}
.#{$prefix}--tag--blue {
@include tag-theme(
get-token-value($tag-colors, 'tag-background-blue'),
get-token-value($tag-colors, 'tag-color-blue'),
get-token-value($tag-colors, 'tag-hover-blue')
);
}
.#{$prefix}--tag--cyan {
@include tag-theme(
get-token-value($tag-colors, 'tag-background-cyan'),
get-token-value($tag-colors, 'tag-color-cyan'),
get-token-value($tag-colors, 'tag-hover-cyan')
);
}
.#{$prefix}--tag--teal {
@include tag-theme(
get-token-value($tag-colors, 'tag-background-teal'),
get-token-value($tag-colors, 'tag-color-teal'),
get-token-value($tag-colors, 'tag-hover-teal')
);
}
.#{$prefix}--tag--green {
@include tag-theme(
get-token-value($tag-colors, 'tag-background-green'),
get-token-value($tag-colors, 'tag-color-green'),
get-token-value($tag-colors, 'tag-hover-green')
);
}
.#{$prefix}--tag--gray {
@include tag-theme(
get-token-value($tag-colors, 'tag-background-gray'),
get-token-value($tag-colors, 'tag-color-gray'),
get-token-value($tag-colors, 'tag-hover-gray')
);
}
.#{$prefix}--tag--cool-gray {
@include tag-theme(
get-token-value($tag-colors, 'tag-background-cool-gray'),
get-token-value($tag-colors, 'tag-color-cool-gray'),
get-token-value($tag-colors, 'tag-hover-cool-gray')
);
}
.#{$prefix}--tag--warm-gray {
@include tag-theme(
get-token-value($tag-colors, 'tag-background-warm-gray'),
get-token-value($tag-colors, 'tag-color-warm-gray'),
get-token-value($tag-colors, 'tag-hover-warm-gray')
);
}

Open to change it though if you think it makes more sens to have the default set as true.

Copy link
Member

@tw15egan tw15egan left a comment

Choose a reason for hiding this comment

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

LGTM 👍 ✅

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

Successfully merging this pull request may close these issues.

Tag component with css variables
3 participants