feat(actionbutton): s2 migration

[mdt2]

Waiting for S2 icons

Description

Migrates Action Button to Spectrum 2 design specifications. Includes:

• Medium is now the default size and .spectrum-Switch--sizeM has been removed.
• Includes the Spectrum 2 down state transform.
• The component border was not removed to continue support for Windows High Contrast Mode (WHCM), but the color was adjusted to transparent and the mod custom properties were removed so as not to interfere with WHCM accessibility.
• Background and content colors were updated.
• Mod custom properties have been adjusted (find changes in actionbutton.yml)
• Adjustment of Storybook testing preview to make Selected and Selected + Emphasized variants instead of states to test more cases

How and where has this been tested?

Please tag yourself on the tests you've marked complete to confirm the tests have been run by someone other than the author.

Validation steps

@jawinn

• [x] Compare ActionButton Storybook to the design specs to ensure correct tokens are used (S2 spec is above, but I also found this Figma file to be useful while making these changes)
• [x] StaticWhite and StaticBlack don't have an emphasized selected state and the emphasized control isn't shown on these stories
• [x] WHCM is looking good
• [x] Check that all variants and states are being shown in the testing preview (I rearranged some things to better align with this Figma file from design and surface the states for selected and selected + emphasized)
• [ ] Design validation @mdt2

Regression testing

Validate:

1. The documentation pages for at least two other components are still loading, including:

• [ ] The pages render correctly, are accessible, and are responsive.

2. If components have been modified, VRTs have been run on this branch:

• [ ] VRTs have been run and looked at.
• [ ] Any VRT changes have been accepted (by reviewer and/or PR author), or there are no changes.

To-do list

• [x] I have read the contribution guidelines.
• [x] I have updated relevant storybook stories and templates.
• [x] I have tested these changes in Windows High Contrast mode.
• [x] If my change impacts documentation, I have updated the documentation accordingly.
• [ ] ✨ This pull request is ready to merge. ✨

[github-actions[bot]]

🚀 Deployed on https://pr-2669--spectrum-css.netlify.app

[github-actions[bot]]

File metrics

Summary

Total size: 1.37 MB^*

Package	Size	Minified	Gzipped
actionbutton	23.98 KB	22.92 KB	3.07 KB

actionbutton

Filename	Head	Minified	Gzipped	Compared to base
index.css	23.98 KB	22.92 KB	3.07 KB	🔴 ⬆ 0.75 KB
metadata.json	10.30 KB	-	-	🟢 ⬇ 0.21 KB

* Size is the sum of all main files for packages in the library.
* An ASCII character in UTF-8 is 8 bits or 1 byte.

[mdt2]

Thanks Rise! To document some context here, when you say "didn't look very closely at spacing since we'd previously talked about that not matching the spec" we're talking about the top to edge spacing tokens, which don't seem to be needed since we're using display: flex. I also didn't make updates to the icon only spacing since visually it looks like the spec even though we're not using the spec tokens to avoid causing more work for SWC.

[changeset-bot[bot]]

🦋 Changeset detected

Latest commit: 0254045716f9aac40880b86bd2b1e55847595541

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@spectrum-css/actionbutton	Major

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[jawinn]

@rise-erpelding All of your comments should be addressed, if you could take another look:

---

• Typography -

  • Font-style: this feels more like a nit, but I don't think we're using the font-style token, although in this case it evaluates to normal either way

Thanks! I've added this, along with its new mod. Note for posterity; this was something we started making sure we specifically define in these S2 migrations, after this PR was originally created. For some of these inheritable font properties, we want to make sure they are all set so they don't inherit from something else.

• CJK: I know I called this one out before and didn't see a clear answer, but it's possible I missed it - is it unnecessary for some reason to set CJK line height here? Is it a little out of scope, maybe, since line-height is set on basebutton and not the actionbutton CSS?

I added the CJK token to address this, but ended up changing the approach after addressing Marissa's comments about the Thai diacritics cutting off. The line-height is now set back to the full component height, like it is in main, and I've added some comments to explain why. bb62c1f

Of note: something also tied to this is the use of overflow: hidden, which is needed for the text-overflow behavior to work. Without the hidden, the diacritics actually do appear with the reduced line-heights that match the tokens. But since text does not wrap in this component, having an even larger line-height as tall as the component continues to seem like an okay approach.

• WHCM - I know we don't really have a design spec for this, but I would say that these borders are looking a little funny. I think this might be happening because border-style is never explicitly set, so it computes to outset? Only an issue in WHCM since other borders are transparent...

Good callout. I've done a bunch of updates and refactoring to the high contrast styles to fix this and other issues. Including making sure that quiet (not selected or disabled) does not show a border. I've included some comments in the high contrast styles to make it super clear. Please take a look. Note; I noticed that Chrome's forced-colors emulation has a lower than 1 opacity for the highlight keywords for some odd reason which is why the selected border may continue to look slightly different than its background.

• Emphasized & static color - on the controls, it looks like this combination of variants can't exist, so would we be able to remove those blocks from the tests?

These are removed from the tests and I've added some docs about this combination not being supported.

• Spacing - I'm still ignoring block spacing, but I'm questioning inline spacing. The previous note was

  I also didn't make updates to the icon only spacing since visually it looks like the spec even though we're not using the spec tokens to avoid causing more work for SWC.

  Maybe that's still valid, but maybe it's not? After looking at this for awhile, I think the inline spacing that I believe was already in place before is actually probably all correct, but it is SO confusing because it's all calcs and some of the spacing is set on the button, and some on the icon. It would be so nice to be able to eliminate some of that complexity if possible, or add explanation in comments if not possible. Although at this point I'm happy to leave it for now in favor of getting this PR through.

You're right that these calcs and the previous approach was pretty confusing. It looks to have been mostly done because of the lack of :has. Since we now support and use this in our library, I've refactored this using :has 🎉 See f1844aa and the addition to the changeset + PR description. The inline spacing now uses these tokens more directly. Let me know if the use of these tokens looks correct now.

[jawinn]

@marissahuysentruyt All of your comments should be addressed as well, if you could take another look:

Yay, let's get this migrated! 🥳 I had a few questions (like normal) for you I left inline, and then a few that I wasn't totally sure where to put sooooo...

• The "Spacing top edge to icon" and "spacing top edge to label": I don't see those tokens in the action button CSS. I looked at the base button CSS, and icon CSS but didn't see them there either. Am I missing something there?

Block-spacing: since this component does not wrap, the vertical centering is handled by flex and we don't need to use those tokens (keeping the existing S1 behavior). Inline-spacing: this got a refactor too use :has and the tokens in a less confusing way per Rise's comments.

• I didn't see any line height styles. I'm not particularly surprised (I know those have been giving us a hard time lately), but I wanted to make sure whether or not I should see them defined. Is that being considered "out of scope" for now? I'm not sure what the state of this Thai diacritic test is on spectrum-two, but it is getting cut off currently.

Good catch on the diacritics! It seems that using the line-height-100 and cjk-line-height-100, as specified on the token specs will result in the diacritics getting cut off. Partly because the overflow is hidden to allow for text-overflow to work correctly. The original CSS on main has the line-height set to the full component height. I've restored that behavior in bb62c1f and have left some comments about it.