feat(actionbutton): s2 migration #2669
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
🚀 Deployed on https://pr-2669--spectrum-css.netlify.app |
File metricsSummaryTotal size: 1.37 MB*
actionbutton
* An ASCII character in UTF-8 is 8 bits or 1 byte. |
mdt2
force-pushed
the
mdt2/css-739-s2-actionbutton
branch
from
de5e49c to
6220dae
Compare
mdt2
commented
Apr 17, 2024
There was a problem hiding this comment.
Hi, this is looking really good!
- S2 Token spec appears to match, mostly
- The only thing I noticed here was that I didn't see a cjk line height being applied, and I think there is one in the spec
- I also didn't look very closely at spacing since we'd previously talked about that not matching the spec, but I can if you'd like me to!
- Static white and static black don't have emphasized styling/no control is visible in Storybook ✅
- WHCM is looking good, both light and dark ✅
- I did have a question though, there is no style differentiation for selected hover state as there is for non-forced-colors, it's the same purple/cyan color for selected as it is for selected & hover. It's already like this in prod, so nothing that was introduced here. Is that something that would be worthwhile to fix here, to give it a different border color or something like that on hover?
- All variants and states are being shown in the testing preview ✅
|
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 |
jawinn
previously approved these changes
Apr 18, 2024
pfulton
force-pushed
the
spectrum-two
branch
2 times, most recently
from
f3dd9ff to
9931a8e
Compare
castastrophe
force-pushed
the
spectrum-two
branch
5 times, most recently
from
6dbc850 to
ae7aedd
Compare
mdt2
force-pushed
the
mdt2/css-739-s2-actionbutton
branch
from
6220dae to
e5e36b9
Compare
🦋 Changeset detectedLatest commit: 0254045 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
pfulton
force-pushed
the
spectrum-two
branch
2 times, most recently
from
e0e0fd2 to
698e904
Compare
mdt2
added
blocked
12 tasks
castastrophe
force-pushed
the
spectrum-two
branch
2 times, most recently
from
0faaab2 to
24419a9
Compare
Some small documentation improvements, especially around the selected state. Adds additional notes about the selected state being optional and for toggles (per SWC docs and guidelines).
This combination of states should continue to have the background color rather than the quiet transparent background. Per design feedback, the quiet + disabled + selected should display the same as the default disabled + selected.
Update tests to remove static black + emphasized and static white + emphasized, as these are not on the design spec and not supported. Updates some Docs page descriptions to make this clear.
Use design spec defined CJK line-height in action button. Replaces line-height declaration coming from the extended BaseButton, along with a mod name appropriate for the component.
- Fix for high contrast border styles not being solid. - Quiet variants no longer show a border and blend into background with Canvas and CanvasText color pair. - Focus indicator color changed from ButtonText to CanvasText, because this indicator appears outside of the element (it does not appear on top of a ButtonFace background).
Fixes the issue where the static black and white, quiet, selected, and disabled state in the testing grid was showing the static quiet background color instead of the static disabled background color.
The noted readability backplate issue was present on more than just the selected state; it was just difficult to see without contrasting colors. So this has been moved to the default, along with setting default system color values.
Refactors high contrast styles and custom properties to remove unnecessary variables, and adds some documentation.
Keep the previous S1 behavior of setting the line-height to the full component height. This prevents Thai diacritics from being cut off (see existing Storybook test). Note: this line-height is taller than both line-height-100 and cjk-line-height-100 listed on the design spec. Using those had resulted in the diacritics cutting off again.
Adjust some selectors to use :has in order to remove some overly complex calc functions, and to use the inline spacing tokens more like they are defined on the token specs. Now that :has is supported and used in our library, this simplifies this usage and removes some requirements for the source order of action button's child elements.
Usage notes about using a mod for static colors no longer applies; the --mod-actionbutton-content-color-default-selected mod and similar selected state mods work correctly to change the content color for static. Adds some basic intro docs for static colors, pulled from the guidelines.
The .spectrum-ActionButton--sizeM class is no longer used but was still appearing in the examples markup.
In Windows High Contrast, the border color briefly was flashing orange (ButtonBorder) after hover, before it switched back to the correct color as defined by the styles. This fixes that issue. This removes CSS transitions in high contrast to avoid some of these rendering issues and previously noticed sluggishness in the system color transitions.
castastrophe
force-pushed
the
mdt2/css-739-s2-actionbutton
branch
from
4bda4e0 to
398aa22
Compare
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Migrates Action Button to Spectrum 2 design specifications. Includes:
.spectrum-Switch--sizeMhas been removed.transparentand the mod custom properties were removed so as not to interfere with WHCM accessibility. High contrast styles have been refactored and improved.:has, since it is now used and supported in our library. This removes some overly complexcalc()functions used for inline spacing, uses the design tokens more directly, and removes the previously documented need for the component's child elements to use a specific source order.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
Regression testing
Expected diffs of note: "Static black - emphasized" and "Static white - emphasized" are removed from tests.
Validate:
To-do list