feat(combobox): adding new s2 migration #3683
Conversation
🦋 Changeset detectedLatest commit: f93eedb 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 |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Remaining comments which cannot be posted as a review comment to avoid GitHub Rate Limit
markdownlint-fix
[markdownlint-fix] reported by reviewdog 🐶
spectrum-css/components/checkbox/dist/metadata.json
Lines 195 to 197 in a986db0
[markdownlint-fix] reported by reviewdog 🐶
[markdownlint-fix] reported by reviewdog 🐶
spectrum-css/components/checkbox/dist/metadata.json
Lines 202 to 205 in a986db0
[markdownlint-fix] reported by reviewdog 🐶
[markdownlint-fix] reported by reviewdog 🐶
[markdownlint-fix] reported by reviewdog 🐶
File metricsSummaryTotal size: 1.40 MB* Table reports on changes to a package's main file. Other changes can be found in the collapsed Details section below.
File change detailscombobox
helptext
tokens
* An ASCII character in UTF-8 is 8 bits or 1 byte. |
|
🚀 Deployed on https://pr-3683--spectrum-css.netlify.app |
aramos-adobe
force-pushed
the
aramos-adobe/css769-combox-s2-migration
branch
from
a986db0 to
ff02395
Compare
castastrophe
force-pushed
the
spectrum-two
branch
3 times, most recently
from
7e8269c to
4ce8d19
Compare
aramos-adobe
added
ready-for-review
size-5 L
aramos-adobe
added
skip_vrt
…hub.com/adobe/spectrum-css into aramos-adobe/css769-combox-s2-migration
There was a problem hiding this comment.
This is an excellent start! I left several comments in the code, but here are some other considerations that didn't fit:
- I did notice that there were autocomplete colors in the design spec, even though we can't (or shouldn't) implement any autocomplete functionality here, we probably should make sure we have the autocomplete styles in here, even if just with a selector like
.spectrum-Combobox-input[autocomplete]. It looks like the textfield template has an autocomplete attribute that can be placed on the textfield input, so we could probably pass that arg to textfield from combobox once it's set up, and also wire up a Storybook control for it. - I think there might be a tiny design bug in the block spacing for the progress circle, I'll Slack you so that I can send screenshots and explain. This wasn't a part of the code you touched. (In fact there were several things I called out here that aren't related to changes you made but seemed like they've been wrong for awhile.)
- Last thing: I saw this interaction error in Storybook, I don't think it's a huge issue, could be a follow up ticket if anything? (There is a
role="presentation"so I'm not sure what it's complaining about)
There was a problem hiding this comment.
This is looking great! I don't have access to the Figma Token Spec to give this a full review yet but I did just have a couple questions for myself. I also had my build fail in my PR after I rebased with the main spectrum-two branch. It looks it was missing a few metadata.json files.
|
|
||
| ### New mods | ||
|
|
||
| `--mod-combobox-line-height-cjk` |
There was a problem hiding this comment.
This question is for future me, @castastrophe had mentioned to me that we were discontinuing use of mods. Are certain there scenarios like this that we will continue to support them?
There was a problem hiding this comment.
@saashapina @castastrophe I'm not entirely sure - I think that might be a web component related thing. But mods are still helpful for passthroughs between each component.
| fieldLabelText: "Select location", | ||
| value: "Ballard", | ||
| }; | ||
| Default.args = {}; |
There was a problem hiding this comment.
Also a question for my future reference, do we still need to define an empty object here if there are no args?
There was a problem hiding this comment.
@saashapina I don't need this at all actually! I can remove it
…hub.com/adobe/spectrum-css into aramos-adobe/css769-combox-s2-migration
| isDisabled, | ||
| isReadOnly, | ||
| value, | ||
| ...args, | ||
| ...passthrough, | ||
| }, context), | ||
| content, | ||
| popoverWidth: size === "s" ? 140 : size === "l" ? 191 : size === "xl" ? 192 : 166, // default value is "m" |
There was a problem hiding this comment.
I think it's valid that you're removing this but I looked into this for a bit and I'm having a hard time understanding why it was here to begin with. It looks like popoverWidth is used to control the width of the popover and align it with the popover width, but only if it's in certain popover positions. I would have thought it'd control the popover's width in combobox, but it seems like the popover position is such that it doesn't need the width (seems like we only use it in bottom-start position, which doesn't look at the width).
| isQuiet, | ||
| customClasses: [`${args.rootClass}-popover`], | ||
| customStyles: { | ||
| "inline-size": size === "s" ? "192px" : size === "l" ? "224px" : size === "xl" ? "240px" : "208px", |
There was a problem hiding this comment.
Ok, so this works to make the popover's width the same as the combobox textfield's, but it also doesn't work because we now get a horizontal scrollbar.
It actually looks pretty good if we remove the divider, so I'm wondering if that's the problem.
I'm wondering if a follow-up ticket is the best option here? It might involve adjustments to the Divider or Menu components' CSS, which feels out of scope here. What do you think?
components/helptext/index.css
Outdated
There was a problem hiding this comment.
we probably need to add this to the changeset since we're adjusting the CSS here
|
|
||
| --spectrum-help-text-to-component: 0px; |
There was a problem hiding this comment.
Was this token not working? It does appear to be in tokens correctly.
| @@ -40,6 +39,7 @@ | |||
| --spectrum-combobox-border-width: var(--spectrum-border-width-200); | |||
|
|
|||
| --spectrum-combobox-spacing-label-to-combobox: var(--spectrum-field-label-to-component); | |||
| --spectrum-combobox-spacing-side-label-to-field: var(--spectrum-spacing-200); | |||
There was a problem hiding this comment.
OH I see now this is the one that you mentioned on Friday was missing from the spec. Good call!
| /** | ||
| * Comboboxes have a read-only option for when content in the disabled state still needs to be shown. This allows for content to be copied, but not interacted with or changed. A combobox does not have a read-only option if no selection has been made. To enable this feature, add the `.isReadOnly` class to the combobox. To enable this feature, add the .isReadOnly class to the combobox. Then within the nested textfield component, add the .isReadOnly class and readonly attribute to the `<input>` element. | ||
| * Comboboxes have a read-only option for when content in the disabled state still needs to be shown. This allows for content to be copied, but not interacted with or changed. A combobox does not have a read-only option if no selection has been made. To enable this feature, add the `.is-readOnly` class to the combobox. To enable this feature, add the .isReadOnly class to the combobox. Then within the nested textfield component, add the .isReadOnly class and readonly attribute to the `<input>` element. |
There was a problem hiding this comment.
Just a nit putting .isReadOnly into code in a couple of places:
| * Comboboxes have a read-only option for when content in the disabled state still needs to be shown. This allows for content to be copied, but not interacted with or changed. A combobox does not have a read-only option if no selection has been made. To enable this feature, add the `.is-readOnly` class to the combobox. To enable this feature, add the .isReadOnly class to the combobox. Then within the nested textfield component, add the .isReadOnly class and readonly attribute to the `<input>` element. | |
| * Comboboxes have a read-only option for when content in the disabled state still needs to be shown. This allows for content to be copied, but not interacted with or changed. A combobox does not have a read-only option if no selection has been made. To enable this feature, add the `.is-readOnly` class to the combobox. To enable this feature, add the `.isReadOnly` class to the combobox. Then within the nested textfield component, add the `.isReadOnly` class and readonly attribute to the `<input>` element. |
| content = [], | ||
| value = "", | ||
| ...args | ||
| } = {}, context = {}) => { | ||
| const popoverHeight = size === "s" ? 106 : size === "l" ? 170 : size === "xl" ? 229 : 142; // default value is "m" | ||
|
|
There was a problem hiding this comment.
Do we need to set the popoverHeight for the docs page? It looks like the "extra-large" heading is running into the popover above it, this might need adjusting if the height has changed.
| autocomplete: { | ||
| table: { | ||
| disable: true, | ||
| }, |
There was a problem hiding this comment.
Is there a way to access autocomplete styles?
| } | ||
| } | ||
|
|
||
| .spectrum-Combobox { | ||
| position: relative; | ||
| display: inline-flex; | ||
| flex-flow: row nowrap; | ||
| display: inline-grid; | ||
|
|
||
| inline-size: var(--mod-combobox-inline-size, var(--spectrum-combobox-inline-size)); | ||
| min-inline-size: var(--mod-combobox-min-inline-size, var(--spectrum-combobox-min-inline-size)); |
There was a problem hiding this comment.
Here's another thing about combobox that I'm wondering about that I sort of touched on in another comment: Should the min-inline-size be set on .spectrum-Combobox or on .spectrum-Combobox-input? When we have a side label the input tends to shrink a lot, in this case it's about 78.5px. Since the minimum width token is a multiplier (2.5 * the height, which is 32px), it looks like for this particular size the combobox should be a minimum of 80px. Which it is, if we're talking about the field label + input width, but the input is smaller. I know this is how it was before, but was that what was intended?
| } | ||
|
|
||
| /* TEXT INPUT */ | ||
| .spectrum-Combobox-input { | ||
| background-color: var(--highcontrast-combobox-background-color-default, var(--mod-combobox-textfield-background-color, var(--spectrum-combobox-background-color-default))); |
There was a problem hiding this comment.
Ok, here's another thing that you didn't introduce in this PR that I'm wondering about:
.spectrum-Combobox-input and .spectrum-Textfield-input are the same element in combobox, right? But in some cases we use passthroughs by setting textfield mods to the tokens we want for combobox, and in other cases we're resetting/overriding the styles. In numberfield (#3681), Marissa used passthroughs wherever possible, but incorporating the infield buttons means that there were some things that just didn't work and she had to override some styles there. I think if anything, I'd recommend a similar approach here?
For instance I see that font-style, font-weight, and font-size don't use the mods for textfield, but are set on .spectrum-Combobox-input instead. Same with padding-block-start and padding-block-end, but we need a separate padding-inline-start and padding-inline-end so it seems ok to override padding-inline for textfield instead of using its mod.
And then background-color and line-height do have mods, but I'm not sure we can reach them from combobox because of where they were placed in the textfield css, so I think setting them on .spectrum-Combobox-input is the only option. 🤷♀️
I don't know what the right answer is since we can't use passthroughs for everything and sometimes we have to override styles, but it's confusing to set styles for the same element in two different places (here and up top in the passthroughs).
Combobox S2 Migration
The new combo box is coming in with more pronounced corner radius, thicker outline, and the use of the infield button nested inside. The quiet styling has also been deprecated.
New tokens
--spectrum-combobox-font-weight--spectrum-combobox-line-height-cjk--spectrum-combobox-spacing-alert-icon-to-text--spectrum-combobox-spacing-to-help-textNew mods
--mod-combobox-line-height-cjk--mod-combobox-popover-animation-distance--mod-combobox-spacing-alert-icon-to-text--mod-combobox-spacing-to-help-text--mod-combobox-textfield-background-colorHow 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
S1
S2
To-do list