Skip to content

fix(search): change keyfocus border to gray-800 #3658

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

Merged
merged 1 commit into from
Apr 9, 2025

Conversation

rise-erpelding
Copy link
Collaborator

@rise-erpelding rise-erpelding commented Apr 7, 2025

Description

This ticket relates to S2 Foundations Fast Follows for Search. In design verification, it was determined that the border color should be adjusted to gray-800 in order to match the design spec for Textfield. This PR makes this change so that we can release a new version of search for adobe/spectrum-web-components#5157

SWC-576

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

  1. Open the preview link to look at Search in the Spectrum 2 context
    • Confirm expected border color changes: search textfield input border color is gray-800/rgb(41, 41, 41) on key-focus or mouse focus (without mouse hovering on the input)
    • Confirm other border colors remain as before:
      • gray-900/rgb(19, 19, 19) for focus + hover (either mouse focus or keyfocus) border color
      • gray-500/rgb(143, 143, 143) for default
      • gray-600/rgb(113, 113, 113) for hover
  2. Using the same preview link, look at Spectrum 1 context, confirm that there are no regressions in border colors, compare to prod as needed:
    • gray-500/rgb(144, 144, 144) for default
    • gray-600/rgb(109, 109, 109) for hover
    • gray-800/rgb(34, 34, 34) for mouse focus only (not hover)
    • gray-900/rgb(0, 0, 0) for key-focus (only)
    • gray-900/rgb(0, 0, 0) for focus + hover

Regression testing

Validate:

  1. The documentation pages for at least two other components are still loading, including:
  • The pages render correctly, are accessible, and are responsive.
  1. If components have been modified, VRTs have been run on this branch:
  • VRTs have been run and looked at. (To do)
  • Any VRT changes have been accepted (by reviewer and/or PR author), or there are no changes.

Screenshots

To-do list

  • I have read the contribution guidelines.
  • I have run VRTs (still todo)
  • ✨ This pull request is ready to merge. ✨

Copy link

changeset-bot bot commented Apr 7, 2025

🦋 Changeset detected

Latest commit: df67f34

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

This PR includes changesets to release 1 package
Name Type
@spectrum-css/search 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

@rise-erpelding rise-erpelding added skip_vrt Add to a PR to skip running VRT (but still pass the action) fast-follows An issue or PR that quickly follows a release labels Apr 7, 2025
Copy link
Contributor

github-actions bot commented Apr 7, 2025

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

Copy link
Contributor

github-actions bot commented Apr 7, 2025

File metrics

Summary

Total size: 2.24 MB*

Package Size Minified Gzipped
search 11.70 KB 11.17 KB 1.87 KB

search

Filename Head Minified Gzipped Compared to base
index-base.css 10.40 KB 9.93 KB 1.75 KB -
index-theme.css 2.43 KB 2.35 KB 0.62 KB -
index.css 11.70 KB 11.17 KB 1.87 KB -
metadata.json 7.87 KB - - -
themes/express.css 2.19 KB 2.12 KB 0.65 KB -
themes/spectrum-two.css 2.05 KB 1.99 KB 0.62 KB -
themes/spectrum.css 2.10 KB 2.04 KB 0.62 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 rise-erpelding marked this pull request as ready for review April 8, 2025 13:28
@rise-erpelding rise-erpelding added the size-1 XS ~1-6hrs; nearly trivial, a few hours, could do more than one in a single day. label Apr 8, 2025
@@ -20,5 +20,6 @@
--spectrum-search-background-color-disabled: var(--spectrum-disabled-background-color);
--spectrum-search-border-color-disabled: var(--spectrum-disabled-background-color);
--spectrum-search-background-color: var(--spectrum-gray-50);
--spectrum-search-border-color-key-focus: var(--spectrum-gray-900);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does express pulls in this file or from index.css?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct, express imports the spectrum.css file and will inherit changes made in Spectrum legacy. Therefore if S1 has no regressions, we probably shouldn't expect to see any in Express, although it never hurts to check!

Copy link
Collaborator

@marissahuysentruyt marissahuysentruyt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good! I went through Express as well, and things continue to look as they were. 👍

Just wanted to double check on the S2 colors- the border colors for the default & hover states in the search field shouldn't have changed correct? I'm seeing gray-500 and gray-600, which I believe is correct, but the validation says they should be gray-300 and gray-400 (which are S2 colors). Let me know, and then I'm happy to approve!

@rise-erpelding
Copy link
Collaborator Author

Looking good! I went through Express as well, and things continue to look as they were. 👍

Just wanted to double check on the S2 colors- the border colors for the default & hover states in the search field shouldn't have changed correct? I'm seeing gray-500 and gray-600, which I believe is correct, but the validation says they should be gray-300 and gray-400 (which are S2 colors). Let me know, and then I'm happy to approve!

Oh gosh I'm sorry! Yes I had the wrong colors in the validation. S2 spec says gray-300 and gray-400 and that must be why I wrote that but the Foundations QA Spec is 500/600. And in any case that's what we had before, so that's what we'd want. I updated the PR description.

Copy link
Collaborator

@marissahuysentruyt marissahuysentruyt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks-good

@rise-erpelding rise-erpelding force-pushed the rise-erpelding/search-ff-focus-border branch from 94b6905 to df67f34 Compare April 9, 2025 18:05
@rise-erpelding rise-erpelding merged commit e9fde67 into main Apr 9, 2025
12 checks passed
@rise-erpelding rise-erpelding deleted the rise-erpelding/search-ff-focus-border branch April 9, 2025 18:17
@github-actions github-actions bot mentioned this pull request Apr 9, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fast-follows An issue or PR that quickly follows a release ready-for-review size-1 XS ~1-6hrs; nearly trivial, a few hours, could do more than one in a single day. skip_vrt Add to a PR to skip running VRT (but still pass the action)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants