feat(search): s2 migration by mdt2 · Pull Request #2673 · adobe/spectrum-css

mdt2 · 2024-04-18T21:08:56Z

Description

I believe this is blocked until we get the S2 icons.

S2 migration of Search to align with design specs. On top of the migration work, this PR includes:

Removal of /themes
Removal of quiet variant
Increased Chromatic coverage
Minor mod adjustments
Updated migration guide notes

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

Storybook testing preview shows all variants (except for hover as we cant show that without adding a class)
Test WHCM in light and dark mode
Be sure to test hover while focused since there are specific tokens applied for this
Design validation

Regression testing

Validate:

The documentation pages for at least two other components are still loading, including:

The pages render correctly, are accessible, and are responsive.

If components have been modified, VRTs have been run on this branch:

VRTs have been run and looked at.
Any VRT changes have been accepted (by reviewer and/or PR author), or there are no changes.

To-do list

I have read the contribution guidelines.
I have updated relevant storybook stories and templates.
I have tested these changes in Windows High Contrast mode.
If my change impacts documentation, I have updated the documentation accordingly.
✨ This pull request is ready to merge. ✨

changeset-bot · 2024-04-18T21:09:00Z

🦋 Changeset detected

Latest commit: ebc11c5

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages

Name	Type
@spectrum-css/tokens	Patch
@spectrum-css/search	Major
@spectrum-css/preview	Patch

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 · 2024-04-18T21:11:20Z

🚀 Deployed on https://pr-2673--spectrum-css.netlify.app

github-actions · 2024-04-18T21:11:38Z

File metrics

Summary

Total size: 1.37 MB^*
Total change (Δ): 🔴 ⬆ 0.06 KB (0.00%)

Table reports on changes to a package's main file. Other changes can be found in the collapsed Details section below.

Package	Size	Minified	Gzipped	Δ
search	12.46 KB	11.95 KB	1.98 KB	🔴 ⬆ 1.29 KB

File change details

search

Filename	Head	Minified	Gzipped	Compared to base
index.css	12.46 KB	11.95 KB	1.98 KB	🔴 ⬆ 1.29 KB
metadata.json	7.34 KB	-	-	🔴 ⬆ 0.60 KB

tokens

Filename	Head	Minified	Gzipped	Compared to base
css/dark-vars.css	46.44 KB	-	-	-
css/global-vars.css	69.59 KB	-	-	🔴 ⬆ 0.03 KB
css/index.css	241.80 KB	-	-	🔴 ⬆ 0.03 KB
css/large-vars.css	40.81 KB	-	-	-
css/light-vars.css	46.43 KB	-	-	-
css/medium-vars.css	40.93 KB	-	-	-
json/tokens.json	380.07 KB	-	-	-

* Size is the sum of all main files for packages in the library.
* An ASCII character in UTF-8 is 8 bits or 1 byte.

rise-erpelding

The CSS for this is looking so much cleaner and the testing preview matches several of the others that we've done! ✨ Here are a few visual things I noticed when going through this PR that might need some changes:

The border for the disabled state is transparent but should have a gray color, it looks like there wasn't a --mod for border-color ever set on the disabled state for textfield:

I'm wondering if this might need an .is-keyboardFocused:hover selector, this hover state looks like it might be unintentional (it goes gray from black):

Other questions I have that were carryover from how the component was before:

Is there a way to preview the search field with the help text? I know there's a spec for spacing the help text from the component, and I see that it's being used in the code, but I didn't see a way to view it in Storybook.
Would it be helpful at all to have a control to show/not show the clear button? I was thinking if it wasn't showing by default I could check the pill-edge-to-text spacing but then I realized that anytime it'd be used, the text would always be the placeholder "Search" text and the spacing between the end of that text box and the edge of the text field is a bit irrelevant. But is there any reason why anyone might want to look at the search field without the clear button instead of it always being there?
In WHCM, there's a clear color difference between the text color of the placeholder text and the text color when there's a value, it was like this before, and it's probably not a big deal, but it seemed a little odd to me so I thought I'd call it out:

jawinn

One thing I am wondering about is the focus and focus indicator on the clear button. In our Storybook, you can tab focus the field and then tab focus the clear button, which shows a rounded focus indicator:

I didn't see a design for that focus on the clear button to reference, and also took a look at the SWC search, which doesn't have it tab focusable (the field clears with the Esc key it seems). Is the intention for us to not have it as a tab stop?

mdt2 · 2024-04-29T21:55:30Z

Thanks for the feedback! Some responses for Rise:

Disabled border color - Great callout. I've updated Textfield to improve this which will probably just be a patch change.
.is-keyboardFocused:hover - I've also added this, good catch.
Help text - Yeah totally! I added some help text sections to our Chromatic coverage, and you should see a control to show the help text and also the ability to change the text now.
Clear button - I went down a bit of a rabbit hole with this one trying to use useArgs() to only show this button when there's an input value. I wasn't able to get it working within a reasonable timebox and came to the same conclusion you did about the spacing.
WHCM placeholder color - I noticed this too. I think that using the disabled color for the placeholder text can help communicate that its placeholder text, so that seemed ok to me. Let me know if you have a different value you think would work better!

And a response for Josh:

Clear button focus - This feels like it's in that gray area of whether we would handle this or if it's an implementation detail 🤔 Seems like removing the focus on our end would be a Storybook-only adjustment and would look something like adding an arg to ClearButton to adjust aria-hidden and tab-index, is that what you were thinking? I can add a commit for that and see what you think.

rise-erpelding

This is mostly a pretty straightforward migration, and doesn't need a whole lot! There are some really nice additions/enhancements to the docs and testing template. The only remnants of the quiet variant in this component are in the changelog, which is expected. My notes:

We'll want to do a rebase on this to include the textfield migration
Definitely should clarify what we're ok with carrying over from textfield and what should be separately defined in search.
Questioning the end spacing on the input, the token not being used there, and the clear button spacing specs.
There was a small change in spec that we talked about separately where we want the keyfocus border color to be gray-800 instead of gray-900 to align with the colors of textfield... not a huge deal and we can wait for the "official" spec to change and follow up later if you think that's a better idea.
Last thing - what is the minimised variant? I assume it's just like a search icon that you click on and then the search field appears? Do you think we'd need to do anything with that?

rise-erpelding · 2025-04-04T19:50:09Z

+- chore(textfield): add border-color custom property mod
+  - Search needed access to the disabled border-color in textfield, so the `--mod-textfield-border-color-disabled` custom property was added.


Hmmm, I'd like to understand why we were doing this previously and whether or not we think we still need it.

There's no impact to the border color either way; disabled-border-color is the border token we want for both textfield and search, and in any case, it evaluates to gray-300 which is the default border color token anyway. But is it ok that the disabled search textfield border is being set by textfield and not search? Is there a situation where we'd need it to be set in search, for instance if someone wants a --mod for this specific to search?

(I have the same question about the focus indicator tokens but will formally ask that somewhere else.)

I'm not sure I'm totally following your question on the search border color. I do see this in all of the passthrough mods:

--mod-textfield-border-color-disabled: var(--mod-search-border-color-disabled, var(--spectrum-search-border-color-disabled));

Does that answer any of your questions? This seems to track with the pattern that was happening in this file, but I haven't come across many other migrations that seem to have done this same, mod-heavy approach. We might have to pair if I'm still not understanding.

rise-erpelding · 2025-04-04T19:56:32Z

-	--spectrum-search-focus-indicator-thickness: var(--spectrum-focus-indicator-thickness);
-	--spectrum-search-focus-indicator-gap: var(--spectrum-focus-indicator-gap);
-	--spectrum-search-focus-indicator-color: var(--spectrum-focus-indicator-color);


Oh, great, I was looking for a place to comment on this, and here you've already commented on it! 🙂

I did notice that they were missing from the CSS file. It feels kind of unnecessary to me also though. The only instance where I think we'd it explicitly redefined that I can think of would be if someone wanted to set focus indicator styles differently for search in comparison to textfield, but that seems massively like an edge case.

I don't have a problem with them being removed but could be convinced if someone else has a strong argument for doing so.

rise-erpelding · 2025-04-04T20:44:38Z

+		inputValue: {
+			name: "Display value and close button",
+			description: "When defined, this will display the input and the clear button within the search field.",


Could you set this arg to "" below (around L66) so the default doesn't say "Set string" or is there a specific reason you weren't?

The close button also doesn't go away if I empty the textfield once I've typed into it. I can see the value of having the close button and value text bound together in a single control but it does make things a little weird, although you could argue maybe not weird enough to matter? This feels similar to other components where we've had radio or select controls and once you've picked one of them you can't go back to the initial state without resetting the controls.

Good catch on the inputValue- thank you!

For the clear button....yeah that's a tricky one. If I use the inputValue controls, the clear button responds as I'm expecting- appearing when that inputValue is defined, and not appearing when inputValue is undefined. But there's a few issues if I actually use the component, like when I type into the search field itself, the button doesn't appear at all. I think you're right about this aspect of the component is stuck in that "resetting" issue we've found before.

Screen.Recording.2025-04-10.at.8.35.25.AM.mov

Should I leave an implementation note in the docs about the expected behavior? "Implementations should be sure that the clear button renders when an input value is defined, and does not render when that value is undefined" (or something like that?)

You could and while I think as a precedent we do provide some basic guidance about how the component should behave, the argument could also be made that we're not the one-stop-shop for explaining all the things about the component and that implementations should probably still reference designs if they have further questions. I'll leave this one up to you! It's ok as is as far as I'm concerned.

rise-erpelding · 2025-04-04T21:11:59Z

 	padding-block-start: calc(var(--mod-search-top-to-text, var(--spectrum-search-top-to-text)) - var(--mod-search-border-width, var(--spectrum-search-border-width)));
 	padding-block-end: calc(var(--mod-search-bottom-to-text, var(--spectrum-search-bottom-to-text)) - var(--mod-search-border-width, var(--spectrum-search-border-width)));
+	padding-inline-start: calc(var(--mod-search-edge-to-visual, var(--spectrum-search-edge-to-visual)) - var(--mod-search-border-width, var(--spectrum-search-border-width)) + var(--mod-search-icon-size, var(--spectrum-search-icon-size)) + var(--mod-search-text-to-icon, var(--spectrum-search-text-to-icon)));
+	padding-inline-end: calc(var(--mod-search-button-inline-size, var(--spectrum-search-button-inline-size)) - var(--mod-search-border-width, var(--spectrum-search-border-width)));


Is this the piece of padding between the text input and the edge?

I noticed we're not using component-pill-edge-to-text tokens here and I'm wondering why? Is it because we're trying to leave more space for the clear button?

But also the clear button isn't really spec'd inside of search at all, which feels odd to me, is there a reason for that, that you know of?

Yes- the padding-inline-start should add space between the value & placeholder text and the search icon, and then the padding-inline-end should give space between the value in the input and the clear button (that's when it's most noticeable I think).

The --spectrum-search-edge-to-visual should be set to the component pill edge to text tokens for each t-shirt size. But if you're asking why aren't we using that in the padding-inline-end, I don't have a good answer for that. I'll have to get back with you.

I don't know why the clear button isn't spec'd, but I can certainly reach out to design and ask. All I can infer from the designs is that the clear button is all the way at the end of the search container. That's how the component seems to be styled currently as well.

jawinn

Here are a couple more suggestions.

marissahuysentruyt · 2025-04-10T17:16:34Z

Last thing - what is the minimised variant? I assume it's just like a search icon that you click on and then the search field appears? Do you think we'd need to do anything with that?

@rise-erpelding I have CSS-1158 written up for the minimized variant. When I last heard from Kami and Lynn, the file/branch for that variant was corrupted so Kami was in the midst of recreating I believe. The minimized search is basically just a search icon-only button, and when clicked, would expand the field, if I remember correctly. I can share Slack threads in a DM if you'd like!

rise-erpelding · 2025-04-11T19:35:23Z

Last thing - what is the minimised variant? I assume it's just like a search icon that you click on and then the search field appears? Do you think we'd need to do anything with that?

@rise-erpelding I have CSS-1158 written up for the minimized variant. When I last heard from Kami and Lynn, the file/branch for that variant was corrupted so Kami was in the midst of recreating I believe. The minimized search is basically just a search icon-only button, and when clicked, would expand the field, if I remember correctly. I can share Slack threads in a DM if you'd like!

I did see that ticket at the last Backlog review, thanks!! No further details needed for now 🙂

rise-erpelding

Nothing else blocking for me!

This is minor, but I think the reviewdog stylelint comments might need addressing as I'm seeing those same lines underlined in my editor.

I think we discussed the keyfocus border color change somewhere at some point (we want it to be gray-800 like the mouse focus, not gray-900), I don't see that in here, but that's fine because the border colors match what's currently in the spec. Let's make a note to keep an eye out for a spec change here though since we were told unofficially that the value is wrong and we'll make the change then!

Nice work on this!

rise-erpelding · 2025-04-11T20:32:21Z

+		inputValue: {
+			name: "Display value and close button",
+			description: "When defined, this will display the input and the clear button within the search field.",


You could and while I think as a precedent we do provide some basic guidance about how the component should behave, the argument could also be made that we're not the one-stop-shop for explaining all the things about the component and that implementations should probably still reference designs if they have further questions. I'll leave this one up to you! It's ok as is as far as I'm concerned.

rise-erpelding · 2025-04-11T20:54:34Z

+	.spectrum-Search .spectrum-Search-textfield,
+	.spectrum-Search .spectrum-Search-textfield .spectrum-Search-input {
+		--highcontrast-search-color-default: CanvasText;
+		--highcontrast-search-color-hover: CanvasText;
+		--highcontrast-search-color-focus: CanvasText;
+		--highcontrast-search-color-disabled: GrayText;


This is what I'm seeing now, which looks great. I think rebasing with textfield addressed the placeholder text color issues.

* chore: move forced colors to bottom for cascade * feat: token value adjustments and design spec alignment - rearranges where some of the textfield mods are (passthroughs) - adds styles for focus+hover in testing - clarifies where the custom properties are in the files - adds some missing tokens for block-start/block-end - removes whitespace in some of the style and/or mod declarations - reimplements custom --spectrum-corner-rounding-full token in global.vars after rebase - removes selectors that are not needed for the border color in key- boardFocus+hover, and colors in mouseFocus+hover states * chore: previous versions for migration guide - removes a stray isQuiet arg from stories * chore: update component name to align with design spec - also updates component name case * docs(search): show new options - remove quiet variant - adds help text story - adds inputValue to control table and corresponding behavior - fixes Search icon - clarify clear button language - add chromatic test coverage - adds aria-label based on guidance page in template - adds headings to SearchOptions template containers - hotfix for textfield's displayLabel arg - adds documentation from guidance page - clarifies some descriptions for args - adds default undefined string to inputValue * fix: token fix and removal of unused, keyboardfocus hover * docs(clearbutton): option to remove from focus for searchfield - remove unnecessary variant control * chore: update stylelint ignore comments - add stylelint disable comments & descriptions * chore(search,tokens): add changeset * chore(searchfield): update metadata * fix(search): fix focus+hover border in WHCM

mdt2 commented Apr 18, 2024

View reviewed changes

Comment thread components/search/stories/search.stories.js Outdated

mdt2 requested review from castastrophe, jawinn, pfulton and rise-erpelding April 18, 2024 21:22

pfulton force-pushed the spectrum-two branch 2 times, most recently from f3dd9ff to 9931a8e Compare April 19, 2024 18:22

rise-erpelding requested changes Apr 19, 2024

View reviewed changes

Comment thread components/search/index.css Outdated

jawinn reviewed Apr 25, 2024

View reviewed changes

Comment thread components/search/stories/search.stories.js

castastrophe force-pushed the spectrum-two branch 5 times, most recently from 6dbc850 to ae7aedd Compare April 26, 2024 20:37

mdt2 force-pushed the mdt2/css-713-s2-search branch from 0871d58 to 2be2d4c Compare April 29, 2024 16:17

castastrophe reviewed Apr 30, 2024

View reviewed changes

Comment thread components/search/index.css Outdated

castastrophe reviewed Apr 30, 2024

View reviewed changes

Comment thread components/search/stories/search.stories.js Outdated

castastrophe reviewed Apr 30, 2024

View reviewed changes

Comment thread components/search/stories/search.stories.js Outdated

pfulton force-pushed the spectrum-two branch 2 times, most recently from e0e0fd2 to 698e904 Compare May 3, 2024 18:39

pfulton force-pushed the spectrum-two branch from f3cbc18 to 051071f Compare May 10, 2024 14:34

mdt2 added the blocked See description and comments for what is blocking this issue label May 29, 2024

castastrophe added the S2 Spectrum 2 label Jun 27, 2024

castastrophe force-pushed the spectrum-two branch 3 times, most recently from cdb180d to 27d01df Compare December 4, 2024 14:54

marissahuysentruyt force-pushed the mdt2/css-713-s2-search branch from c695845 to 589966f Compare March 25, 2025 13:11

marissahuysentruyt added ready-for-review and removed wip This is a work in progress, don't judge. labels Mar 28, 2025

rise-erpelding self-requested a review March 31, 2025 18:45

rise-erpelding requested changes Apr 4, 2025

View reviewed changes

jawinn suggested changes Apr 7, 2025

View reviewed changes

Comment thread .changeset/shaggy-schools-sing.md Outdated

Comment thread components/search/stories/template.js

Comment thread components/search/stories/search.test.js Outdated

Comment thread components/search/stories/search.stories.js Outdated

marissahuysentruyt force-pushed the mdt2/css-713-s2-search branch from 589966f to c9717c7 Compare April 10, 2025 13:29

github-actions Bot reviewed Apr 10, 2025

View reviewed changes

Comment thread components/search/index.css

marissahuysentruyt force-pushed the mdt2/css-713-s2-search branch from 0e892ed to 7b5ec61 Compare April 10, 2025 17:07

jawinn suggested changes Apr 11, 2025

View reviewed changes

Comment thread components/search/index.css Outdated

rise-erpelding approved these changes Apr 11, 2025

View reviewed changes

marissahuysentruyt force-pushed the mdt2/css-713-s2-search branch 2 times, most recently from 3e48b3f to 4eb0786 Compare April 14, 2025 14:35

jawinn approved these changes Apr 14, 2025

View reviewed changes

marissahuysentruyt force-pushed the mdt2/css-713-s2-search branch from 2852b95 to ebc11c5 Compare April 14, 2025 18:36

marissahuysentruyt merged commit 89a3f2b into spectrum-two Apr 14, 2025
12 checks passed

marissahuysentruyt deleted the mdt2/css-713-s2-search branch April 14, 2025 18:47

github-actions Bot mentioned this pull request Apr 14, 2025

chore: release (next) #3668

Merged

castastrophe mentioned this pull request Apr 15, 2025

feat(tokens,card): update to @adobe/spectrum-tokens@13.3.0 #3669

Merged

12 tasks

This was referenced May 16, 2025

chore(swatch,menu,swatchgroup): fix reverted metadata assets #3752

Merged

chore: merge latest updates from main #3777

Merged

castastrophe mentioned this pull request Jul 25, 2025

ci: chromatic runs for S2 [CSS-1233] #4070

Merged

3 tasks

castastrophe mentioned this pull request Aug 6, 2025

fix: minor linting updates #4113

Merged

11 tasks

This was referenced Aug 27, 2025

chore(popover): wrapper placement in story #4173

Closed

feat(storybook): chromatic a11y testing #4180

Closed

chore: updating lint-staged file inheritance order #4193

Closed

castastrophe mentioned this pull request Oct 22, 2025

feat(badge): remove modifiers from the API [SWC-1264] #4283

Merged

14 tasks

		- chore(textfield): add border-color custom property mod
		- Search needed access to the disabled border-color in textfield, so the `--mod-textfield-border-color-disabled` custom property was added.

Conversation

mdt2 commented Apr 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

How and where has this been tested?

Validation steps

Regression testing

To-do list

Uh oh!

changeset-bot Bot commented Apr 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🦋 Changeset detected

Uh oh!

github-actions Bot commented Apr 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Apr 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

File metrics

Summary

search

tokens

Uh oh!

Uh oh!

rise-erpelding left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jawinn left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mdt2 commented Apr 29, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rise-erpelding left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jawinn left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

marissahuysentruyt commented Apr 10, 2025

Uh oh!

rise-erpelding commented Apr 11, 2025

Uh oh!

Uh oh!

rise-erpelding left a comment

Choose a reason for hiding this comment

mdt2 commented Apr 18, 2024 •

edited

Loading

changeset-bot Bot commented Apr 18, 2024 •

edited

Loading

github-actions Bot commented Apr 18, 2024 •

edited

Loading

github-actions Bot commented Apr 18, 2024 •

edited

Loading

jawinn left a comment •

edited

Loading