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
121 stars 88 forks source link

Card consolidation and updates #3875

Open navkaur76 opened 1 month ago

navkaur76 commented 1 month ago

Task 1 (design + dev):

Task 2 (design):

Problem:

Current use of accent is confusing. By default it is grey, but purpose of accent bar is to display color.

Should Cards be:

  1. Default = no accent, grey border
  2. Accent = color accent, grey border

Hoverable? 1) Remove it? (Is it confusing to have hover state but no click? was an MCP requirement) or... 2) Keep it? If hoverable is true, then the accent bar color is only be displayed on hover.

navkaur76 commented 1 month ago

26/07 - regroup meeting with Nav, Ben, Darrin, Zhihao + Josh.

Nav to pick back up figma work on 30/07 after AL. Zhihao has left some comments around component structure in figma, in the meantime.

origami-z commented 1 month ago

Aug 2nd - Code / doc update in Cortado

origami-z commented 1 month ago

Aug 5 - more Figma component work needed to address borderless snap issue

mark-tate commented 1 month ago

Cortado Goal: KIckoff required Ready For Dev by EOS

navkaur76 commented 1 month ago

Card token discussion notes:

Requirement (if Card is using selectable tokens):

To add a disabled token for interactable card (so the accent is blue, essentially treating disabled card as an alpha of default card).

Outcome:

Explore using actionable tokens for card, instead of selectable. Reasoning:

We would end up with: Static card, Link card and Interactable card (which covers both “actionable” and “selectable” interactions)

Question: do we want to consider a name change from “Interactable card” to “Actionable card”? We don’t use the term “interactable” elsewhere. OR, do we document what we mean by “interactable” – i.e., Interactable card is both actionable and selectable. This could be added as an entry in the site glossary page.

Other agenda items discussed:

pseys commented 1 month ago

Had a discussion with Nav today about actionable token use in Card. Nav has set-up time for us to look into this in more detail tomorrow as it's not as straight forward as first thought and we need to review our options.

origami-z commented 1 month ago

Aug 12 - discussed tokens on Friday

joshwooding commented 4 weeks ago

Aug 14 - Made token updates to card set. Waiting for review from @dplsek @pseys @origami-z + others

origami-z commented 3 weeks ago

Aug 16 - likely slip to next sprint

joshwooding commented 3 weeks ago

Aug 20 - Tokens agreed, but need updates to the library. @origami-z to look at the code updates.

pseys commented 3 weeks ago

The variable accent-background-disabled has been added to the Salt (Next) Style Library. The variable references --palette-accent-disabled

pseys commented 3 weeks ago

In Next I've added: accent-background-disabled that points to an existing palette token of --palette-accent-disabled

In Legacy we need to also add --palette-accent-disabled which points to --salt-color-blue-500-fade-background – It needs to be 'accent-disabled' because --palette-accent-background-disabled has previously been deprecated.

joshwooding commented 3 weeks ago

In Next I've added: accent-background-disabled that points to an existing palette token of --palette-accent-disabled

In Legacy we need to also add --palette-accent-disabled which points to --salt-color-blue-500-fade-background – It needs to be 'accent-disabled' because --palette-accent-background-disabled has previously been deprecated.

We've undeprecated tokens before (tertiary container tokens). Is the a reason accent-disabled is different?

pseys commented 3 weeks ago

We've undeprecated tokens before (tertiary container tokens). Is the a reason accent-disabled is different?

If we're OK undeprecating the token then let's do that rather than add more to the legacy palette. I'll change what I've done to use --palette-accent-background-disabled

Thanks Josh.

joshwooding commented 3 weeks ago

We've undeprecated tokens before (tertiary container tokens). Is the a reason accent-disabled is different?

If we're OK undeprecating the token then let's do that rather than add more to the legacy palette. I'll change what I've done to use --palette-accent-background-disabled

Thanks Josh.

Worthing confirming with @origami-z but we've done it before so we have precedence

origami-z commented 3 weeks ago

if that's the right naming, let's undeprecate i guess

origami-z commented 2 weeks ago

Aug 28 - Token used is blocked by Button update (extend a sprint)

mark-tate commented 1 week ago

Espresso Goal to be released by September 13th