-
Notifications
You must be signed in to change notification settings - Fork 201
feat(actionbar): new s2 migration #3657
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
base: spectrum-two
Are you sure you want to change the base?
feat(actionbar): new s2 migration #3657
Conversation
🦋 Changeset detectedLatest commit: ba9179c 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 |
File metricsSummaryTotal size: 1.38 MB*
actionbar
* An ASCII character in UTF-8 is 8 bits or 1 byte. |
🚀 Deployed on https://pr-3657--spectrum-css.netlify.app |
701ca81
to
0e43d2d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before I went too far, I wanted to ask why several of the components were downgraded from the spectrum-two
branch. I see some of our plugins have been downgraded, as well as our token package and bundle. Was that intentional, do we have conflicting dependencies or something? Those changes just caught me off guard, so I wanted to double check the reasoning.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here are a couple of things I wanted to bring up with you! I wasn't sure where to put them...
- In the actionbar.stories.js file, would you update the documentation regarding the emphasized's blue background, since it got changed to the gray for S2?
- If I'm understanding Figma correctly, the expectation for an emphasized action bar, in dark mode, is to have a border, correct? If I inspect the Figma component, there's still a border:

But right now, the border-color: transparent
at line 132ish is overriding that. Should it match Figma? This wasn't something you introduced, but something I noticed. Based on the comments about WHCM, I wonder if the transparent border is even something we need, now that this component has a border in the emphasized variant. Just testing with that line removed, it looks like the borders in high contrast mode are still looking good.
I will give the rest of the tokens a better look tomorrow!
components/actionbar/index.css
Outdated
.spectrum-ActionGroup .spectrum-ActionButton { | ||
.spectrum-ActionButton-label { | ||
--mod-actionbutton-label-color: var(--spectrum-actionbar-emphasized-action-button-label-color); | ||
} | ||
|
||
.spectrum-ActionButton-icon { | ||
color: var(--spectrum-actionbar-emphasized-icon-color); | ||
} | ||
} | ||
|
||
.spectrum-CloseButton .spectrum-Icon { | ||
color: var(--spectrum-actionbar-emphasized-icon-color); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interestingly, I only get the black boxes in the Chromatic test preview. 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OHHHH I think this is related to the background colors passed to Container()
s. I'm more and more convinced the longer I stare at this that we have to revert the Container()
addition.
Container({ | ||
withBorder: false, | ||
wrapperStyles: { | ||
"column-gap": "var(--spectrum-action-bar-close-button-to-counter)", | ||
"align-items": "center", | ||
}, | ||
content: [ | ||
CloseButton({ | ||
label: "Clear selection", | ||
staticColor: isEmphasized ? "white" : undefined, | ||
}, context), | ||
FieldLabel({ size: "s", label: "2 Selected" }, context), | ||
] | ||
}), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm...I'm unsure if Container()
was really intended to be used in our templates. Right now, it's adding a lot of additional markup that doesn't seem necessary, and I'm not sure SWC would be able to do anything with these Storybook-only classes & divs.
We may have to revert this back to the old markup.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
components/actionbar/index.css
Outdated
.spectrum-ActionBar--emphasized .spectrum-ActionBar-popover { | ||
--highcontrast-actionbar-popover-border-color: CanvasText; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This selector is a little repetitive to me, so can we remove it? --highcontrast-actionbar-popover-border-color: CanvasText;
was already declared above in .spectrum-ActionBar
, and this second selector block is just variants, so isn't it already set?
components/actionbar/index.css
Outdated
|
||
.spectrum-ActionGroup .spectrum-ActionButton { | ||
.spectrum-ActionButton-label { | ||
--mod-actionbutton-label-color: var(--spectrum-actionbar-emphasized-action-button-label-color); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if, for the emphasized action bar specifically in dark mode in force colors/WHCM 🥵 if this should use a high contrast variable. Testing it in both the Chrome forced-colors emulator and Assistiv labs, if I switch to dark mode, the emphasized action buttons are not visible.
--mod-actionbutton-label-color: var(--highcontrast-actionbar-emphasized-action-button-label-color, var(--spectrum-actionbar-emphasized-action-button-label-color));
I don't know where quite yet we'd want that mod, if it's here or somewhere else.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, and somewhat related- I'd like to see this action button mod passthrough be organized with the rest of the custom property definitions, instead of mixed in with the style definitions.
.changeset/deep-sloths-fetch.md
Outdated
|
||
### Actionbar S2 Migration | ||
|
||
Action bar now has some updated colors, corner radius, icons and outline has been removed. The default and emphasized variant now have a light and dark mode theme. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we clarify the part about the outline/border a little? The border is still there, but just different colors based on light vs dark modes.
--spectrum-actionbar-shadow-color: var(--spectrum-drop-shadow-elevated-color); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After these drop shadows, I would also suggest setting --mod-closebutton-icon-color-hover: var(--spectrum-actionbar-emphasized-icon-color);
, so that in dark mode, the emphasized action bar's close button doesn't change its icon color. (it mimics the dark mode default action bar's close button).
Actionbar S2 Migration
Action bar now has some updated colors, corner radius, and icons The default and emphasized variant now have a light and dark mode theme.
The emphasized variant has visibly changed from blue to gray.
New tokens
--spectrum-actionbar-emphasized-actionbutton-label-color
--spectrum-actionbar-emphasized-icon-color
--spectrum-actionbar-minimum-width
--spectrum-actionbar-spacing-label-to-actiongroup
--spectrum-actionbar-spacing-action-group-edge
New mods
--mod-actionbar-minimum-width
--mod-actionbar-spacing-action-group-edge
--mod-actionbar-spacing-label-to-actiongroup
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
Validate:
Screenshots
To-do list