Skip to content

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

Merged
siddharthkp merged 5 commits into
mainfrom
fix-actionmenu-focused-divider
Jul 5, 2023
Merged

ActionMenu: Fix missing divider for first item when the menu is open with mouse#3476
siddharthkp merged 5 commits into
mainfrom
fix-actionmenu-focused-divider

Conversation

@siddharthkp

@siddharthkpsiddharthkp commented Jul 3, 2023

Copy link
Copy Markdown
Member

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)

BeforeAfter
Divider is missing on first itemDivider is present on first item

@siddharthkp
siddharthkp requested review from a team and mperrottiJuly 3, 2023 11:03
@changeset-bot

changeset-botBot commented Jul 3, 2023

Copy link
Copy Markdown

🦋 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
NameType
@primer/reactPatch

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

@siddharthkpsiddharthkp changed the title Fix ActionMenu focused dividerFix ActionMenu divider for the first item on openJul 3, 2023
@siddharthkp
siddharthkp requested review from broccolinisoup and langermank and removed request for mperrottiJuly 3, 2023 11:03
@siddharthkpsiddharthkp changed the title Fix ActionMenu divider for the first item on openActionMenu: Fix missing divider for the first item on openJul 3, 2023
@siddharthkpsiddharthkp changed the title ActionMenu: Fix missing divider for the first item on openActionMenu: Fix missing divider for first item when the menu is open with mouseJul 3, 2023
@siddharthkpsiddharthkp self-assigned this Jul 3, 2023
@github-actions

github-actionsBot commented Jul 3, 2023

Copy link
Copy Markdown
Contributor

size-limit report 📦

PathSize
dist/browser.esm.js101.8 KB (-0.01% 🔽)
dist/browser.umd.js102.34 KB (-0.01% 🔽)

@github-actions
github-actionsBot 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-actionsBot 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-actionsBot 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-actionsBot temporarily deployed to storybook-preview-3476 July 3, 2023 11:31 Inactive

@broccolinisoupbroccolinisoup left a comment

Copy link
Copy Markdown
Member

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 queueJul 5, 2023
Merged via the queue into main with commit 379d947Jul 5, 2023
@siddharthkp
siddharthkp deleted the fix-actionmenu-focused-divider branch July 5, 2023 14:02
@primer-cssprimer-css mentioned this pull request Jul 5, 2023
Sign up for freeto 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

@siddharthkp@broccolinisoup