Implement functional color variables part 2 [Label, ButtonTableList, ButtonInvisible, Button, Breadcrumb] #1097

VanAnderson · 2021-03-01T15:50:37Z

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

Label
ButtonTableList
ButtonInvisible
Button
Breadcrumb

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

vercel · 2021-03-01T15:50:42Z

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/8htXmHMPDvMSFL1MvRApCgcn3hXB
✅ Preview: https://primer-compone-git-vananderson-implement-functional-colo-05e841.vercel.app

changeset-bot · 2021-03-01T15:50:42Z

🦋 Changeset detected

Latest commit: 7ed5269

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

VanAnderson · 2021-03-01T15:51:16Z

src/__tests__/__snapshots__/Label.tsx.snap

+.c0 background-color:function (props) {
+  return: (0,_core.get)(props.theme,path,fallback);
+}
+
+.c0 background-color:function (props) {
+  return: (0,_core.get)(props.theme,path,fallback);
+}
+


Looks like L73 of src/Label.tsx is not behaving how I'm intending...

VanAnderson · 2021-03-01T15:52:39Z

src/Label.tsx

@@ -70,7 +70,7 @@ const Label = styled.span<

 Label.defaultProps = {
  theme,
-  bg: 'gray.5',
+  bg: get('colors.label.primary.border'),


I don't think this is the right way to do this - is there a correct way to reference these color vars outside of a template?

These props are theme-aware. So you can pass the theme key directly. Props like bg and color assume you're indexing into the colors key so you don't need to write it:

Suggested change

bg: get('colors.label.primary.border'),

bg: 'label.primary.border',

ahhh, okay, that makes sense!

VanAnderson · 2021-03-01T16:03:05Z

src/Label.tsx

@@ -54,7 +54,7 @@ const Label = styled.span<
 >`
  display: inline-block;
  font-weight: ${get('fontWeights.semibold')};
-  color: ${get('colors.white')};
+  color: ${get('colors.text.inverse')};
  border-radius: ${get('radii.3')};

  &:hover {


Commenting here because I'm not sure how to comment on collapsed lines! L66 has a shadow value with a color but I can't find any shadow that would correspond to it - one thing I'm starting to notice is there are places that could benefit from upstream changes to primitives... is that something we want to consider?

Yeah, upstreaming changes to Primer Primitives is part of milestone 3. In this case, I think the friction is caused by a mismatch of the Primer React and Primer CSS implementation of Label. Fixing the mismatch is probably out of scope for this PR (and potentially this epic) but it's something we should be aware of as we work towards aligning all the libraries cc @ashygee @emplums @joelhawksley

@VanAnderson for now, I think these variable choices are fine.

colebemis · 2021-03-01T19:28:26Z

src/Breadcrumb.tsx

  display: inline-block;
  font-size: ${get('fontSizes.1')};
  text-decoration: none;
  &:hover {
    text-decoration: underline;
  }
  &.selected {
-    color: ${get('colors.gray.7')};
+    color: ${get('colors.text.secondary')};


Suggested change

color: ${get('colors.text.secondary')};

color: ${get('colors.text.primary')};

colebemis · 2021-03-01T19:55:52Z

src/Label.tsx

@@ -70,7 +70,7 @@ const Label = styled.span<

 Label.defaultProps = {
  theme,
-  bg: 'gray.5',
+  bg: get('colors.label.primary.border'),


These props are theme-aware. So you can pass the theme key directly. Props like bg and color assume you're indexing into the colors key so you don't need to write it:

Suggested change

bg: get('colors.label.primary.border'),

bg: 'label.primary.border',

bscofield

👍

VanAnderson · 2021-03-03T17:19:02Z

src/Button/Button.tsx

-    box-shadow: ${get('buttons.default.shadow.hover')};
+    background-color: ${get('colors.btn.hoverBg')};
+    border-color: ${get('colors.btn.hoverBorder')};
+    box-shadow: ${get('shadows.btn.focusShadow')}, ${get('shadows.btn.insetShadow')};


There didn't seem to be a clear mapping here for hover shadow in primer css, took my best guess 🤷‍♂️

VanAnderson · 2021-03-03T17:24:17Z

src/Button/Button.tsx

-    box-shadow: ${get('buttons.default.shadow.active')};
-    border-color: ${get('buttons.default.border.active')};
+    background-color: ${get('colors.btn.selectedBg')};
+    box-shadow: ${get('shadows.btn.shadowActive')};


I think this is mapped to shadowInset in primer css, but it seemed like it would be better suited as shadowActive

breadcrumb tweaks

label tweaks

src/Button/Button.tsx

colebemis · 2021-03-04T00:44:13Z

src/Button/ButtonInvisible.tsx

@@ -12,11 +12,11 @@ const ButtonInvisible = styled(ButtonBase)<ButtonBaseProps & ButtonSystemProps &
  box-shadow: none;

  &:disabled {
-    color: ${get('buttons.default.color.disabled')};
+    color: ${get('colors.text.disabled')};


Line 9 of this file should use colors.text.link

Co-authored-by: Cole Bemis <colebemis@github.com>

colebemis

🎉

VanAnderson commented Mar 1, 2021

View reviewed changes

VanAnderson marked this pull request as ready for review March 1, 2021 15:54

VanAnderson commented Mar 1, 2021

View reviewed changes

VanAnderson force-pushed the VanAnderson/implement-functional-color-vars-2 branch from 295075c to 4b794b3 Compare March 1, 2021 16:19

vercel bot temporarily deployed to Preview March 1, 2021 16:19 Inactive

VanAnderson force-pushed the VanAnderson/implement-functional-color-vars-2 branch from 4b794b3 to 29de1a0 Compare March 1, 2021 18:25

vercel bot temporarily deployed to Preview March 1, 2021 18:25 Inactive

VanAnderson force-pushed the VanAnderson/implement-functional-color-vars-2 branch from 29de1a0 to a9fdcbb Compare March 1, 2021 18:56

vercel bot temporarily deployed to Preview March 1, 2021 18:56 Inactive

VanAnderson force-pushed the VanAnderson/implement-functional-color-vars-2 branch from a9fdcbb to 2aa25b7 Compare March 1, 2021 18:57

vercel bot temporarily deployed to Preview March 1, 2021 19:00 Inactive

colebemis reviewed Mar 1, 2021

View reviewed changes

VanAnderson force-pushed the VanAnderson/implement-functional-color-vars-2 branch from 2aa25b7 to 56e28a2 Compare March 2, 2021 16:28

vercel bot temporarily deployed to Preview March 2, 2021 16:28 Inactive

VanAnderson force-pushed the VanAnderson/implement-functional-color-vars-2 branch from 56e28a2 to 9aed7e1 Compare March 2, 2021 17:24

vercel bot temporarily deployed to Preview March 2, 2021 17:24 Inactive

VanAnderson changed the title ~~Implement functional color variables [Label, ButtonTableList, ButtonInvisible, Button, Breadcrumb]~~ Implement functional color variables part 2 [Label, ButtonTableList, ButtonInvisible, Button, Breadcrumb] Mar 2, 2021

bscofield approved these changes Mar 2, 2021

View reviewed changes

VanAnderson commented Mar 3, 2021

View reviewed changes

vercel bot temporarily deployed to Preview March 3, 2021 17:21 Inactive

VanAnderson force-pushed the VanAnderson/implement-functional-color-vars-2 branch from f697979 to 9aed7e1 Compare March 3, 2021 17:23

VanAnderson commented Mar 3, 2021

View reviewed changes

VanAnderson added 5 commits March 3, 2021 11:30

implement functional color variables for Breadcrumb

565f198

breadcrumb tweaks

implement functional color variables for Button

170876e

implement functional color variables for ButtonInvisible

67cc5b2

implement functional color variables for ButtonTableList

1fb750a

Use functional color variables in Label

85381fc

label tweaks

update snapshots

ef7cd96

VanAnderson force-pushed the VanAnderson/implement-functional-color-vars-2 branch from 9aed7e1 to ef7cd96 Compare March 3, 2021 17:31

vercel bot had a problem deploying to Preview March 3, 2021 17:31 Failure

remove default theme prop

4161efd

vercel bot temporarily deployed to Preview March 3, 2021 17:39 Inactive

colebemis reviewed Mar 4, 2021

View reviewed changes

src/Button/Button.tsx Outdated Show resolved Hide resolved

colebemis reviewed Mar 4, 2021

View reviewed changes

src/Button/Button.tsx Outdated Show resolved Hide resolved

colebemis reviewed Mar 4, 2021

View reviewed changes

Remove unnecessary colors

0debd74

Co-authored-by: Cole Bemis <colebemis@github.com>

vercel bot temporarily deployed to Preview March 4, 2021 15:39 Inactive

VanAnderson added 2 commits March 4, 2021 09:41

ButtonInvisible uses functional variables for link color

b71bd19

update button snapshot

05ec28d

VanAnderson force-pushed the VanAnderson/implement-functional-color-vars-2 branch from a0096aa to 05ec28d Compare March 4, 2021 15:42

vercel bot temporarily deployed to Preview March 4, 2021 15:43 Inactive

update snapshots

f0644c0

vercel bot temporarily deployed to Preview March 4, 2021 16:33 Inactive

Merge branch 'main' into VanAnderson/implement-functional-color-vars-2

7ed5269

vercel bot temporarily deployed to Preview March 4, 2021 16:42 Inactive

colebemis approved these changes Mar 4, 2021

View reviewed changes

colebemis merged commit 7fd57c8 into main Mar 4, 2021

colebemis deleted the VanAnderson/implement-functional-color-vars-2 branch March 4, 2021 16:46

github-actions bot mentioned this pull request Mar 4, 2021

Release Tracking #1093

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement functional color variables part 2 [Label, ButtonTableList, ButtonInvisible, Button, Breadcrumb] #1097

Implement functional color variables part 2 [Label, ButtonTableList, ButtonInvisible, Button, Breadcrumb] #1097

VanAnderson commented Mar 1, 2021

vercel bot commented Mar 1, 2021 •

edited

Loading

changeset-bot bot commented Mar 1, 2021 •

edited

Loading

VanAnderson Mar 1, 2021

VanAnderson Mar 1, 2021

colebemis Mar 1, 2021

VanAnderson Mar 1, 2021

VanAnderson Mar 1, 2021

colebemis Mar 1, 2021

colebemis Mar 1, 2021

colebemis Mar 1, 2021

colebemis Mar 1, 2021

bscofield left a comment

VanAnderson Mar 3, 2021

VanAnderson Mar 3, 2021

colebemis Mar 4, 2021

colebemis left a comment

	bg: get('colors.label.primary.border'),
	bg: 'label.primary.border',

	color: ${get('colors.text.secondary')};
	color: ${get('colors.text.primary')};

Implement functional color variables part 2 [Label, ButtonTableList, ButtonInvisible, Button, Breadcrumb] #1097

Implement functional color variables part 2 [Label, ButtonTableList, ButtonInvisible, Button, Breadcrumb] #1097

Conversation

VanAnderson commented Mar 1, 2021

vercel bot commented Mar 1, 2021 • edited Loading

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

🦋 Changeset detected

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bscofield left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

colebemis left a comment

Choose a reason for hiding this comment

vercel bot commented Mar 1, 2021 •

edited

Loading

changeset-bot bot commented Mar 1, 2021 •

edited

Loading