jpmorganchase / salt-ds

React UI components built with a focus on accessibility, customization and ease-of-use
https://www.saltdesignsystem.com
Apache License 2.0
112 stars 86 forks source link

Theme Next: Tertiary background #3330

Closed origami-z closed 4 days ago

origami-z commented 3 months ago

Salt 1 colors defined in Figma: https://www.figma.com/design/xc2MLpc4yEfJghHbh1YjMQ/Salt-1.0-tertiary-background-colors?node-id=0-1&t=JdwquPPA8iXgtsTt-1 Salt next token already in code:

https://github.com/jpmorganchase/salt-ds/blob/dde10d12dd3dff3b332ec2de701e53585e6a25c9/packages/theme/css/palette/background-next.css#L6

https://github.com/jpmorganchase/salt-ds/blob/dde10d12dd3dff3b332ec2de701e53585e6a25c9/packages/theme/css/palette/background-next.css#L14

### Tasks
- [x] New design token (salt 1 & 2)
- [ ] Relevant component updated (e.g. panel)
- [ ] Figma library updated
mark-tate commented 3 weeks ago

Goal: release changes + new tokens and add new variant

pseys commented 2 weeks ago

@origami-z @dplsek is the intention at this point to just release the tertiary backgrounds or does this issue also include the component updates in code and Figma? I can see it included in the Tasks, but there's no clarification of which components this includes.

pseys commented 2 weeks ago

@origami-z @dplsek to confirm, have I got this correct?

Salt Current

Salt Next

This is correct.

origami-z commented 2 weeks ago

Salt Current

  • light: gray-30
  • dark: gray-500

Salt Next

  • light: limestone
  • dark: leather

Looks good to me

origami-z commented 2 weeks ago

@origami-z @dplsek is the intention at this point to just release the tertiary backgrounds or does this issue also include the component updates in code and Figma? I can see it included in the Tasks, but there's no clarification of which components this includes.

Component work starting as well, #3452, Card and Panel variants

pseys commented 2 weeks ago

Salt Current

  • light: gray-30
  • dark: gray-500

Salt Next

  • light: limestone
  • dark: leather

Looks good to me

Thanks @origami-z – the values are in Figma Next but not in Current, I'll add them now.

pseys commented 2 weeks ago

Salt Current

  • light: gray-30
  • dark: gray-500

Looks good to me

@origami-z have you referenced gray-30 and gray-500 directly or have you added palette tokens?

origami-z commented 2 weeks ago

@origami-z have you referenced gray-30 and gray-500 directly or have you added palette tokens?

Haven't added. But will imagine being similar structure of primary/secondary ones. Pointing to palette layer with similar naming.

https://github.com/jpmorganchase/salt-ds/blob/9d6919df329931a8c5bc82b18120c0d4977da103/packages/theme/css/characteristics/container.css#L4-L12

Also needing border tokens for Card as well. I believe @dplsek checked using same border of primary/secondary.

pseys commented 2 weeks ago

I've added the following into Salt Current in Figma:

characteristic palette Light Dark
container-tertiary-background palette-neutral-tertiary-background color-gray-30 color-gray-500
container-tertiary-background-disabled palette-neutral-tertiary-background-disabled color-gray-30 40% color-gray-500 40%
container-tertiary-border palette-neutral-tertiary-border color-gray-50 color-gray-300
container-tertiary-border-disabled palette-neutral-tertiary-border-disabed color-gray-50 40% color-gray-300 40%

These are all ready for release.

pseys commented 2 weeks ago

I've added the following into Salt Next in Figma:

characteristic palette Light Dark
container-tertiary-background palette-background-tertiary color-limestone color-leather
container-tertiary-background-disabled palette-background-tertiary-disabled color-limestone 40% color-leather 40%
container-tertiary-border palette-alpha color-black 30% color-white 30%
container-tertiary-border-disabled palette-alpha-weaker color-black 15% color-white 15%

These are all ready for release.

pseys commented 2 weeks ago

@origami-z @dplsek if you're both happy that these are all the tokens we need I'll publish them so that the component updates can be completed.

dplsek commented 2 weeks ago

@pseys i'm confused on the tertiary-border alpha value... it looks like primary, secondary, and tertiary are all set to 30%, is that correct?

mark-tate commented 1 week ago

@dplsek to review with a view of it going into next release

dplsek commented 1 week ago

These background colors are correct.

However, I'm curious about the border tokens also included here. if they're all the same, do we need individual tokens per background type?

pseys commented 6 days ago

Hey @dplsek, sorry I missed the previous comment. I can't remember exactly when it happened, but either end of last year or earlier this year the primary and secondary container border was made the same color. I wasn't involved with the decision so can't justify the reasoning. When creating the tertiary styles the background has been added to match.

The need for separate tokens means that, similar to the work we're doing on Button, each border can be targeted separately if needed for other themes. Whether or not they're different values in our Salt themes is our decision to make.

dplsek commented 6 days ago

Thanks, @pseys. Does anyone @bhoppers2008 @joshwooding @origami-z recollect our reason for this? The only thing I can think is that we wanted to associate primary/secondary/tertiary to the fill color alone, therefore we kept the 30% alpha border value the same across the board even though we have individual tokens for each. Assuming these are all decorative to begin with, i think there should be three tokens shared for the set – primary 30%, secondary 20%, tertiary 10% – that can be used on any panel, ie a consumer can apply a tertiary border on a secondary panel and achieve a more subtle look. And though different underlying alpha values, It would also align with the way we handle separators.

At minimum, it's confusing when using our Figma libraries because the tokens are not tied to the container fills. I needed a subtle border last night for the CC designs and when searching 'container border' I clearly see the three options, but when applying them, no visual change.

What are our thoughts on this?

joshwooding commented 6 days ago

@dplsek comments:

Primary and secondary text passes on both tertiary backgrounds (in legacy)

image

They fail/pass the same way with primary and secondary backgrounds as well

image

dplsek commented 5 days ago

@bhoppers2008 @mark-tate the comment i made yesterday about the tertiary backgrounds in Salt legacy is simply that primary buttons do not pass ADA when appearing on them in either mode, and CTAs don't pass when appearing on dark tertiary. However, this is a rather moot point as it is consistent with the results of the primary and secondary backgrounds in legacy (snippets above). That said, it's not a background issue, it's a button issue. Short of not doing it at all, there is no alternative. If there are no objections, I'm going to approve this.

bhoppers2008 commented 5 days ago

The requirement for button to have contrast against the background is a new enhancement for Salt. The distinguishing factor on legacy is the upper case... but in truth the label just needs to have contrast against the button background. We can merge this update. If it helps we can point towards that criteria... it's here.

mark-tate commented 5 days ago

Assuming this solves itself once we move to Next but in the meantime, do we create a defect on it, to acknowledge the issue in Legacy.

bhoppers2008 commented 5 days ago

It is resolved in Next, yep... but it's not actually a default. Best would be to add that WCAG reference to docs (either on site or some other relevant place, e.g., migration guide) and explain that the Next theme adds additional affordances to enhance experience.