fix(button,statuslight,picker,etc): address regressions in migrated components#3687
Conversation
🦋 Changeset detectedLatest commit: 1de55cf The changes in this PR will be included in the next version bump. This PR includes changesets to release 7 packages
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 |
4971254 to
1cddd04
Compare
|
🚀 Deployed on https://pr-3687--spectrum-css.netlify.app |
File metricsSummaryTotal size: 1.38 MB*
File change detailsbutton
floatingactionbutton
link
logicbutton
picker
statuslight
switch
* An ASCII character in UTF-8 is 8 bits or 1 byte. |
| ></div> | ||
| ${ColorHandle({ | ||
| isDisabled, | ||
| isFocused, |
There was a problem hiding this comment.
Passing isFocused to the color handle is in line with the example on the guidelines site for color area.
b1b84b5 to
ddd5e69
Compare
| iconName: "Edit", | ||
| iconSet: "workflow", |
There was a problem hiding this comment.
These are already defined in the template being used, so it should be safe to remove them.
There was a problem hiding this comment.
Do we need a workflow example though to validate spacing?
There was a problem hiding this comment.
Hm, hopefully I'm not misunderstanding the test template, but I'm pretty sure they're already defined. The "Line wrap" test scenario calls for the ButtonIconGroup template. ButtonIconGroup has 4 wrapping buttons- 2 with workflow icons and 2 with UI icons. So I think we have the workflow examples & spacing covered. These extra iconName and iconSet args were just repeats and not doing anything anyways (since the icons are defined in ButtonIconGroup instead).
If that's not accurate, I can definitely revert this!
| label: "Be a premium member", | ||
| noWrap: true, |
There was a problem hiding this comment.
These 2 args were being repeated in each template in TextWrapTemplate, so I moved them here (it also made it clearer to me to see the noWrap: true in the story args as well).
|
|
||
| .spectrum-FloatingActionButton-icon { | ||
| fill: var(--highcontrast-floating-action-button-icon-color-hover, var(--mod-floating-action-button-icon-color-hover, var(--spectrum-floating-action-button-icon-color-hover))); | ||
| color: var(--highcontrast-floating-action-button-icon-color-hover, var(--mod-floating-action-button-icon-color-hover, var(--spectrum-floating-action-button-icon-color-hover))); |
|
|
||
| --spectrum-link-focus-indicator-color: var(--spectrum-static-white-focus-indicator-color); |
| padding: var(--mod-logic-button-padding, var(--spectrum-component-edge-to-text-50)); | ||
|
|
||
| border-width: var(--mod-logic-button-border-width, var(--spectrum-border-width-200)); | ||
| border-style: solid; |
There was a problem hiding this comment.
I noticed the weird border was occurring on logic button, similar to the border we saw on action button. That's the solution I used here too 👍


| control: "boolean", | ||
| if: { arg: "labelPosition", eq: "side" }, | ||
| }, | ||
| showWorkflowIcon: { |
There was a problem hiding this comment.
This addition can be found in the docs-to-storybook PR for picker: Picker docs to storybook migration
|
|
||
| /* Font */ | ||
| --spectrum-statuslight-font-family: var(--spectrum-default-font-family); | ||
| --spectrum-statuslight-font-family: var(--spectrum-sans-font-family-stack); |
There was a problem hiding this comment.
I changed this to the font-family-stack token that we've been favoring instead.
| --spectrum-switch-animation-duration-100: var(--spectrum-animation-duration-100); | ||
| --spectrum-switch-animation-duration-200: var(--spectrum-animation-duration-200); |
There was a problem hiding this comment.
From the switch migration, I noticed these 2 custom props were missing. I don't think we should see any difference, expect that the styles have one extra layer of token abstraction.
| --highcontrast-switch-background-color-selected-disabled: GrayText; | ||
|
|
||
| --highcontrast-switch-focus-indicator-color: ButtonText; | ||
| forced-color-adjust: none; |
There was a problem hiding this comment.
I noticed this had been added to this force-colors query in the switch migration: S2 switch migration
| }, context)) | ||
| } | ||
| <span class="${rootClass}-label is-placeholder">${placeholder}</span> | ||
| ${when(showWorkflowIcon, () => |
There was a problem hiding this comment.
Looks like S2 still supports a leading icon, so all of this has been added. If I remember correctly, however, it wasn't in the initial S2 migration, but it was in the docs-to-storybook migration listed in my other comment.
| &.is-selected { | ||
| --spectrum-button-background-color-default: var(--spectrum-neutral-subdued-background-color-default); | ||
| --spectrum-button-background-color-hover: var(--spectrum-neutral-subdued-background-color-hover); | ||
| --spectrum-button-background-color-down: var(--spectrum-neutral-subdued-background-color-down); | ||
| --spectrum-button-background-color-focus: var(--spectrum-neutral-subdued-background-color-key-focus); | ||
| } |
There was a problem hiding this comment.
I don't believe there's a "selected" state for buttons. That doesn't seem to an option in Figma, isn't represented anywhere in the guidelines page, there's no "selected" state in the Storybook controls. We do have this block on main (actually, several is-selected selector blocks).
Think it's ok to remove? I don't see is-selected getting added to a button anywhere, but am I missing it in a component somewhere?
There was a problem hiding this comment.
I double checked the web component and you're correct that there is no selected state for button. Safe to remove!
| --spectrum-button-icon-size-difference: max(0px, var(--spectrum-button-intended-icon-size) - var(--spectrum-icon-block-size, var(--spectrum-button-intended-icon-size))); | ||
| /* stylelint-disable-next-line spectrum-tools/no-unknown-custom-properties -- Any block-size difference between the intended workflow icon size and actual icon used. Helps support any existing use of smaller UI icons instead of intended Workflow icons. */ | ||
| --_icon-size-difference: max(0px, var(--spectrum-button-intended-icon-size) - var(--spectrum-icon-block-size, var(--spectrum-button-intended-icon-size))); |
There was a problem hiding this comment.
Some of the changes in this selector corresponded to the changes in this PR: PostCSS plugin updates/fixes
| } | ||
|
|
||
| /* Disable button label wrap */ | ||
| .spectrum-Button--noWrap .spectrum-Button-label { |
There was a problem hiding this comment.
ddd5e69 to
4a1b2d4
Compare
jawinn
left a comment
There was a problem hiding this comment.
This looks good to me. I looked over the button CSS in detail with a diff against the original S2 migration, and this looks correct when merged with the later changes made to main.
I only have one minor suggestion for two variable names.
There was a problem hiding this comment.
Thanks for the detailed changesets ⭐
| --spectrum-switch-animation-duration-100: var(--spectrum-animation-duration-100); | ||
| --spectrum-switch-animation-duration-200: var(--spectrum-animation-duration-200); |
There was a problem hiding this comment.
Can we name these for their purpose instead of their value? Something like --spectrum-switch-animation-duration and --spectrum-switch-animation-duration-label?
7e8269c to
4ce8d19
Compare
- redefines correct tokens, particularly for button border colors - reimplements button label overflow behavior (--noWrap class) - updates metadata - cleans up test case and disabled text wrap story to reduce repetitive args/code
- redefines and uses correct tokens, particularly for switch animation duration - updates metadata
- fix token usage, particularly for border color and width - fixes picker template to support a leading icon and adds associated controls to story args - passes state data into picker template so hover, active, focus styles are applied correctly
- also updates metadata
- adds a note to the changeset
- border-style: solid removes the 3D effect on the button border - adds a note to the changeset
- updates the animation custom property names to reflect their purpose - updates metadata - fixes up some changeset wording
4a1b2d4 to
1de55cf
Compare




Description
This work addresses small regressions found in several components. For the most part. it was simple enough to update the token definitions correctly. Button and picker however, were slightly more involved, and required some adjustments to their templates and/or story files. The following components needed CSS fixes:
There was also a missing argument not being passed to the color handle child within the color area component, so that was added.
Jira
CSS-1175
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
Recent PR preview
3687 PR preview
colorproperty)border-style: solidto remove the 3d border jenkiness)Additional validation
Because the button component is used so widely, feel free to check on the following components to ensure they look as intended and follow their own design specs as well:
Regression testing
Validate:
Screenshots
To-do list