Skip to content

feat: implement ActionButton core tokens#1430

Merged
pfulton merged 28 commits into
lazd/CSS-75-core-tokens-prefrom
lazd/CSS-98-actionbutton-core-tokens
May 16, 2022
Merged

feat: implement ActionButton core tokens#1430
pfulton merged 28 commits into
lazd/CSS-75-core-tokens-prefrom
lazd/CSS-98-actionbutton-core-tokens

Conversation

@lazd
Copy link
Copy Markdown
Member

@lazd lazd commented Apr 25, 2022

Description

This implements core tokens in ActionButton. This is a BREAKING CHANGE because a class is now required on ActionButton's icon.

To-do list

  • Implement ActionButton's dimensions with core tokens
  • Implement ActionButton's colors with core tokens
  • Validate custom properties API approach with team
  • Document custom properties API
    • [ ] Explore adding a copy Button for custom property API
  • Validate implementation with SWC
  • Validate custom property API with SWC
  • Implement WHCM support
  • Validate WHCM support approach with @jnurthen
  • Document WHCM process (testing, implementation)
  • Design verification deployed here

lazd added 2 commits April 26, 2022 11:26
BREAKING CHANGE: .spectrum-ActionButton-icon is now required
they now live in components/coretokens/custom.css
@lazd lazd force-pushed the lazd/CSS-98-actionbutton-core-tokens branch from 7646821 to 3be0152 Compare April 26, 2022 18:26
@jnurthen
Copy link
Copy Markdown
Contributor

won't this break High Contrast mode?

@lazd
Copy link
Copy Markdown
Member Author

lazd commented Apr 27, 2022

@jnurthen yes, it will have to be re-implemented with the new approach. I'll take a stab at doing so in this PR.

@jnurthen
Copy link
Copy Markdown
Contributor

@jnurthen yes, it will have to be re-implemented with the new approach. I'll take a stab at doing so in this PR.

I have a draft PR (#1431) with a bunch of high contrast changes including some for actionbutton which are much better than the original WHCM changes. Would be a better starting point to re-implement than the original version.

@lazd
Copy link
Copy Markdown
Member Author

lazd commented Apr 27, 2022

@jnurthen ok, I will pull changes from your draft PR. I already fixed it up with the existing WHCM, just one thing I had a Q about.

@jnurthen
Copy link
Copy Markdown
Contributor

@lazd We should also validate whether core tokens will resolve adobe/spectrum-web-components#2229

@Westbrook
Copy link
Copy Markdown
Contributor

Oh, yes please 🙏🏼 I think this could be achieved by not actually applying color to the icon content by default and instead using inherit but not all content follows exactly the same path or styling here.

Copy link
Copy Markdown
Contributor

@Westbrook Westbrook left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Highly yak shaving, but possible community alignment questions.

  • Do we like custom being the "public" API as opposed to the Spectrum brand?
  • Is there be benefit from leveraging the _ syntax to imply privacy of the defaults? I've seen a couple of projects pick up this pattern, but not sure it's a "best practice" by any means.
background-color: var(
    --_highcontrast-actionbutton-background-color-default, /* WHCM is "private" */
    var(--spectrum-actionbutton-background-color-default), /* public API is Spectrum branded */
    var(--_actionbutton-background-color-default) /* default is "private" and unbranded */
);

Separately, we should make sure that the highcontrast usage gets into the core tokens API so that it easy to mechanically generate https://css-tricks.com/using-property-for-css-custom-properties/ so we can animate on these things if the need/x-browser support arises.

@lazd
Copy link
Copy Markdown
Member Author

lazd commented Apr 28, 2022

Highly yak shaving, but possible community alignment questions.

  • Do we like custom being the "public" API as opposed to the Spectrum brand?

I haven't vetted the naming convention with the rest of the team yet, but I can explain why I chose custom. I wanted to differentiate the API that people can modify from the "defaults" (the --spectrum tokens), and tossed around the idea of appending -override to the end of the token name, but eventually just said hey, let's just change the prefix to --custom to indicate it's a customization using custom properties. It felt right, heh.

I'm open to changing it though!

  • Is there be benefit from leveraging the _ syntax to imply privacy of the defaults? I've seen a couple of projects pick up this pattern, but not sure it's a "best practice" by any means.

Hmm, we could consider it, though it "undocumented" generally means the same thing, and we'll only be documenting the --custom-* versions.

Separately, we should make sure that the highcontrast usage gets into the core tokens API so that it easy to mechanically generate https://css-tricks.com/using-property-for-css-custom-properties/ so we can animate on these things if the need/x-browser support arises.

I would like to see a set: { highcontrast: ButtonFace } in the core tokens data alongside wireframe and light/dark... @GarthDB thoughts? It would be a large undertaking for design, but if we did this at the core tokens level, we could make @jnurthen's life way easier.

@pfulton
Copy link
Copy Markdown
Collaborator

pfulton commented May 9, 2022

I don't have a strong preference between the two suggested prefixes (_ and custom). I noticed that the Open Props project is using the _ for customized theming in some places (see screenshot below).

IMO the word custom is a bit more readable than an underscore, so maybe that's swinging my opinion in that direction.

Curious to hear what others think, and happy to side with the majority opinion or a better recommendation.

Screen Shot 2022-05-09 at 4 17 37 PM

@lazd
Copy link
Copy Markdown
Member Author

lazd commented May 10, 2022

I'm inclined to stick with the naming convention as-is, it feels the most true to what it's doing and most readable. @GarthDB, can you weigh in?

@lazd lazd force-pushed the lazd/CSS-98-actionbutton-core-tokens branch from a9bf4a3 to 642f528 Compare May 11, 2022 17:20
@lazd lazd marked this pull request as ready for review May 11, 2022 17:29
@GarthDB
Copy link
Copy Markdown
Member

GarthDB commented May 12, 2022

@lazd I think we covered it in meetings. Custom isn't my favorite, but it's functional and I don't have a better recommendation.

lazd added 2 commits May 12, 2022 11:19
re-organize vars, use alias for static focus-ring color, remove duplication
@lazd
Copy link
Copy Markdown
Member Author

lazd commented May 16, 2022

Design verification was completed, @badimon and friends actually went through line-by-line and looked at the custom property definition and were able to use that as the data for their verification. This PR is ready to get merged!

@lazd lazd mentioned this pull request May 16, 2022
11 tasks
@pfulton pfulton merged commit 0ce1cb5 into lazd/CSS-75-core-tokens-pre May 16, 2022
@pfulton pfulton deleted the lazd/CSS-98-actionbutton-core-tokens branch May 16, 2022 16:32
lazd added a commit that referenced this pull request May 17, 2022
BREAKING CHANGE: .spectrum-ActionButton-icon is now required on icons

BREAKING CHANGE: .spectrum--express must be added to support Express ActionButton
pfulton pushed a commit that referenced this pull request Jun 10, 2022
BREAKING CHANGE: .spectrum-ActionButton-icon is now required on icons

BREAKING CHANGE: .spectrum--express must be added to support Express ActionButton
pfulton pushed a commit that referenced this pull request Jun 14, 2022
BREAKING CHANGE: .spectrum-ActionButton-icon is now required on icons

BREAKING CHANGE: .spectrum--express must be added to support Express ActionButton
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

Successfully merging this pull request may close these issues.

5 participants