Skip to content

Conversation

@siddharthkp
Copy link
Member

@siddharthkp siddharthkp commented May 1, 2023

vertical panels of different themes, inside which a text string adapts its foreground and border color based on the theme

There are a couple things to note in this implementation:

  1. temporary dev dependency on primer/css

    With primer/primitives@8 just around the corner, we are using primitives v8 in the component with a fallback on primitives v7

    For v7, css variables for primitives are shipped in primer/css, which is why we have a devDependency on primer/css to test in storybook. This is only till primitives v8 are shipped in dotcom (~ 1-2 weeks), we can remove primer/css from our setup after that.

  2. data attributes added to BaseStyles
    color primitives change value color modes, this works by setting data attributes on the root of your application. See documentation.

    With primer/react, this should be taken care by ThemeProvider, but because ThemeProvider does not render any wrapper elements, I have chosen to add them to BaseStyles instead, which is used along with ThemeProvider (including in dotcom)

    This includes mapping the additional color modes for primer/react to primer/primitives (day → light, night → dark). We should look into deprecating these in the future!

@changeset-bot

This comment was marked as resolved.

@siddharthkp siddharthkp self-assigned this May 1, 2023
@siddharthkp siddharthkp added the skip changeset This change does not need a changelog label May 1, 2023
@siddharthkp siddharthkp changed the base branch from main to setup-for-css May 1, 2023 14:10
@siddharthkp siddharthkp requested a review from langermank May 1, 2023 14:33
import {withThemeProvider, withSurroundingElements, toolbarTypes} from '../src/utils/story-helpers'
import {PrimerBreakpoints} from '../src/utils/layout'

import '@primer/css/dist/primitives.css'
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a little concerned about add a dep for PCSS even though I know this is experimental. I think it makes more sense to just go ahead and start using the new tokens. The size tokens (imported here through primitives.css) will be rolled out to dotcom this week or next (like removing the feature flag) and the colors will start to be tested soon. Since this is a test, I think just go ahead and use the new color tokens!

Here are the imports (but from dist of course) https://github.com/primer/primitives/blob/main/docs/storybook/.storybook/preview.css

I'll be cutting a new release probably this week

Copy link
Member Author

@siddharthkp siddharthkp May 8, 2023

Choose a reason for hiding this comment

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

Made v8 the default now with a fallback on v7.

primer/css is now only a dev dependency to test the fallback. We can remove that as well as soon as v8 is rolled out in dotcom :)

* valid color modes for primer/primer: auto | day | night | light | dark
*/
type colorModesForPrimitives = 'auto' | 'light' | 'dark'
const primerColorModeToPrimitiveColorMode: {[key in ColorModeWithAuto]: colorModesForPrimitives} = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Im a little confused about "day" and "night". Can we standardize on just using light/dark/auto? https://github.com/primer/primitives/blob/main/docs/storybook/.storybook/preview.js#L35

Copy link
Member Author

@siddharthkp siddharthkp May 3, 2023

Choose a reason for hiding this comment

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

Would love to 😭 but we've been stuck with this for a while for backward compatibility. Created an issue to deprecate them in next major release

Base automatically changed from setup-for-css to main May 4, 2023 12:47
@github-actions
Copy link
Contributor

github-actions bot commented May 4, 2023

size-limit report 📦

Path Size
dist/browser.esm.js 121.64 KB (+0.06% 🔺)
dist/browser.umd.js 121.88 KB (+0.07% 🔺)

@siddharthkp siddharthkp temporarily deployed to github-pages May 4, 2023 13:09 — with GitHub Actions Inactive
@github-actions github-actions bot temporarily deployed to storybook-preview-3238 May 4, 2023 13:10 Inactive
@siddharthkp siddharthkp temporarily deployed to github-pages May 4, 2023 13:33 — with GitHub Actions Inactive
@github-actions github-actions bot temporarily deployed to storybook-preview-3238 May 4, 2023 13:33 Inactive
@siddharthkp
Copy link
Member Author

siddharthkp commented May 4, 2023

@langermank Need to confirm if this is a breaking change or a bug,

In primer/css/color-modes.css, [data-color-mode="light"][data-light-theme="dark"] is a valid combination (same for all dark themes)

see code on unpkg

screenshot, notice the border colors. (the body color comes from primer/css globals)

image

but in primer/primitives/tokens-next-private/css/, this isn't true:

see code on unpkg

screenshot, notice missing border color. (the body color is white, which is expected because we are not including primer/css globals here)

image

@siddharthkp siddharthkp temporarily deployed to github-pages May 4, 2023 14:12 — with GitHub Actions Inactive
@github-actions github-actions bot temporarily deployed to storybook-preview-3238 May 4, 2023 14:12 Inactive
@siddharthkp siddharthkp changed the title [WIP] Theming for css with primitives [WIP] Theming for css with primitives v8 May 4, 2023
@siddharthkp siddharthkp changed the title [WIP] Theming for css with primitives v8 [WIP] Theming for css with primitives v7 and v8 May 4, 2023
@siddharthkp siddharthkp changed the title [WIP] Theming for css with primitives v7 and v8 [WIP] Theming for css with primitives May 4, 2023
@siddharthkp siddharthkp changed the title [WIP] Theming for css with primitives Theming for css with primitives May 8, 2023
@siddharthkp siddharthkp requested a review from joshblack May 8, 2023 13:28
@siddharthkp siddharthkp marked this pull request as ready for review May 8, 2023 13:28
@siddharthkp siddharthkp requested a review from a team May 8, 2023 13:28
@siddharthkp siddharthkp marked this pull request as draft May 8, 2023 13:29
@github-actions github-actions bot temporarily deployed to storybook-preview-3238 May 8, 2023 13:34 Inactive
@siddharthkp siddharthkp marked this pull request as ready for review May 8, 2023 13:35
@siddharthkp siddharthkp temporarily deployed to github-pages May 8, 2023 13:40 — with GitHub Actions Inactive
@github-actions github-actions bot temporarily deployed to storybook-preview-3238 May 8, 2023 13:41 Inactive
@joshblack
Copy link
Member

@siddharthkp quick question, in the dark themes it seems like the border tokens are not being defined. Just wanted to check-in to see if this was intended with this setup or not 👀

Screenshot 2023-05-08 at 10 16 53 AM

It seems like potentially for these combos the custom properties are not being set at the theme provider level

Screenshot 2023-05-08 at 10 20 42 AM

@siddharthkp
Copy link
Member Author

@joshblack Yep! Noted that in #3238 (comment)

Seems like a bug (or a intentional breaking change) in primitives, talking to @langermank about it.

@lukasoppermann
Copy link
Contributor

In primer/css/color-modes.css, [data-color-mode="light"][data-light-theme="dark"] is a valid combination (same for all dark themes)

@siddharthkp this has been removed, as we figured that there is no way to get this combination in the wild. A few people checked it, but maybe we missed something. If we did, how can you get to this combination?

@siddharthkp
Copy link
Member Author

@siddharthkp this has been removed, as we figured that there is no way to get this combination in the wild.

That's perfect! Happy to drop support for it :)

@siddharthkp
Copy link
Member Author

siddharthkp commented May 9, 2023

For storybook, setting colorMode based on compatible scheme now. Looks much better:

vertical panels of different themes, inside which a text string adapts its foreground and border color based on the theme

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

react skip changeset This change does not need a changelog

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants