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(reset): fix reset specifying anchor color #7216

Merged
merged 22 commits into from
Dec 9, 2020

Conversation

DusanMilko
Copy link
Contributor

@DusanMilko DusanMilko commented Nov 3, 2020

Closes: #7206

Currently reset is setting hard hex color for anchor that is not ideal.
Color is not a token so causes issues in regards to theme

@netlify
Copy link

netlify bot commented Nov 3, 2020

Deploy preview for carbon-elements ready!

Built with commit c572c43

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

@netlify
Copy link

netlify bot commented Nov 3, 2020

Deploy preview for carbon-components-react ready!

Built without sensitive environment variables with commit c572c43

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

@netlify
Copy link

netlify bot commented Nov 3, 2020

✔️ Deploy preview for carbon-elements ready!

🔨 Explore the source changes: 0302891

🔍 Inspect the deploy logs: https://app.netlify.com/sites/carbon-elements/deploys/5fd15942eb244f0007cca5f9

😎 Browse the preview: https://deploy-preview-7216--carbon-elements.netlify.app

@netlify
Copy link

netlify bot commented Nov 3, 2020

Deploy preview for carbon-components-react ready!

Built with commit 0302891

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

@andreancardona
Copy link
Contributor

andreancardona commented Nov 4, 2020

@DusanMilko hello! it seems the tests are failing - you will have to run yarn test -u to update the snapshot

@@ -75,7 +75,7 @@
}

a {
color: #0062ff;
color: $interactive-01;
Copy link
Contributor

Choose a reason for hiding this comment

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

If I remember correctly, I think it's a hex value due to this token coming from the themes package. Since themes depends on type, if we had type depends on themes then we would have a cycle between the two.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

then should we remove this hard hex from the reset?

Copy link
Contributor

Choose a reason for hiding this comment

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

@DusanMilko maybe we could swing something like:

  a {
    @if global-variable-exists($carbon--theme) and
      map-has-key($carbon--theme, 'link-01')
    {
      color: map-get($carbon--theme, 'link-01');
    } @else {
      color: #0062ff;
    }
  }

?

Copy link
Member

Choose a reason for hiding this comment

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

Do we even need to reset the color? It doesn't look like we reset any other color values?

Copy link
Contributor

Choose a reason for hiding this comment

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

@tw15egan I think you're right that we don't need to reset colors, I assume that when the default type mixin was made it was just translating over from the v9 default styles which we could totally just drop in v11 👍

@DusanMilko
Copy link
Contributor Author

@DusanMilko hello! it seems the tests are failing - you will have to run yarn test -u to update the snapshot

Got this error:

● Validation Error:

Preset jest-config-carbon not found.

Configuration Documentation:
https://jestjs.io/docs/configuration.html

@DusanMilko
Copy link
Contributor Author

@joshblack why is this failing?

@tw15egan
Copy link
Member

tw15egan commented Nov 23, 2020

Screen Shot 2020-11-23 at 2 58 55 PM

Error: argument $name of global-variable-exists($name) must be a string

Seems like the @if global-variable-exists($carbon--theme) and is causing it, would it work if we switch it to #{$carbon--theme}, or would this always evaluate true?

@DusanMilko
Copy link
Contributor Author

DusanMilko commented Nov 23, 2020

Screen Shot 2020-11-23 at 2 58 55 PM

Error: argument $name of global-variable-exists($name) must be a string

Seems like the @if global-variable-exists($carbon--theme) and is causing it, would it work if we switch it to #{$carbon--theme}, or would this always evaluate true?

Not sure, i know josh mentioned removing this possibly in carbon 11, is there a big reason we can't remove it now or atleast move out of reset and move into theme where it makes more sense? (I don't feel like its a breaking change but I differ to you)

@joshblack
Copy link
Contributor

Hey all! Sorry for the delay over break, global-variable-exists takes in a string which I think is causing it to fail. Instead of global-variable-exists($carbon--theme) it would be global-variable-exists('carbon--theme')

Reference: https://sass-lang.com/documentation/modules/meta#global-variable-exists

@DusanMilko
Copy link
Contributor Author

Hey all! Sorry for the delay over break, global-variable-exists takes in a string which I think is causing it to fail. Instead of global-variable-exists($carbon--theme) it would be global-variable-exists('carbon--theme')

Reference: https://sass-lang.com/documentation/modules/meta#global-variable-exists

Still seems to be failing

@kodiakhq kodiakhq bot merged commit 52390f4 into carbon-design-system:master Dec 9, 2020
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.

bug(theme): reset causing issue in theme
5 participants