feat(picker): refactoring template markup, testing token passthroughs #3792
Conversation
🦋 Changeset detectedLatest commit: 6f2e972 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.42 MB*
picker
* An ASCII character in UTF-8 is 8 bits or 1 byte. |
|
🚀 Deployed on https://pr-3792--spectrum-css.netlify.app |
aramos-adobe
force-pushed
the
aramos-adobe/css1065-picker-markup-followup
branch
from
d7c1ead to
e519171
Compare
aramos-adobe
added
size-5 L
aramos-adobe
changed the title
There was a problem hiding this comment.
I think there's some style fixes that need to be addressed, specifically for the quiet variants. I left a thought about probably reimplementing the isQuiet arg getting passed to Picker() (not Template()). We'll also need to double check the side+quiet variant since the text alignment is pretty wonky:
This PR preview:
The table migration preview:
|
@aramos-adobe Would you be open to adding a test case where we check side + quiet pickers? That's the controls I used to find the text alignment issue. |
There was a problem hiding this comment.
This is looking pretty good. My comments are mainly organizational suggestions, but there are some quiet spacing tokens that were removed, that I believe need to be added back in.
However, I'm a little puzzles with WHCM, and didn't dig too much into them. Testing on AssitivLabs, I'm seeing a double border around the component. I'm assuming this has to do with changing from .spectrum-Picker to .spectrum-Picker-button?
The disabled picker borders are getting cut off, so there might be some exploration needed there. I'm thinking if we fix the double borders first, the disabled cut off borders will probably fix themselves.
Is the chevron for a loading picker supposed to be disabled? It looks like it was before, but I don't actually know. It doesn't really look like that in the token specs, so I'm actually inclined to keep your changes. However, it does make sense to disable that chevron, since a user can't open the picker while it's loading...thoughts?
before:
after:
| @@ -0,0 +1,16 @@ | |||
| --- | |||
| "@spectrum-css/picker": major | |||
There was a problem hiding this comment.
So this is on the spectrum-two branch. I'm not certain how we're handling the changesets, so this is just a question for and about the future. When we do finally merge spectrum-two into main, do you know yet how we're going to handle this "major" change, AND the S2 picker migration "major" change? Are they going to get lumped together, and picker will get bumped only 1 version number, or are they going to be separated so that picker actually gets 2 version bumps?
This isn't really a question related to your work, I agree that these are breaking changes. 👍
There was a problem hiding this comment.
@marissahuysentruyt Can this be a patch? I'm not sure how it works but hopefully that doesn't bump the version number
There was a problem hiding this comment.
I don't think it's small enough to be a patch, personally. Maybe Cassondra or Patrick have a preference? I mean....it's a breaking/major change from the last iteration of the picker. That picker however, isn't available to consumers though.
The changesets stuff trips me up a bunch. If it makes you feel better, when we broke the dialog migration up, I added 2 changesets both marked major. I had the same question then!! 😆
There was a problem hiding this comment.
We can (and probably will) fuss with the changelog manually for this kind of release when the time comes, so I'm not worried about semver, per say. At this point, I am thinking about what impact changes might have on the SWC project. Would they require significant refactoring to support these changes? Then yeah, it probably is a major from that perspective.
There was a problem hiding this comment.
(Such a great clarification and I so appreciate y'all asking it! Can we surface this in the team chat as well to make sure we're all in agreement?)
| @@ -0,0 +1,16 @@ | |||
| --- | |||
| "@spectrum-css/picker": major | |||
There was a problem hiding this comment.
I don't think it's small enough to be a patch, personally. Maybe Cassondra or Patrick have a preference? I mean....it's a breaking/major change from the last iteration of the picker. That picker however, isn't available to consumers though.
The changesets stuff trips me up a bunch. If it makes you feel better, when we broke the dialog migration up, I added 2 changesets both marked major. I had the same question then!! 😆
There was a problem hiding this comment.
I think this is looking good! Since Cassondra is working on the POC for web components, this might be a good PR to have her look at. To me, it more closely matches the markup for the current sp-picker, but I don't have a good enough understanding of the reason the SWC popover is in the shadow root, and if we've introduced anything that really makes it hard for them to consume the CSS.
Overall, this PR does what the ticket says- it adds the t-shirt sized spacing between the picker and the popover, and (at least to me) more closely aligns our template markup, with SWC's sp-picker markup. 👍
Description
Refactored the Picker component template to encapsulate the popover, help text, label, and
spectrum-Picker-buttonwithin a unified structure. Thespectrum-Pickernow acts as the root class, allowing for more targeted button styles and streamlined --mod and style passthroughs to child elements.For example,
--mod-popover-animation-distance—defined in the Popover—now registers correctly within the Picker, enabling size variant tokens to apply expected values when the corresponding size class is used.New token
--spectrum-picker-popover-animation-distanceHow 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
- [ ] Correct
--spectrum-picker-popover-animation-distancetoken changes according per size variant- [ ] Layout still works for top and side labels
- [ ] Test grid environment still in tact
Regression testing
Validate:
Screenshots
To-do list