-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
feat(colors): add hover colors to @carbon/colors
#8942
feat(colors): add hover colors to @carbon/colors
#8942
Conversation
FYI @sstrubberg here's that draft PR! |
✔️ Deploy Preview for carbon-elements ready! 🔨 Explore the source changes: e1a51c5 🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-elements/deploys/60d50a4eb84985000813696f 😎 Browse the preview: https://deploy-preview-8942--carbon-elements.netlify.app |
✔️ Deploy Preview for carbon-components-react ready! 🔨 Explore the source changes: e1a51c5 🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-components-react/deploys/60d50a4ef2583a000757f528 😎 Browse the preview: https://deploy-preview-8942--carbon-components-react.netlify.app |
Thx! I got all the colors in. Still getting some build errors though and i'm not confident about the tests either. |
✔️ Deploy Preview for carbon-react-next ready! 🔨 Explore the source changes: e1a51c5 🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-react-next/deploys/60d50a4e97b0a600075d8340 😎 Browse the preview: https://deploy-preview-8942--carbon-react-next.netlify.app/iframe |
FYI @aagonzales in this PR we are adding the hover colors from the IDL into the colors package. We used the values from the file and converted them to HEX. The value themselves are defined in the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so many colors!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good to me
Also just a heads up @aagonzales I updated the colors preview so the table should include the color hovers in there with the hex value 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Values that need to be updated:
Purple-10-hover: #ede5ff
Cyan-100-hover: #0b2947
Blue-30-hover: #8ab6ff
packages/styles/scss/__tests__/__snapshots__/colors-test.js.snap
Outdated
Show resolved
Hide resolved
packages/styles/scss/__tests__/__snapshots__/colors-test.js.snap
Outdated
Show resolved
Hide resolved
packages/styles/scss/__tests__/__snapshots__/colors-test.js.snap
Outdated
Show resolved
Hide resolved
Thanks @aagonzales ! Just updated. Just a quick heads up, all the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lol i was dumb and didn't even think to look at the elements preview smh
…n into 8822-add-hover-colors-v2
Closes #8822
This PR updates our
@carbon/colors
package to include the hover palette from the IDL. It also updates our build task to export hover variables in Sass.Changelog
New
Changed
Removed
Testing / Reviewing
yarn build
in@carbon/colors
scss/mixins.scss
file includes the hover colors from the IDLindex.scss
file includes the hover colors from the IDL