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

Remove incorrect focus styles on ActionList items #3056

Merged
merged 5 commits into from
Mar 22, 2023
Merged

Conversation

langermank
Copy link
Contributor

@langermank langermank commented Mar 21, 2023

I don't think the first item in ActionMenu should be focused at all by default, but this at least fixes the design bug with it. Focus styles utilize the accent blue outline, while the background is reserved for hover and active states.

Closes # https://github.com/github/primer/issues/1455

Screenshots

Before:
image

After:
image

Merge checklist

  • Added/updated tests
  • Added/updated documentation
  • Tested in Chrome
  • Tested in Firefox
  • Tested in Safari
  • Tested in Edge

Take a look at the What we look for in reviews section of the contributing guidelines for more information on how we review PRs.

@langermank langermank requested review from a team and broccolinisoup March 21, 2023 01:10
@changeset-bot
Copy link

changeset-bot bot commented Mar 21, 2023

🦋 Changeset detected

Latest commit: 61123f8

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

This PR includes changesets to release 1 package
Name Type
@primer/react 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
Copy link
Contributor

github-actions bot commented Mar 21, 2023

size-limit report 📦

Path Size
dist/browser.esm.js 95.01 KB (-0.03% 🔽)
dist/browser.umd.js 95.55 KB (-0.03% 🔽)

@langermank langermank temporarily deployed to github-pages March 21, 2023 01:18 — with GitHub Actions Inactive
@github-actions github-actions bot temporarily deployed to storybook-preview-3056 March 21, 2023 01:18 Inactive
@github-actions github-actions bot temporarily deployed to storybook-preview-3056 March 21, 2023 01:18 Inactive
Copy link
Member

@broccolinisoup broccolinisoup left a comment

Choose a reason for hiding this comment

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

This change makes sense to me! I am thinking if there would be any cases that cause an issue because we don't have an visual indicator for the focus state for mouse users? 🤔
I can't think of anything though but just to double check.

@langermank
Copy link
Contributor Author

This change makes sense to me! I am thinking if there would be any cases that cause an issue because we don't have an visual indicator for the focus state for mouse users? 🤔 I can't think of anything though but just to double check.

Great question! That is not a requirement to my knowledge. That's why the focus-visible property is nice, as it allows us to only show a focus state if a keyboard is being used to navigate.

@primer primer bot temporarily deployed to github-pages March 21, 2023 15:27 Inactive
@github-actions github-actions bot temporarily deployed to storybook-preview-3056 March 21, 2023 15:28 Inactive
@broccolinisoup
Copy link
Member

This change makes sense to me! I am thinking if there would be any cases that cause an issue because we don't have an visual indicator for the focus state for mouse users? 🤔 I can't think of anything though but just to double check.

Great question! That is not a requirement to my knowledge. That's why the focus-visible property is nice, as it allows us to only show a focus state if a keyboard is being used to navigate.

Sounds good to me! 🚀

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

Successfully merging this pull request may close these issues.

None yet

3 participants