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 outline reset of .SelectMenu-closeButton #2211

Merged
merged 2 commits into from
Aug 24, 2022
Merged

Remove outline reset of .SelectMenu-closeButton #2211

merged 2 commits into from
Aug 24, 2022

Conversation

imjohnbo
Copy link
Contributor

What are you trying to accomplish?

The current .SelectMenu-closeButton focus state is nearly hidden, even to the keenest of users. An outline state was recommended by @edokoa. This PR removes the outline reset to let keyboard users see an outline around .SelectMenu-closeButton when focused.

Before

selectmenu_closebutton_focus.mp4

After

selectmenu_closebutton_focus_fixed.mp4

What approach did you choose and why?

Removing the outline as suggested by @langermank 🙏

What should reviewers focus on?

Nothing especially tricky! 😄

Can these changes ship as is?

  • Yes, this PR does not depend on additional changes. 🚢

@imjohnbo imjohnbo requested a review from a team as a code owner August 23, 2022 18:52
@imjohnbo imjohnbo requested a review from jonrohan August 23, 2022 18:52
@changeset-bot
Copy link

changeset-bot bot commented Aug 23, 2022

🦋 Changeset detected

Latest commit: 1d873fa

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

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

@simurai simurai left a comment

Choose a reason for hiding this comment

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

I think the "after video" shows each item also getting the blue outline, but the change seems only to affect the close button?

Screen.Recording.2022-08-24.at.16.52.46.mov

Anyways, I think that's good for now. Thanks. 🙇

@simurai simurai merged commit 8e5f622 into primer:main Aug 24, 2022
@primer-css primer-css mentioned this pull request Aug 24, 2022
@imjohnbo
Copy link
Contributor Author

Thanks @simurai!

I think the "after video" shows each item also getting the blue outline, but the change seems only to affect the close button?

Right you are. That was unintentional from devtool fiddling, but the change just merged should only affect SelectMenu-closeButtons. 🙇

@imjohnbo imjohnbo deleted the patch-1 branch August 24, 2022 13:07
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.

3 participants