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

Add Primitives v8 colors #529

Merged
merged 11 commits into from
Mar 28, 2023
Merged
Show file tree
Hide file tree
Changes from 10 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/cold-houses-swim.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@primer/primitives": patch
---

Add Primitives v8 colors (private dist for testing)
Copy link
Contributor

@simurai simurai Mar 27, 2023

Choose a reason for hiding this comment

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

Do we have another name for this effort? It would be v3 of renaming efforts, although it doesn't match Primitives's current semver version. πŸ€”

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm trying to refer to this as the next major (v8) release of Primitives vs. v3, which I think sounds a little nicer and more accurate! In v8 we will also ship all the sizing tokens without fallbacks, so it truly feels like a big major release.

2 changes: 1 addition & 1 deletion docs/storybook/.storybook/preview.js
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,7 @@ const tempTheme = deepmerge(theme, {
borderHover: 'var(--underlineNav-borderColor-hover)',
},
actionListItem: {
inlineDivider: 'var(--borderColor-decorative)',
inlineDivider: 'var(--borderColor-muted)',
default: {
hoverBg: 'var(--control-transparent-bgColor-hover)',
hoverBorder: 'var(--control-transparent-borderColor-hover)',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,13 +8,7 @@ export default {
},
}

const borderColors = [
'borderColor-default',
'borderColor-muted',
'borderColor-decorative',
'borderColor-emphasis',
'borderColor-disabled',
]
const borderColors = ['borderColor-default', 'borderColor-muted', 'borderColor-emphasis', 'borderColor-disabled']

export const Border = () => {
return (
Expand Down
2 changes: 1 addition & 1 deletion docs/storybook/stories/Demos/BorderDemo.stories.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ export const Borders: ComponentStory<typeof ColorPreview> = () => {
<ActionList.Item variant="danger">Danger</ActionList.Item>
</ActionList>
</div>
<CSSTokenSwatch color="borderColor-decorative" />
<CSSTokenSwatch color="borderColor-muted" />
</div>
<div style={{display: 'flex', flexDirection: 'column', gap: '1rem'}}>
<div className="ActionList alt" style={{display: 'flex', gap: '1rem'}}>
Expand Down
4 changes: 2 additions & 2 deletions src/platforms/css.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,10 @@ const getCssSelectors = (outputFile: string): {selector: string; selectorLight:
const lastSlash = outputFile.lastIndexOf('/')
const outputBasename = outputFile.substring(lastSlash + 1, outputFile.indexOf('.'))
const themeName = outputBasename.replace(/-/g, '_')
const mode = outputBasename.substring(0, 4) === 'dark' ? 'dark' : 'light'
// const mode = outputBasename.substring(0, 4) === 'dark' ? 'dark' : 'light'

return {
selector: `[data-color-mode="${mode}"][data-${mode}-theme="${themeName}"]`,
selector: `[data-color-mode="light"][data-light-theme="${themeName}"], [data-color-mode="dark"][data-dark-theme="${themeName}"]`,
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are you changing this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was working on updating the theme picker in Storybook last week with @colebemis and we found a bug with the selector stack. I think the theme picker was written in such a way that the theme would never fail to match the selector, and Cole suggested this change to make it match the logic in dotcom. What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

If this is really the case, I will do a PR just for this, so that it is not hidden in this giant one and can be more easily tracked.

However I am not sure this is actually correct. Maybe @colebemis can add to this discussion.
In the current version we get the following:

[data-color-mode="light"][data-light-theme="light_colorblind"],
[data-color-mode="auto"][data-light-theme="light_colorblind"] {

or

[data-color-mode="dark"][data-dark-theme="dark_high_contrast"],
[data-color-mode="auto"][data-light-theme="dark_high_contrast"] {

With the proposed changes we get:

[data-color-mode="light"][data-light-theme="light_colorblind"],
[data-color-mode="dark"][data-dark-theme="light_colorblind"],
[data-color-mode="auto"][data-light-theme="light_colorblind"] {

or

[data-color-mode="light"][data-light-theme="dark_high_contrast"],
[data-color-mode="dark"][data-dark-theme="dark_high_contrast"],
[data-color-mode="auto"][data-light-theme="dark_high_contrast"] {

What is added is a line I think will never execute. As far as I see you can't get a combination of data-color-mode="dark" and a light theme like data-dark-theme="light_colorblind". The only way you can get a light theme in dark mode is using auto.

Am I missing something?

Copy link
Member

Choose a reason for hiding this comment

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

What is added is a line I think will never execute. As far as I see you can't get a combination of data-color-mode="dark" and a light theme like data-dark-theme="light_colorblind". The only way you can get a light theme in dark mode is using auto.

Am I missing something?

Yeah that's correct, in dotcom ui, when selecting single mode, you'll only get the corresponding data-color-mode to the stored theme value. The mismatch only happens if someone selects the opposite theme for there light/dark auto themes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removing this change for now, and we can bring it back later if we find it necessary in dotcom!

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, you're right. I got confused because that combination is technically possible in code. I didn't realize dotcom prevents that from happening

selectorLight: `[data-color-mode="auto"][data-light-theme="${themeName}"]`,
selectorDark: `[data-color-mode="auto"][data-dark-theme="${themeName}"]`,
}
Expand Down
2 changes: 1 addition & 1 deletion src/tokens/base/color/light/light.json5
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
base: {
color: {
black: {
$value: '#1b1f24',
$value: '#1f2328',
$type: 'color',
},
transparent: {
Expand Down
2 changes: 1 addition & 1 deletion src/tokens/functional/border/dark.json5
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
outline: {
focus: {
$value: {
color: '{color.focus}',
color: '{focus.outlineColor}',
style: 'solid',
width: '1px',
},
Expand Down
6 changes: 3 additions & 3 deletions src/tokens/functional/border/light.json5
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
{
outline: {
focus: {
focus: {
outline: {
$value: {
color: '{color.focus}',
color: '{focus.outlineColor}',
style: 'solid',
width: '1px',
},
Expand Down
74 changes: 0 additions & 74 deletions src/tokens/functional/color/dark/ansi-dark.json5

This file was deleted.

242 changes: 242 additions & 0 deletions src/tokens/functional/color/dark/app-dark.json5
Original file line number Diff line number Diff line change
@@ -0,0 +1,242 @@
{
topicTag: {
borderColor: {
$value: '{base.color.transparent}',
$type: 'color',
},
},
highlight: {
neutral: {
bgColor: {
$value: '{base.color.yellow.3}',
$type: 'color',
alpha: 0.4,
},
},
},
page: {
header: {
bgColor: {
$value: '{bgColor.default}',
$type: 'color',
},
},
},
diffBlob: {
addition: {
fgColor: {
text: {
$value: '{fgColor.default}',
$type: 'color',
},
num: {
$value: '{fgColor.default}',
$type: 'color',
},
},
bgColor: {
num: {
$value: '{base.color.green.3}',
$type: 'color',
alpha: 0.3,
},
line: {
$value: '{base.color.green.4}',
$type: 'color',
alpha: 0.15,
mix: null,
},
word: {
$value: '{base.color.green.4}',
$type: 'color',
alpha: 0.4,
},
},
},
deletion: {
fgColor: {
text: {
$value: '{fgColor.default}',
$type: 'color',
},
num: {
$value: '{fgColor.default}',
$type: 'color',
},
},
bgColor: {
num: {
$value: '{base.color.red.4}',
$type: 'color',
alpha: 0.3,
},
line: {
$value: '{base.color.red.4}',
$type: 'color',
alpha: 0.15,
mix: null,
},
word: {
$value: '{base.color.red.4}',
$type: 'color',
alpha: 0.4,
},
},
},
hunk: {
bgColor: {
num: {
$value: '{borderColor.accent.muted}',
$type: 'color',
},
},
},
expander: {
iconColor: {
$value: '{fgColor.muted}',
$type: 'color',
},
},
},
diffStat: {
addition: {
bgColor: {
$value: '{bgColor.success.emphasis}',
$type: 'color',
},
},
},
codeMirror: {
fgColor: {
$value: '{fgColor.default}',
$type: 'color',
},
bgColor: {
$value: '{bgColor.default}',
$type: 'color',
},
gutters: {
bgColor: {
$value: '{bgColor.default}',
$type: 'color',
},
},
gutterMarker: {
fgColor: {
default: {
$value: '{bgColor.default}',
$type: 'color',
},
muted: {
$value: '{fgColor.muted}',
$type: 'color',
},
},
},
lineNumber: {
fgColor: {
$value: '{fgColor.muted}',
$type: 'color',
},
},
cursor: {
fgColor: {
$value: '{fgColor.default}',
$type: 'color',
},
},
selection: {
bgColor: {
$value: '{borderColor.accent.muted}',
$type: 'color',
},
},
activeline: {
bgColor: {
$value: '{bgColor.neutral.muted}',
$type: 'color',
},
},
matchingBracket: {
fgColor: {
$value: '{fgColor.default}',
$type: 'color',
},
},
lines: {
bgColor: {
$value: '{bgColor.default}',
$type: 'color',
},
},
syntax: {
fgColor: {
comment: {
$value: '{base.color.gray.3}',
$type: 'color',
},
constant: {
$value: '{base.color.blue.2}',
$type: 'color',
},
entity: {
$value: '{base.color.purple.2}',
$type: 'color',
},
keyword: {
$value: '{base.color.red.3}',
$type: 'color',
},
storage: {
$value: '{base.color.red.3}',
$type: 'color',
},
string: {
$value: '{base.color.blue.1}',
$type: 'color',
},
support: {
$value: '{base.color.blue.2}',
$type: 'color',
},
variable: {
$value: '{base.color.orange.2}',
$type: 'color',
},
},
},
},
header: {
fgColor: {
default: {
$value: '{fgColor.onEmphasis}',
$type: 'color',
alpha: 0.7,
},
logo: {
$value: '{fgColor.onEmphasis}',
$type: 'color',
},
},
bgColor: {
$value: '{base.color.gray.9}',
$type: 'color',
},
borderColor: {
divider: {
$value: '{base.color.gray.3}',
$type: 'color',
},
},
},
headerSearch: {
bgColor: {
$value: '{base.color.gray.9}',
$type: 'color',
},
borderColor: {
$value: '{base.color.gray.6}',
$type: 'color',
},
},
}
Loading