-
Notifications
You must be signed in to change notification settings - Fork 48
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
[Token update]: Shadows #359
Comments
@lukasoppermann what do you think about mocking up how this might impact various Primer components that use shadows? It's hard to conceptualize if this change will be okay without "seeing" it.. or maybe even just comparing existing shadow values to what you're proposing in code. Our existing shadow tokens are a bit broken in that they contain both size and color, so I am 100% in favor of breaking that out into different token types 🙌 |
Here is a branch of react storybook that uses Here are a view screens from figma (before / after, using |
Thank you for the visuals! I feel like this is probably a good change to make. I want to make sure other folks can weigh in so tagging @simurai and @vdepizzol Vini is on vacation, so let's make sure to loop him in and be open to making adjustments later in case we move forward before he returns |
If we think this can go, I can create a PR and we can plan for it in the style dictionary rework, and we can keep the PR open until @vdepizzol had time to look over it. 👍 |
I can't really see a big difference, but there is probably something about blending in with the rest of Primer's grays. They all have a small tint of "blue", so using Also, using So, yes, maybe good to have @vdepizzol chime in before making changes to it. 👍 |
We could also use a tinted color like
That is a very interesting thought. It would only apply for big shadows, like dialog. But we use all different kind of grays, so I am not sure this is actually the case. However if this is the case, we could probably use a black |
👋 Hello @lukasoppermann! Catching up to this now.
I think it's essential to keep shadows themeable. A dark theme may take advantage of a stronger, less opaque shadow to compensate with the rest of the screen. A high contrast theme may increase the shadow or even render it as a thick outline to separate the element or overlay from the rest of the page.
Unlike typography and spacing, the choice of how the shadow is rendered should depend on its environment. Take a look at our current --some-shadow-variable: 0 0 0 1px #444c56, 0 16px 32px rgba(28,33,40,0.85) The shadow itself has a lighter outline ring, heping to elevate the surface in this dark environment. That line is part of the shadow, since we don't want to use borders that take space, especially at the viewport edges. In high contrast, right now we don't do anything special with shadows, but I expect we should support making them, hm, higher contrast: --some-shadow-variable: 0 0 0 2px #000, 0 4px 16px #0004
We're already using these multi-layer shadows in some implementations (
I'm not sure I'm understanding what you're saying here, but I expect any theme to choose how to compose their shadow, and not be limited by a predefined structure that expects a certain shadow composition. As in the examples above, a theme may need a composition of 2 shadows, while another may benefit from 5 or more. I'd like to make sure our design tokens enable such a flexible theming capability.
I wonder if this is a naming problem after all? As far as I know our themes live in the Regarding Primer grays and using a pitch As a side note, these shadow tests should be done with multiple backgrounds. The end result may look different if a dialog per example appears on top of a dark/black box such as our Actions' log view. ;-). |
Hey @vdepizzol thanks for the detailed thoughts. 🙏 Makes sense to me. I was wondering if we are planning on using more of complex shadows.
So this would be correct for the shadow color, but not the shadow itself. But I agree that shadows may need custom implementations per theme. However this can be done by bringing themability into the shadow folder. If we in the future need to make even more things themable (e.g. typography or size) we should rethink how we organize and build tokens, but I think for now this would add to much complexity. So let's end this issue with the following takeaways now:
I'll build this into the system. |
Taking stock of shadows that are currently in the code// app_dark.ts
inputShadow: (theme: any) => `0 0 0 1px ${get('border.default')(theme)}`,
dropdownShadow: alpha(get('scale.black'), 0.3),
// global_dark.ts
shadow: {
small: '0 0 transparent',
medium: (theme: any) => `0 3px 6px ${get('scale.black')(theme)}`,
large: (theme: any) => `0 8px 24px ${get('scale.black')(theme)}`,
extraLarge: (theme: any) => `0 12px 48px ${get('scale.black')(theme)}`
},
primer: {
shadow: {
highlight: '0 0 transparent', // top highlight
inset: '0 0 transparent', // top inner shadow
focus: (theme: any) => `0 0 0 3px ${get('scale.blue.8')(theme)}`,
}
},
// component_dark.ts
avatar: {
childShadow: (theme: any) => `-2px -2px 0 ${get('scale.gray.9')(theme)}`
},
overlay: {
shadow: (theme: any) => `0 0 0 1px ${get('scale.gray.6')(theme)}, 0 16px 32px ${alpha(get('scale.black'), 0.85)(theme)}`,
},
btn: {
shadow: '0 0 transparent',
insetShadow: '0 0 transparent',
focusShadow: (theme: any) => `0 0 0 3px ${alpha(get('scale.gray.3'), 0.3)(theme)}`,
shadowActive: (theme: any) => `inset 0 0.15em 0.3em ${alpha(get('scale.black'), 0.15)(theme)}`,
shadowInputFocus: (theme: any) => `0 0 0 0.2em ${alpha(get('scale.blue.5'), 0.3)(theme)}`,
// primary button
primary: {
shadow: '0 0 transparent',
insetShadow: '0 0 transparent',
selectedShadow: '0 0 transparent',
focusShadow: (theme: any) => `0 0 0 3px ${alpha('#2ea44f', 0.4)(theme)}`,
},
// outline button
outline: {
hoverShadow: (theme: any) => `0 1px 0 ${alpha(get('scale.black'), 0.1)(theme)}`,
hoverInsetShadow: (theme: any) => `inset 0 1px 0 ${alpha(get('scale.white'), 0.03)(theme)}`,
selectedShadow: '0 0 transparent',
focusShadow: (theme: any) => `0 0 0 3px ${alpha(get('scale.blue.6'), 0.4)(theme)}`,
},
// danger button
danger: {
hoverShadow: '0 0 transparent',
hoverInsetShadow: '0 0 transparent',
selectedShadow: '0 0 transparent',
focusShadow: (theme: any) => `0 0 0 3px ${alpha(get('scale.red.4'), 0.4)(theme)}`,
}
} Focus shadows will be moved to an outline style.
|
Just a quick note that we removed those focus shadows 😄 I have an open PR removing them in primitives |
@langermank But we will need at least one focus shadow, right? We need a focus. Did you remove all? |
@lukasoppermann its not a shadow anymore, its actually an |
I think we need 3 types of shadows in different intensities: Inset shadows
Resting shadows
For this I think we only need one or two shadows, as hour elements are technically mostly the "same size / height towards the user" thinking in 3D space and so they should cast a similar shadow. Floating shadows
We need a couple different shadows in this categories for different elevation levels. |
I noticed that shadows are currently part of colors. This makes no sense as the token actually is a css shadow with offset, etc. and not just the color.
At the moment we also don't really have important reasons to make shadows adjust to color modes. In dark shadows are invisible and in light we currently keep shadows the same.
Atm I added some new shadow-colors that are used in the new shadows in #353
However I think a better approach would do not use the color scale and just use
#000000
black with different opacities. And keep it the same for all color modes. This would not change much for the visual, but would allow us to keep shadows and their colors in one file without depending on color modes.Looking at some of the shadows that e.g. @vdepizzol added we probably want to move into multi/layer shadows in the future. They look more polished and also more natural as this is how light behaves in the real world.
Using color tokens would mean we would need a lot of color tokens (one per opacity). If we end up with just 5 or 10 that would be no problem. However if we have more custom alpha values, including colors into the shadow files would allow us to avoid tokens and directly define colors inside the shadow token.
CC: @langermank @josepmartins @rezrah any objections?
The text was updated successfully, but these errors were encountered: