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

chore(menu): remove keyboard focus outline in spectrum-two #3595

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

TarunAdobe
Copy link
Contributor

@TarunAdobe TarunAdobe commented Mar 3, 2025

Description

removed keyboard focus outline in spectrum-two

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 storybook for the menu component:
  • Verify that keyboard focused menu item has no blue color ring. [@cdransf]

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

Old:
Screenshot 2025-03-03 at 2 32 56 PM
New:
Screenshot 2025-03-03 at 2 33 10 PM

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 other components, I have tested to make sure they don't break.
  • If my change impacts documentation, I have updated the documentation accordingly.
  • ✨ This pull request is ready to merge. ✨

Copy link

changeset-bot bot commented Mar 3, 2025

🦋 Changeset detected

Latest commit: 47f17ec

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/menu 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 3, 2025

File metrics

Summary

Total size: 2.25 MB*

Package Size Minified Gzipped
menu 41.72 KB 39.69 KB 4.50 KB

menu

Filename Head Minified Gzipped Compared to base
index-base.css 41.02 KB 39.02 KB 4.41 KB -
index-theme.css 1.36 KB 1.33 KB 0.55 KB -
index.css 41.72 KB 39.69 KB 4.50 KB 🟢 ⬇ < 0.01 KB
metadata.json 20.47 KB - - -
themes/express.css 1.25 KB 1.22 KB 0.57 KB -
themes/spectrum-two.css 1.29 KB 1.26 KB 0.58 KB 🟢 ⬇ < 0.01 KB
themes/spectrum.css 1.25 KB 1.22 KB 0.57 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 3, 2025

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

@TarunAdobe TarunAdobe requested a review from a team March 3, 2025 09:03
@cdransf cdransf self-requested a review March 3, 2025 18:20
Copy link
Member

@cdransf cdransf left a comment

Choose a reason for hiding this comment

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

Behaves as expected when running through validation. ✨

@@ -23,6 +23,6 @@
--spectrum-menu-item-focus-indicator-shadow: none;
--spectrum-menu-item-focus-indicator-offset: var(--spectrum-spacing-50);
--spectrum-menu-item-spacing-multiplier: 1;
--spectrum-menu-item-focus-indicator-outline-style: solid;
--spectrum-menu-item-focus-indicator-outline-style: none;
Copy link
Collaborator

@marissahuysentruyt marissahuysentruyt Mar 3, 2025

Choose a reason for hiding this comment

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

Do we want to remove the focus indicator styles completely? The menu S2 token specs have the focus indicator, so is there a better solution? You may have already double checked with design and they're fine with its removal already! I wanted to mention it since it seems further away from S2 than I was expecting.

Screenshot 2025-03-03 at 1 39 37 PM

I also might just not understand why we need this change!

"@spectrum-css/menu": patch
---

removed keyboard focus outline in spectrum-two
Copy link
Collaborator

@jawinn jawinn Mar 3, 2025

Choose a reason for hiding this comment

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

Can you please provide information in the PR description and changeset about why this change is being made? This is a pretty big change that affects accessibility, and needs to be communicated and validated. Peeking at the Spectrum 2 specs for menu, it does continue to have a focus indicator.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants