Skip to content
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

fix(combobox): border color fast follows swc-582 #3609

Merged

Conversation

marissahuysentruyt
Copy link
Collaborator

@marissahuysentruyt marissahuysentruyt commented Mar 7, 2025

Description

Some design feedback on the s2 foundations combobox included border color fixes. This PR aims to address those!

  • In the default variant, focus+hover state (not shown in the testing grid):
    • border should resolve all the way to gray-900 (via --spectrum-combobox-border-color-focus-hover)

  • In the default variant, focus state:
    • border color should resolve to gray-800 (via --spectrum-combobox-border-color-focus)

  • In the default variant, keyboard focus state:
    • border should resolve to gray-800 (via --spectrum-combobox-border-color-key-focus)

  • In the default variant, disabled state:
    • the background color should resolve to gray-25 (by way of some textfield mods and --spectrum-combobox-background-color-disabled)
    • the border color should resolve to gray-300 (via some textfield mods and --spectrum-combobox-border-color-disabled)
    • the picker button border color should also resolve to gray-300 (via --spectrum-combobox-border-color-disabled and --spectrum-disabled-background-color)
    • the picker button background color should also resolve to gray-100 (via some picker mods and --spectrum-disabled-background-color)

Additionally, the style query in the spectrum.css file was --system: spectrum instead of --system: legacy, so that has been fixed as well.

Jira/Specs

SWC-582

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

@jawinn

  • Pull down the branch or visit the deploy preview
  • Visit the combobox testing grid (for easiest validating) for S2 foundations
  • Inspect the focused combobox, and turn on the hover state in your browser inspector (there is no test case for focus+hover, but that's what we want to validate in this PR)
  • Verify the border resolves to gray-900
  • Remove the forced hover state, and inspect the same element
  • Verify the border is gray-800
  • Inspect the keyboard focused combobox
  • Verify the border is also gray-800
  • Inspect the disabled combobox
  • Verify the combobox border is gray-300. This is aliased many times, through --spectrum-combobox-* properties, as well as --mod-textfield-* properties
  • Verify the combobox background color now resolves to gray-25
  • Verify the picker button border is also gray-300. This is another instance of several layers of picker button mods being defined by --spectrum-combobox-* properties
  • Verify the picker button background color is gray-100. Once more, this is another instance of picker button mods and using --spectrum-disabled-background-color.
  • Verify no regressions have occurred in the S1 context
    - [ ] Verify no regressions have occurred in the Express context
  • EDIT: Inspect the read-only combobox in the Express context.
  • Confirm that the .spectrum-Combobox-input border color resolves to gray-400 via --spectrum-combobox-readonly-input-border-color.

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.
  • 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 tested these changes in Windows High Contrast mode.
  • If my change impacts other components, I have tested to make sure they don't break.
  • ✨ This pull request is ready to merge. ✨

Sorry, something went wrong.

Copy link

changeset-bot bot commented Mar 7, 2025

🦋 Changeset detected

Latest commit: 5513713

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/combobox 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

Copy link
Contributor

github-actions bot commented Mar 7, 2025

File metrics

Summary

Total size: 2.25 MB*

Package Size Minified Gzipped
combobox 30.26 KB 29.19 KB 2.97 KB

combobox

Filename Head Minified Gzipped Compared to base
index-base.css 29.70 KB 28.65 KB 2.91 KB 🔴 ⬆ 0.25 KB
index-theme.css 1.35 KB 1.32 KB 0.53 KB 🔴 ⬆ 0.19 KB
index.css 30.26 KB 29.19 KB 2.97 KB 🔴 ⬆ 0.41 KB
metadata.json 16.30 KB - - 🔴 ⬆ 0.26 KB
themes/express.css 1.19 KB 1.16 KB 0.54 KB 🔴 ⬆ 0.13 KB
themes/spectrum-two.css 1.18 KB 1.15 KB 0.53 KB 🔴 ⬆ 0.15 KB
themes/spectrum.css 1.18 KB 1.16 KB 0.54 KB 🔴 ⬆ 0.13 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.

Copy link
Contributor

github-actions bot commented Mar 7, 2025

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

@marissahuysentruyt marissahuysentruyt self-assigned this Mar 7, 2025
@marissahuysentruyt marissahuysentruyt added size-1 XS ~1-6hrs; nearly trivial, a few hours, could do more than one in a single day. fast-follows An issue or PR that quickly follows a release labels Mar 7, 2025
@marissahuysentruyt marissahuysentruyt marked this pull request as ready for review March 10, 2025 13:40
@marissahuysentruyt
Copy link
Collaborator Author

TODO: run VRTs

@marissahuysentruyt marissahuysentruyt marked this pull request as draft March 10, 2025 13:41
@marissahuysentruyt marissahuysentruyt force-pushed the marissahuysentruyt/swc-582-combobox-fast-follows branch from cb419de to 73f4770 Compare March 10, 2025 18:55
@marissahuysentruyt marissahuysentruyt force-pushed the marissahuysentruyt/swc-582-combobox-fast-follows branch 2 times, most recently from 550b3f8 to 78e1588 Compare March 11, 2025 12:12
@marissahuysentruyt marissahuysentruyt marked this pull request as ready for review March 11, 2025 12:17
@marissahuysentruyt marissahuysentruyt force-pushed the marissahuysentruyt/swc-582-combobox-fast-follows branch 2 times, most recently from f69bc97 to 33f2b97 Compare March 11, 2025 13:18
@marissahuysentruyt marissahuysentruyt added size-2 S ~6-18hrs; not hard or time consuming, one or two work days to complete. ready-for-review labels Mar 11, 2025

Fast follow fixes for combobox

- corrects style query for the `--system` reference to "legacy" in the combobox/themes/spectrum.css file
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

container query 🤦‍♀️

Copy link
Collaborator

Choose a reason for hiding this comment

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

Good catch! I did a quick search for --system: spectrum and found one more instance of this in a spectrum.css file for alert banner. We should fix that too! Since it's a tiny related fix, I don't see a problem adding a commit and a separate changeset, but that could be a separate PR too.

Copy link
Collaborator Author

@marissahuysentruyt marissahuysentruyt Mar 14, 2025

Choose a reason for hiding this comment

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

@jawinn I'll have another PR to fix alert banner!! 🎉

@marissahuysentruyt marissahuysentruyt removed the size-2 S ~6-18hrs; not hard or time consuming, one or two work days to complete. label Mar 11, 2025
@castastrophe castastrophe force-pushed the main branch 9 times, most recently from 9dcf26f to c68f4e3 Compare March 12, 2025 21:47
Copy link
Collaborator

@jawinn jawinn left a comment

Choose a reason for hiding this comment

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

This looks great! Thanks for the thorough validation instructions. I just had one question about the read-only diff for Express before approving.


Fast follow fixes for combobox

- corrects style query for the `--system` reference to "legacy" in the combobox/themes/spectrum.css file
Copy link
Collaborator

Choose a reason for hiding this comment

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

Good catch! I did a quick search for --system: spectrum and found one more instance of this in a spectrum.css file for alert banner. We should fix that too! Since it's a tiny related fix, I don't see a problem adding a commit and a separate changeset, but that could be a separate PR too.

@@ -22,5 +22,7 @@
--spectrum-combobox-border-color-focus: var(--spectrum-gray-900);
--spectrum-combobox-border-color-focus-hover: var(--spectrum-gray-800);
--spectrum-combobox-border-color-key-focus: var(--spectrum-gray-900);

--spectrum-combobox-readonly-input-border-color: var(--spectrum-gray-400);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I see one Express difference in the VRT for this change in the read-only border color. I don't see this one mentioned in the Jira or PR/validation; was this change intended/discussed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh shoot, I forgot to add that to the validation! Great catch. I did mean to make this change, but it was not discussed.

I stumbled on this by chance. As I was going through a bunch of border colors, I saw the default border color was gray-500, and the read-only border color was gray-500 in both S1 and S2 contexts. The default express.css border color was gray-400, but because the read-only input variable wasn't defined, I noticed it was still rendering at gray-500. It seemed like the intention was the keep the same border color for read-only.

I can add the read-only validation to the PR description if you're good with that logic!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Here's what is on production currently:
Screenshot 2025-03-13 at 5 19 57 PM
Screenshot 2025-03-13 at 5 20 03 PM

That just didn't seem right, compared to S1 & S2.

Copy link
Collaborator

@jawinn jawinn Mar 13, 2025

Choose a reason for hiding this comment

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

That makes sense that it should match the default like the other themes (and not be darker than default). I'd add to the changeset to document that bug fix for Express, including the border color that is changing.

Copy link
Collaborator

@5t3ph 5t3ph 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! I validated the read-only in Express as well

- add mods for s2 foundations disabled picker button BG/border colors
- correct disabled+read-only border color
- add read-only border custom property
- update metadata.json
@marissahuysentruyt marissahuysentruyt force-pushed the marissahuysentruyt/swc-582-combobox-fast-follows branch from a56cc7e to 5513713 Compare March 14, 2025 17:49
@marissahuysentruyt marissahuysentruyt added the run_vrt For use on PRs looking to kick off VRT label Mar 14, 2025
@marissahuysentruyt marissahuysentruyt merged commit 851be13 into main Mar 14, 2025
18 of 24 checks passed
@marissahuysentruyt marissahuysentruyt deleted the marissahuysentruyt/swc-582-combobox-fast-follows branch March 14, 2025 18:11
@github-actions github-actions bot mentioned this pull request Mar 14, 2025
castastrophe added a commit that referenced this pull request Mar 20, 2025
* feat(actionbutton): use s2 colors in spectrum-two theme (#3606)
* feat(actionbutton): use closer to s2 colors in spectrum-two theme

Requested colors update for action button, aligning the colors closer
to the S2 design, post-foundations.

In the foundations spectrum-two theme:
- Removes the border
- Changes the default background colors to match s2 specs
- Updates the background colors used for static black and static white

SWC-497

* fix(actionbutton): fix high contrast styles for selected disabled

The selected + disabled button was not showing up as the disabled colors
in high contrast mode. Fixed by adjusting the source order slightly
in the high contrast media query so disabled is after selected and takes
precedence.

* fix(search): update disabled state in spectrum two (#3593)

Co-authored-by: rise-erpelding <54716846+rise-erpelding@users.noreply.github.com>
Co-authored-by: [ Cassondra ] <castastrophe@users.noreply.github.com>

* chore(deps): bump the npm_and_yarn group with 2 updates (#3618)

Bumps the npm_and_yarn group with 2 updates: [@babel/helpers](https://github.com/babel/babel/tree/HEAD/packages/babel-helpers) and [@babel/runtime](https://github.com/babel/babel/tree/HEAD/packages/babel-runtime).


Updates `@babel/helpers` from 7.26.0 to 7.26.10
- [Release notes](https://github.com/babel/babel/releases)
- [Changelog](https://github.com/babel/babel/blob/main/CHANGELOG.md)
- [Commits](https://github.com/babel/babel/commits/v7.26.10/packages/babel-helpers)

Updates `@babel/runtime` from 7.24.4 to 7.26.10
- [Release notes](https://github.com/babel/babel/releases)
- [Changelog](https://github.com/babel/babel/blob/main/CHANGELOG.md)
- [Commits](https://github.com/babel/babel/commits/v7.26.10/packages/babel-runtime)

---
updated-dependencies:
- dependency-name: "@babel/helpers"
  dependency-type: indirect
  dependency-group: npm_and_yarn
- dependency-name: "@babel/runtime"
  dependency-type: indirect
  dependency-group: npm_and_yarn
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* chore: update release script install settings

* fix(button): adjust s2 static colors [SWC-496] (#3613)

* chore: release (#3619)

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

* fix(slider): corrects contrast bug caused by template default arg (#3614)

* fix(stepper): fast follows for focus/focus+hover/keyboardFocus borders (#3621)

* fix(stepper): focus/focus+hover/keyboardFocus border colors

* chore(stepper): add changeset

* fix(slider): offset variant border radius bug fix (#3611)

* feat(slider): offset variant border radius bug fix

* feat(slider): fix range slider

* fix(combobox): border color fast follows swc-582  (#3609)

* fix(combobox): correct s1/legacy container variable

* fix(combobox): fast follow border color remapping
- add mods for s2 foundations disabled picker button BG/border colors
- correct disabled+read-only border color
- add read-only border custom property
- fixes express read-only border from gray-500 to gray-400
- update metadata.json

* chore(combobox): create changeset

* chore: release (#3623)

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

* chore: patch tj-actions vulnerability (#3626)

* fix(alertbanner): change system variable from spectrum to legacy (#3624)

* fix(alertbanner): change system: spectrum to legacy
* chore(alertbanner): create changeset

* test(checkbox): add more coverage for checkbox (#3625)

* chore(checkbox): add isHovered state to checkbox

- adds isHovered shared type and control to checkbox stories
- adds several isHovered test cases
- updates checkbox template to include isHovered arg

* chore(form): fix the fieldgroup component input and labels

* chore: release (#3631)

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

* fix(checkbox): add invalid+checked+hover checkbox styles (#3617)

- add missing ::before pseudo to target the before element in the
invalid/checked/hover state
- update metadata.json
- create changeset

* chore: release (#3632)

* chore: release

---------

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

* fix: undefined and duplicated variable after merging main

fix(slider): remove duplicated values

Remove a large number of duplicate values causing stylelint
"Unexpected duplicate" warnings. It looks like things got doubled up
somehow in a previous rebase or merge. This included duplicate t-shirt
size classes.

Also moves root styles block under the custom property definitions to be
consistent with other components.

fix(combobox): fixes undefined and duplicated values

---------

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: TaraT <ttomar@adobe.com>
Co-authored-by: rise-erpelding <54716846+rise-erpelding@users.noreply.github.com>
Co-authored-by: [ Cassondra ] <castastrophe@users.noreply.github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Cory Dransfeldt <cdransfeldt@adobe.com>
Co-authored-by: Marissa Huysentruyt <69602589+marissahuysentruyt@users.noreply.github.com>
Co-authored-by: aramos-adobe <abdulr@adobe.com>
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 run_vrt For use on PRs looking to kick off VRT size-1 XS ~1-6hrs; nearly trivial, a few hours, could do more than one in a single day.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants