primer / primitives

Color, typography, and spacing primitives in json.
https://primer.style/primitives
MIT License
306 stars 44 forks source link

[Token update]: Shadows #359

Closed lukasoppermann closed 11 months ago

lukasoppermann commented 2 years ago

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?

langermank commented 2 years ago

@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 πŸ™Œ

lukasoppermann commented 2 years ago

Here is a branch of react storybook that uses base.scale.black. However you have to run it locally as the build fails.

Here are a view screens from figma (before / after, using base.scale.black) With slight adjustments to opacity it is basically impossible to see a difference.

Screenshot 2022-10-07 at 15 45 51 Screenshot 2022-10-07 at 15 45 29 Screenshot 2022-10-07 at 15 45 13 Screenshot 2022-10-07 at 15 45 23
lukasoppermann commented 2 years ago

And again, this time:

current | base.scale.black | #000000

Screenshot 2022-10-07 at 15 56 36 Screenshot 2022-10-07 at 15 56 27
langermank commented 2 years ago

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 ☺️

lukasoppermann commented 2 years ago

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. πŸ‘

simurai commented 2 years ago

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 #000000 would make the shadows a bit warmer. At least in theory? πŸ€”

Also, using gray.4 but with 20% transparency vs like 8% with #000000 makes the shadow look not too strong but still cover more of what's underneath. Not sure if that's the thinking behind using gray.4?

So, yes, maybe good to have @vdepizzol chime in before making changes to it. πŸ‘

lukasoppermann commented 2 years ago

We could also use a tinted color like #00000F but I am not sure it actually makes a visible difference. Especially since out shadows are very subtle.

Also, using gray.4 but with 20% transparency vs like 8% with #000000 makes the shadow look not too strong but still cover more of what's underneath. Not sure if that's the thinking behind using gray.4?

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 #00000F for shadows and a grey for shadows were we want this effect. Or we use this grey everywhere.

vdepizzol commented 1 year ago

:wave: Hello @lukasoppermann! Catching up to this now.

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.

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.

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.

Unlike typography and spacing, the choice of how the shadow is rendered should depend on its environment. Take a look at our current Dark dimmed shadow in the Dialog component:

--some-shadow-variable: 0 0 0 1px #444c56, 0 16px 32px rgba(28,33,40,0.85)

Screen Shot

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

Screen Shot

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.

We're already using these multi-layer shadows in some implementations (Overlay patterns in both Primer CSS and React). But yes, these haven't replaced our generic small | medium large shadows.

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.

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 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.

I wonder if this is a naming problem after all? As far as I know our themes live in the color folder, but is this semantically correct if we consider that shadows are also meant to be themeable?


Regarding Primer grays and using a pitch #000000 black, I think it's a very small change but I'd prefer not to mix different tones of gray. Primer has a blueish tone and a truly neutral gray may look perceptively out of tune depending on the monitor.

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. ;-).

lukasoppermann commented 1 year ago

Hey @vdepizzol thanks for the detailed thoughts. πŸ™

Makes sense to me. I was wondering if we are planning on using more of complex shadows. I already implemented both systems, so having themable shadows is no problem. πŸ‘

I wonder if this is a naming problem after all? As far as I know our themes live in the color folder, but is this semantically correct if we consider that shadows are also meant to be themeable?

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:

  1. We keep shadows themable
  2. We compose shadows with the theme so that we can access theme colors at build time

I'll build this into the system.

lukasoppermann commented 1 year ago

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. This means we actually only have the following shadows:

Shadow usage count used in comment
transparent / none 11 buttons, primer shadows, shadow.small
1px outline 2 inputShadow, part of overlay.shadow using different colors
0 3px 6px 1 Shadow.medium
0 8px 24px 1 Shadow.large
0 12px 48px 1 Shadow.extraLarge
-2px -2px 0 1 Avatar
0 0 0 1px, 0 16px 32px 1 Overlay two shadows
inset 0 0.15em 0.3em 1 btn.shadowActive
0 1px 0 1 btn.outline.hoverShadow
inset 0 1px 0 1 btn.outline.hoverInsetShadow
~focusShadow~ ~5~ ~buttons, primer.shadow.focus~ ~same shadow but different color in every use~
langermank commented 1 year ago

Just a quick note that we removed those focus shadows πŸ˜„ I have an open PR removing them in primitives

lukasoppermann commented 1 year ago

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?

langermank commented 1 year ago

@lukasoppermann its not a shadow anymore, its actually an outline (like a border) πŸ˜„ Brand uses shadows for focus I believe, but they would use their own functional tokens for that.

lukasoppermann commented 1 year ago

I think we need 3 types of shadows in different intensities:

Inset shadows

Elements that are below the canvas like some inputs. We are currently using inset shadows in some inputs like the search input. I don't know if this is something we want to keep, but if we do, we need one inset shadow for elements like this.

Resting shadows

For elements that are resting on the canvas like buttons or dropdowns.

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

For elements that are layered on top of the canvas like sidebars or overlays

We need a couple different shadows in this categories for different elevation levels. A sticky header may have a different shadow than an open dropdown or a sidebar. An overlay has probably the strongest shadow as it floats above all the content.