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

Add missing semantic tokens for critical & accent buttons #312

Open
nadonomy opened this issue Feb 6, 2024 · 4 comments
Open

Add missing semantic tokens for critical & accent buttons #312

nadonomy opened this issue Feb 6, 2024 · 4 comments

Comments

@nadonomy
Copy link
Contributor

nadonomy commented Feb 6, 2024

We're missing:

  1. A token for critical/pressed buttons
  2. Accent button tokens

However when I went to add I noticed inconsistencies with how we've defined tokens:

Screenshot 2024-02-06 at 12 06 57

You can see we have:

bg/action/primary/rest
bg/action/primary/hovered
bg/action/primary/pressed
bg/action/secondary/rest
bg/action/secondary/hovered
bg/action/secondary/pressed

bg/critical/primary
bg/critical/hovered

when I think instead we should have:

bg/action/primary/rest
bg/action/primary/hovered
bg/action/primary/pressed
bg/action/secondary/rest
bg/action/secondary/hovered
bg/action/secondary/pressed
bg/action/critical/rest
bg/action/critical/hovered
bg/action/critical/pressed
bg/action/accent/rest
bg/action/accent/hovered
bg/action/accent/pressed

Open questions:

  1. Does anyone disagree with unifying the action tokens as above?
  2. Should I deprecate the misplaced bg/critical tokens in Token Studio? If so, will that introduce any breaking changes?
@pixlwave
Copy link
Member

pixlwave commented Feb 6, 2024

For 1. Would it be fair to say we might have a critical background for something that isn't a button?

@nadonomy
Copy link
Contributor Author

nadonomy commented Feb 6, 2024

For 1. Would it be fair to say we might have a critical background for something that isn't a button?

Yep! I didn't note them in this issue but we also have:

bg/critical/subtle
bg/critical/subtle-hovered
border/critical/primary
border/critical/hovered
border/critical/subtle

which we use for stuff like critical alerts.

@nadonomy
Copy link
Contributor Author

nadonomy commented Feb 6, 2024

Separately we also have:

bg/success/subtle
bg/info/subtle

So instead, I think we should unify overall to this and then re-map:

bg/action/primary/rest
bg/action/primary/hovered
bg/action/primary/pressed
bg/action/secondary/rest
bg/action/secondary/hovered
bg/action/secondary/pressed
bg/action/critical/rest
bg/action/critical/hovered
bg/action/critical/pressed
bg/action/accent/rest
bg/action/accent/hovered
bg/action/accent/pressed

bg/critical/subtle
bg/success/subtle
bg/info/subtle

this would mean deprecating bg/critical/subtle-hovered too, but it's the only subtle-hovered token and intuitively I'm not sure where we'd need it.

@pixlwave
Copy link
Member

pixlwave commented Feb 7, 2024

I guess I'm not sure I see the benefit of defining a difference between "critical" and "critical as an action". E.g. we have text/critical/primary would we additionally need a text/action/critical/primary for when the text is a secondary/tertiary button?

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

No branches or pull requests

2 participants