Implement functional color variables part 5 [Flash, Header] #1102
Conversation
🦋 Changeset detectedLatest commit: 1f1d387 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/primer/primer-components/9tzccCtsZamkGmwgHhbQAeFGzcJP |
VanAnderson
force-pushed
the
VanAnderson/implement-functional-color-vars-5
branch
from
292ac11 to
2781d06
Compare
VanAnderson
force-pushed
the
VanAnderson/implement-functional-color-vars-5
branch
from
2781d06 to
f6101f0
Compare
colebemis
left a comment
colebemis
left a comment
There was a problem hiding this comment.
Nice work, @VanAnderson! Just left a few suggestions.
src/Flash.tsx
Outdated
| svg { | ||
| color: ${props => get(`flashIcon.${props.variant}`)(props.theme)}; | ||
| color: ${props => get(`colors.alert.${variantAliases[props.variant || 'default']}.icon`)(props.theme)}; |
There was a problem hiding this comment.
I think we can move this logic into the variants object and eliminate the need for variantAliases :)
| color: ${props => get(`colors.alert.${variantAliases[props.variant || 'default']}.icon`)(props.theme)}; |
| } | ||
| } | ||
|
|
||
| const flash = { |
There was a problem hiding this comment.
It's technically a breaking change to remove these values from the theme. Let's hold off on doing that until the "Remove old color variables from theme" step of this milestone so we can batch all the breaking changes together.
VanAnderson
force-pushed
the
VanAnderson/implement-functional-color-vars-5
branch
from
6773c27 to
67d27b3
Compare
This PR updates the following components to use functional color variables in preparation for color mode support:
Part of https://github.com/github/design-systems/issues/1219