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

ActionMenu: Fix missing divider for first item when the menu is open with mouse #3476

Merged
merged 5 commits into from
Jul 5, 2023

Conversation

siddharthkp
Copy link
Member

@siddharthkp siddharthkp commented Jul 3, 2023

Context:

  • When you open ActionMenu with mouse click, we focus the first item by default (this matched a11y guidance at the time)
  • In the past, we treated both focus and focus-visible the same and added a focus ring on the focused item
  • To avoid both a divider and focus ring at the same time, we have some logic to hide divider if a surrounding item has focus or focus-visible
  • In Remove incorrect focus styles on ActionList items #3056, we removed the focus ring on focus (keeping it only on focus-visible)
  • Without matching divider logic, we end up with the divider hidden on focus even when there is no ring (reported in [ActionList] showDividers bug #3461)

This pull request fixes that by removing divider logic for focus (still keeping it for focus-visible)

Before After
Divider is missing on first item Divider is present on first item

@siddharthkp siddharthkp requested review from a team and mperrotti July 3, 2023 11:03
@changeset-bot
Copy link

changeset-bot bot commented Jul 3, 2023

🦋 Changeset detected

Latest commit: 4ec29ea

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

@siddharthkp siddharthkp changed the title Fix ActionMenu focused divider Fix ActionMenu divider for the first item on open Jul 3, 2023
@siddharthkp siddharthkp requested review from broccolinisoup and langermank and removed request for mperrotti July 3, 2023 11:03
@siddharthkp siddharthkp changed the title Fix ActionMenu divider for the first item on open ActionMenu: Fix missing divider for the first item on open Jul 3, 2023
@siddharthkp siddharthkp changed the title ActionMenu: Fix missing divider for the first item on open ActionMenu: Fix missing divider for first item when the menu is open with mouse Jul 3, 2023
@siddharthkp siddharthkp self-assigned this Jul 3, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Jul 3, 2023

size-limit report 📦

Path Size
dist/browser.esm.js 101.8 KB (-0.01% 🔽)
dist/browser.umd.js 102.34 KB (-0.01% 🔽)

@github-actions github-actions bot temporarily deployed to storybook-preview-3476 July 3, 2023 11:11 Inactive
@siddharthkp siddharthkp temporarily deployed to github-pages July 3, 2023 11:11 — with GitHub Actions Inactive
@github-actions github-actions bot temporarily deployed to storybook-preview-3476 July 3, 2023 11:12 Inactive
@siddharthkp siddharthkp temporarily deployed to github-pages July 3, 2023 11:22 — with GitHub Actions Inactive
@github-actions github-actions bot temporarily deployed to storybook-preview-3476 July 3, 2023 11:23 Inactive
@siddharthkp siddharthkp temporarily deployed to github-pages July 3, 2023 11:31 — with GitHub Actions Inactive
@github-actions github-actions bot temporarily deployed to storybook-preview-3476 July 3, 2023 11:31 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.

Looks great to me 🚀
Also, thanks so much for the context! I love how you wrote it like a story 🤗

@siddharthkp siddharthkp added this pull request to the merge queue Jul 5, 2023
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.

[ActionList] showDividers bug
2 participants