Skip to content
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

Card consolidation and updates #3875

Closed
navkaur76 opened this issue Jul 22, 2024 · 36 comments
Closed

Card consolidation and updates #3875

navkaur76 opened this issue Jul 22, 2024 · 36 comments

Comments

@navkaur76
Copy link
Contributor

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
Copy link
Contributor Author

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

  • Confirmed card states + characteristics
  • Accent display is optional for all variants.
  • Rounded corners - some further figma + code work to be done to address the structure. Exploring accent inset within border.

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
Copy link
Contributor

Aug 2nd - Code / doc update in Cortado

@origami-z
Copy link
Contributor

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

@mark-tate
Copy link
Contributor

mark-tate commented Aug 5, 2024

Cortado Goal:
KIckoff required
Ready For Dev by EOS

@navkaur76
Copy link
Contributor Author

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:

  • Actionable/background contains colours for all card accent states (apart from a new token that will need adding for “active disabled”).
  • Instead of adding action states to accent (which is only treated as a static border + background colour) - we’re saying it is an interactive card, therefore an actionable item. This semantically aligns to actionable tokens.
  • Provides continuity with button which is also an actionable item.
  • Q: Is Card then considered an actionable or selectable component? A: We can take precedent for this from toggle button group, which uses actionable tokens but is a component you make a selection from. Similarly, we’re saying interactable card is using actionable tokens, but can be considered selectable when used within a group (i.e., selectable pattern). Either way, visually card wouldn’t change and would use actionable tokens across both use cases.
  • (Follow up note: does toggle button group also need an active-disabled token?)

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:

  • When wiring up card to category, the category colours will require equivalent disabled tokens
  • Figma borderless issue – Nav to first make token updates and then address the borderless behaviour as a secondary problem. Follow up call on this pending.

@pseys
Copy link
Contributor

pseys commented Aug 8, 2024

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
Copy link
Contributor

Aug 12 - discussed tokens on Friday

@joshwooding
Copy link
Contributor

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

@origami-z
Copy link
Contributor

Aug 16 - likely slip to next sprint

@joshwooding
Copy link
Contributor

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

@pseys
Copy link
Contributor

pseys commented Aug 20, 2024

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

@pseys
Copy link
Contributor

pseys commented Aug 20, 2024

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
Copy link
Contributor

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
Copy link
Contributor

pseys commented Aug 20, 2024

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
Copy link
Contributor

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
Copy link
Contributor

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

@origami-z
Copy link
Contributor

origami-z commented Aug 28, 2024

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

@mark-tate
Copy link
Contributor

Espresso Goal to be released by September 13th

@mark-tate
Copy link
Contributor

Frappe Goal: Code & Design in sync and published to Core

@origami-z
Copy link
Contributor

@origami-z
Copy link
Contributor

PR #4111 ready for design review

@mark-tate
Copy link
Contributor

Frappe Goal: released by EOS

@origami-z
Copy link
Contributor

Sep 17 - code changed, needs design review, especially around Salt current token on hover/active (which doesn't have border)

@navkaur76
Copy link
Contributor Author

Sep 20 - meeting with @pseys on the Salt (current) tokens for the hover and active borders. Paul to update the actionable/accented/border tokens in current to change those two borders from transparent to blue 700 + blue 500. Will also result in a small border update to button too.

@pseys
Copy link
Contributor

pseys commented Sep 20, 2024

As a follow up to Nav's comment, in order to accommodate Card having an accented border on hover and active we need to update the following two characteristics in Salt 'current' to use two new palette tokens:

Characteristic Palette Light mode Dark mode
actionable-accented-borderColor-hover palette-interact-cta-border-hover (new) blue-500 blue-500
actionable-accented-borderColor-active palette-interact-cta-border-active (new) blue-700 blue-700

As Nav mentioned, this will result in the bordered button having a coloured border on hover and active, however, this will match the background color so won't be visible to users.

If this is an acceptable solution from a technical perspective I'll update the Figma files accordingly.

cc @origami-z

@origami-z
Copy link
Contributor

Shouldn't be a problem, preview after the change button and accented card

@origami-z
Copy link
Contributor

Sep 23 - Code change ready in #4111, needs design review with 3 card types

@mark-tate
Copy link
Contributor

Meeting oni Tue, update on Wed, by end of sprint expected to be complete

@mark-tate
Copy link
Contributor

Galao Goal: meeting 30th Sept / by end of wk1 design complete

@pseys
Copy link
Contributor

pseys commented Oct 7, 2024

Figma only variable has been added to unblock work. FYI @navkaur76

@mark-tate
Copy link
Contributor

part of next figma release

@origami-z
Copy link
Contributor

code complete, in release from #4179

@pseys
Copy link
Contributor

pseys commented Oct 10, 2024

actionable-accented-border-hover and actionable-accented-border-active colors updated in Figma light and dark mode libraries. cc @navkaur76

@bhoppers2008 bhoppers2008 assigned pseys and yunjungyeh and unassigned navkaur76 Oct 14, 2024
@mark-tate
Copy link
Contributor

Latte goal: Design side ready to publish, requires Figma variable.
(Code released already)

@pseys
Copy link
Contributor

pseys commented Oct 15, 2024

Bug raised with corner radius not adhering to style option choices.

@yunjungyeh
Copy link
Contributor

16 Oct - in Figma: updated to the new variables, metadata, component description and some fixes, and merged!

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

No branches or pull requests

6 participants