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.

Sign up for GitHub

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

Jump to bottom

Implement functional color variables part 5 [Flash, Header] #1102

Merged
merged 6 commits into from
Mar 4, 2021
Merged

Implement functional color variables part 5 [Flash, Header] #1102

merged 6 commits into from
Mar 4, 2021

Conversation

VanAnderson
Copy link
Contributor

This PR updates the following components to use functional color variables in preparation for color mode support:

  • Flash
  • Header

Part of https://github.com/github/design-systems/issues/1219

@changeset-bot
Copy link

changeset-bot bot commented Mar 3, 2021 •
edited
Loading

🦋 Changeset detected

Latest commit: 1f1d387

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@primer/components Patch

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

@vercel
Copy link

vercel bot commented Mar 3, 2021 •
edited
Loading

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/primer/primer-components/9tzccCtsZamkGmwgHhbQAeFGzcJP
✅ Preview: https://primer-compone-git-vananderson-implement-functional-colo-b19ae0.vercel.app

VanAnderson
VanAnderson commented Mar 3, 2021
View reviewed changes
src/Flash.tsx Outdated Show resolved Hide resolved
VanAnderson
VanAnderson commented Mar 3, 2021
View reviewed changes
src/__tests__/Flash.tsx Outdated Show resolved Hide resolved
@VanAnderson VanAnderson force-pushed the VanAnderson/implement-functional-color-vars-5 branch from 292ac11 to 2781d06 Compare March 3, 2021 15:41
@vercel vercel bot temporarily deployed to Preview March 3, 2021 15:41 Inactive
@VanAnderson VanAnderson force-pushed the VanAnderson/implement-functional-color-vars-5 branch from 2781d06 to f6101f0 Compare March 3, 2021 15:51
@vercel vercel bot temporarily deployed to Preview March 3, 2021 15:51 Inactive
colebemis
colebemis requested changes Mar 4, 2021
View reviewed changes
Copy link
Contributor

@colebemis colebemis left a comment

Choose a reason for hiding this comment

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

Nice work, @VanAnderson! Just left a few suggestions.

src/Flash.tsx Outdated Show resolved Hide resolved
src/Flash.tsx Outdated
@@ -28,7 +57,7 @@ const Flash = styled.div<
}

svg {
color: ${props => get(`flashIcon.${props.variant}`)(props.theme)};
color: ${props => get(`colors.alert.${variantAliases[props.variant || 'default']}.icon`)(props.theme)};
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can move this logic into the variants object and eliminate the need for variantAliases :)

Suggested change
color: ${props => get(`colors.alert.${variantAliases[props.variant || 'default']}.icon`)(props.theme)};

src/__tests__/Flash.tsx Outdated Show resolved Hide resolved
src/__tests__/Flash.tsx Outdated Show resolved Hide resolved
src/theme-preval.js Outdated
@@ -258,33 +258,6 @@ const buttons = {
}
}

const flash = {
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

@vercel vercel bot temporarily deployed to Preview March 4, 2021 16:23 Inactive
@vercel vercel bot temporarily deployed to Preview March 4, 2021 16:29 Inactive
@VanAnderson VanAnderson force-pushed the VanAnderson/implement-functional-color-vars-5 branch from 6773c27 to 67d27b3 Compare March 4, 2021 16:40
@vercel vercel bot temporarily deployed to Preview March 4, 2021 16:40 Inactive
@vercel vercel bot temporarily deployed to Preview March 4, 2021 16:47 Inactive
@colebemis colebemis merged commit a300955 into main Mar 4, 2021
@colebemis colebemis deleted the VanAnderson/implement-functional-color-vars-5 branch March 4, 2021 17:05
@github-actions github-actions bot mentioned this pull request Mar 4, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@colebemis colebemis colebemis approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@VanAnderson @colebemis