feat(checkbox/group): s2 token migration, control spacing, subtle color changes #3531
Conversation
🦋 Changeset detectedLatest commit: 48aabaa 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 |
|
🚀 Deployed on https://pr-3531--spectrum-css.netlify.app |
File metricsSummaryTotal size: 1.37 MB*
checkbox
* An ASCII character in UTF-8 is 8 bits or 1 byte. |
aramos-adobe
force-pushed
the
aramos-adobe/css1016-checkbox-group-s2-migration
branch
from
8a329ce to
6ca8078
Compare
There was a problem hiding this comment.
This is a great start! I pointed out some of the changes that I thought were still needed to the CSS within the CSS file, but here's a quick rundown of what I think needs another look:
- Down state
- Some of the invalid default colors (emphasized colors seem to be working fine for whatever reason) - this is a (hopefully small) bugfix that I also noticed on main
- Typography use of tokens
- Updating CJK to match other components
- Probably removing some stray dist files
- There are SO many test cases covered already, but for some reason we don't have any focus, hover, or active states. I don't think there's any particular reason we've left them out as far as I know, so I'm wondering how you would feel about adding them? I think it would make catching bugs (like the bug I just mentioned above) a little easier!
As far as I can tell, I think the template.js and stories.js files for this component are fine, so probably no changes needed there!
There was a problem hiding this comment.
Going through the Figma specs here are some things I noticed on the CSS:
- Checkbox layout - all of the tokens look good here except for Component size (down):
- Awhile ago we wrote up some docs for down state in feat(downstate): docs + implementation for example components #2520, at that time the down state for checkbox was migrated to S2 as well so that it could be used on that docs page. There's also a comment in that PR from SWC about the approach there not working, that's something that we probably need to take some action on and find out more about to follow up. As it in this PR, it's changed from the checkbox being just a bit smaller when active, to being really small when active, so this will probably need another look.
- Checkbox group layout - Checking the fieldgroup component, this looks good!
- Checkbox colors - this needs some work:
- The invalid/error default checked hover (screenshot below is hovered) color is the
neutral-content-color-hoverblack and not thenegative-color-1000red. I think this is a bug that exists onmaintoo, not something you introduced.
- The down/active color on this same invalid default checked checkbox is also a black color and not a red color, so this would need to be updated as well.
It's a little strange that there's not a visual for the error, I wonder what's going on with that, but I think that's outside of the work of this PR 🤷♀️
- The invalid/error default checked hover (screenshot below is hovered) color is the
- Checkbox typography
- We're not using
default-font-familyin the component, should we be? - Same for
regular-font-weightand - Also
default-font-style, but I see TODO comments in here around L370, it might be time to start setting those styles? I'm not sure of the historical reason why they weren't before. - CJK - this looks good and works just fine! But could you revise it so that the custom properties are redefined instead of resetting the style (probably moving the whole selector up to the top with the custom property definitions)? Similar to how you did it in Illustrated Message but with line height. Nothing is wrong with the way you have it, but after analyzing our components and realizing we're doing it 3 different ways across all of them, we want to try to standardize how we do this wherever we can, I just documented this in our style guide draft.
- We're not using
There was a problem hiding this comment.
@rise-erpelding the default-font-family doesn't render correctly for some reason. For component size (down) there is something in documentation describing the usage.
There was a problem hiding this comment.
Should we have this components/checkbox/dist/index.css.map and components/checkbox/dist/index.css committed?
There was a problem hiding this comment.
Wondering if these dist files for progresscircle also need to be removed.
castastrophe
force-pushed
the
spectrum-two
branch
2 times, most recently
from
56c6806 to
793571c
Compare
castastrophe
force-pushed
the
spectrum-two
branch
3 times, most recently
from
f34473b to
cf46c2b
Compare
aramos-adobe
force-pushed
the
aramos-adobe/css1016-checkbox-group-s2-migration
branch
from
0a7d424 to
cf46c2b
Compare
aramos-adobe
force-pushed
the
aramos-adobe/css1016-checkbox-group-s2-migration
branch
from
f6bfaa1 to
988b92a
Compare
castastrophe
force-pushed
the
spectrum-two
branch
4 times, most recently
from
dee81f5 to
ce2f3e0
Compare
aramos-adobe
force-pushed
the
aramos-adobe/css1016-checkbox-group-s2-migration
branch
from
644902d to
f77a28f
Compare
components/checkbox/index.css
Outdated
| /* WHCM settings */ | ||
| .spectrum-Checkbox-box, | ||
| .spectrum-Checkbox-input:checked + .spectrum-Checkbox-box, | ||
| &.is-indeterminate .spectrum-Checkbox-input:checked + .spectrum-Checkbox-box, | ||
| &.is-indeterminate .spectrum-Checkbox-box { | ||
| &::before { | ||
| border-color: var(--highcontrast-checkbox-color-default); | ||
| } | ||
| } | ||
|
|
||
| &:hover { | ||
| .spectrum-Checkbox-box::before, | ||
| .spectrum-Checkbox-input:checked + .spectrum-Checkbox-box::before { | ||
| border-color: var(--highcontrast-checkbox-color-default); | ||
| } |
There was a problem hiding this comment.
High contrast only styles should live in the forced-colors media query. ✅
Also could these possibly be handled by changes to custom property values instead?
castastrophe
force-pushed
the
spectrum-two
branch
from
95af748 to
5923e29
Compare
castastrophe
force-pushed
the
aramos-adobe/css1016-checkbox-group-s2-migration
branch
from
8a2062a to
a737bf0
Compare
There was a problem hiding this comment.
I'm seeing a couple things left:
Disabled styling looks to have changed. There's also a stylelint warning on the PR that might be related?
On default focus, the checked state isn't showing the darker blue background. Same for the read-only checked. The darker blue does show on focus for indeterminate; see the difference in blues here:
The high contrast focus indicator definitely looks better than before, thanks!
When inspecting I happened to notice some styles in the forced colors media query that are overridden and don't appear to be used. Can this line be removed? It also seems to have an extra "0" on the box-shadow as compared to the other styles:
spectrum-css/components/checkbox/index.css
Lines 514 to 516 in a737bf0
components/checkbox/index.css
Outdated
| border-color: var(--highcontrast-checkbox-color-default, var(--mod-checkbox-control-selected-color-default, var(--spectrum-checkbox-control-selected-color-default))); | ||
| background-color: var(--spectrum-checkbox-checkmark-color); | ||
| &:hover, | ||
| .is-focus-visible { |
There was a problem hiding this comment.
| .is-focus-visible { | |
| &:focus-visible { |
We shouldn't be using the .is-focus-visible class in our CSS, as this class is only added within our Storybook by the build process so we can test pseudo classes on the VRTs. That's done by the postcss-pseudo-classes plugin.
This is leading to an incorrect color appearing on the Docs page when I tab focus on the emphasized indeterminate checkbox, which is different than what appears on the testing preview grid (that looks correct):
There was a problem hiding this comment.
This is almost there.
Some pseudo selectors that need to apply to .spectrum-Checkbox-input
For the updated uses of :focus-visible and :disabled, those need to be applied to the input and not the root class. Reviewing the metadata.json I saw some selectors starting like these where they are being applied to the root classes:
.spectrum-Checkbox.is-readOnly:focus-visible...
.spectrum-Checkbox--emphasized:not(:disabled):focus-visible...
.spectrum-Checkbox--emphasized.is-indeterminate:not(:disabled):focus-visible...
One leftover disabled class
There is also still one instance of .is-disabled.
Down state when selecting with keyboard
I noticed this when testing the pseudo states and considering our :active selectors. The down state is not shown when tabbing to a checkbox and selecting it with space, which I'm guessing it should? I think this could be resolved by moving the :active to also apply to the input and not the root element---that'd need some testing.
There was a problem hiding this comment.
This is looking really good, and it's come a long way since I last looked at it!
I left a lot of my CSS feedback in the comments below, re: CJK and a question about a custom property declaration. I also had an additional question about WHCM that need not be a blocker here: are we meant to have differing colors for variants like invalid or emphasized? All I see is green for disabled and blue otherwise. We can definitely address this as part of the WHCM follow-up work once we have clearer specs from design though!
I had a few callouts about testing also in the comments, I think we're definitely covering all the things we should aside from down state but maybe should consider eliminating redundant test cases.
Last question/callout: I didn't see any changes made to checkbox group (field group), it seems from looking at the spec like they weren't needed, should we remove group from the commit message in that case? And maybe note it to Patrick separately that this component did not need any changes to be considered migrated.
| /* CJK (Chinese, Japanese, and Korean) language support */ | ||
| &:lang(ja), | ||
| &:lang(zh), | ||
| &:lang(ko) { | ||
| line-height: var(--mod-checkbox-line-height-cjk, var(--spectrum-checkbox-line-height-cjk)); | ||
| } |
There was a problem hiding this comment.
I noticed that I don't think this is working? I'm wondering if it's because line-height isn't set on .spectrum-Checkbox as it's being set for CJK here, it's set on .spectrum-Checkbox-label and never reset for .spectrum-Checkbox-label for CJK.
Redefining --spectrum-checkbox-line-height within this block here might work instead though.
| .spectrum-Checkbox { | ||
| --spectrum-checkbox-selected-border-width: calc(var(--spectrum-checkbox-control-size) / 2); |
There was a problem hiding this comment.
I notice we're setting this custom property in this selector where otherwise we're setting style properties, is there a reason this needs to be here? If so, let's add a comment! If not, can we move it up with the other custom properties?
| .spectrum-Checkbox { | ||
| &.is-disabled.is-indeterminate { | ||
| .spectrum-Checkbox-input:focus-visible + .spectrum-Checkbox-box { | ||
| &::before { | ||
| border-color: var(--highcontrast-checkbox-disabled-color-default); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| &.is-readOnly.is-indeterminate { | ||
| .spectrum-Checkbox-input:focus-visible + .spectrum-Checkbox-box { | ||
| &::before { | ||
| border-color: var(--highcontrast-checkbox-highlight-color-default); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| &.is-readOnly { | ||
| .spectrum-Checkbox-input:focus-visible + .spectrum-Checkbox-box { | ||
| &::before { | ||
| border-color: var(--highcontrast-checkbox-highlight-color-default); | ||
| } |
There was a problem hiding this comment.
This whole section is interesting, is there a reason we're setting --highcontrast custom properties down here in the WHCM section instead of putting them in a var stack further up? It seems like some of this was here before and I know battles have already been fought with WHCM in this PR, so would guess maybe we do need this for a reason? Just curious 🙂
| /* Change the font styles in all browsers for input. */ | ||
| font-family: inherit; | ||
| font-size: 100%; | ||
| line-height: 1.15; |
There was a problem hiding this comment.
Really curious about these styles and why exactly we need them, the comments don't really explain it for me really well and/or I don't have a great grasp of how this would behave with various types of assistive technology. But I know these were here before so this may not be an answerable question.
There was a problem hiding this comment.
The added test coverage is so welcome!!! ❤️
I'm wondering if we can pare this back just a little though to exclude some of the repetitive cases? For instance, the default section has "hovered" and then also the "Hovered" section has "hovered", are they both necessary?
I'm personally a fan of using include/ignore to hone in on what needs to be tested and take out what doesn't, but that definitely hasn't been a common/popular approach among the team and may not even be necessary here.
I also see that we have a "focused" section and that shows a focus ring around checkboxes that don't normally get one, like disabled checkboxes, which might be misleading.
Last thing on testing: should we test active/down state? Is there a good reason not to that I don't know of?
Checkbox/Group S2 Migrations
Checkbox has some new tokens for control and color. Most of the tokens have been updated in the global vars.
New tokens
--spectrum-component-size-width-ratio-down--spectrum-checkbox-bottom-to-text(S,M,L,XL)--spectrum-checkbox-top-to-control (S,M,L,XL)--spectrum-accent-content-color-default--spectrum-accent-content-color-hover--spectrum-accent-content-color-down--spectrum-accent-content-color-key-focusModified tokens
--spectrum-checkbox-checkmark-color--spectrum-checkbox-invalid-color-down--spectrum-checkbox-control-color-default--spectrum-checkbox-control-color-hover--spectrum-checkbox-control-color-down--spectrum-checkbox-control-color-focusHow 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