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.

Sign up for GitHub

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

ActionList: Allow items to remain focusable when disabled #4481

Open
wants to merge 13 commits into
base: main
Choose a base branch
from
Open

ActionList: Allow items to remain focusable when disabled #4481

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
f59808e
Allow `ActionMenu` items, or `SelectPanel` items to be focusable when…
TylerJDev Apr 8, 2024
b06148f
Add changeset
TylerJDev Apr 8, 2024
a21013f
Merge branch 'main' into tylerjdev/action-menu-disabled-items
TylerJDev Apr 11, 2024
cd058e2
Merge branch 'main' into tylerjdev/action-menu-disabled-items
TylerJDev Apr 24, 2024
de8674b
Merge branch 'main' into tylerjdev/action-menu-disabled-items
siddharthkp May 8, 2024
5f55eb6
Merge branch 'main' into tylerjdev/action-menu-disabled-items
TylerJDev Jun 21, 2024
6ccbc76
Merge branch 'main' into tylerjdev/action-menu-disabled-items
TylerJDev Jul 12, 2024
77a20b9
Merge branch 'main' into tylerjdev/action-menu-disabled-items
TylerJDev Aug 5, 2024
f1a12d5
Update PR based on feedback
TylerJDev Aug 6, 2024
cb54689
Fix for test
TylerJDev Aug 6, 2024
0455742
Merge branch 'main' into tylerjdev/action-menu-disabled-items
TylerJDev Nov 5, 2024
227daab
Merge branch 'main' into tylerjdev/action-menu-disabled-items
TylerJDev Jan 8, 2025
39e8c3f
Merge branch 'main' into tylerjdev/action-menu-disabled-items
TylerJDev Jan 9, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/tall-ravens-hope.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@primer/react': patch
---

Allows `ActionMenu` and `SelectPanel` items to remain focusable when `disabled`
9 changes: 8 additions & 1 deletion packages/react/src/ActionList/Item.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -317,13 +317,20 @@ export const Item = React.forwardRef<HTMLLIElement, ActionListItemProps>(
const selectableRoles = ['menuitemradio', 'menuitemcheckbox', 'option']
const includeSelectionAttribute = itemSelectionAttribute && itemRole && selectableRoles.includes(itemRole)

let focusable

// if item is disabled and is of type (menuitem*, option) it should remain focusable, if inactive, apply the same rules
if ((disabled && !inferredItemRole) || showInactiveIndicator) {
focusable = true
}

const menuItemProps = {
onClick: clickHandler,
onKeyPress: !buttonSemantics ? keyPressHandler : undefined,
'aria-disabled': disabled ? true : undefined,
'data-inactive': inactive ? true : undefined,
'data-loading': loading && !inactive ? true : undefined,
tabIndex: disabled || showInactiveIndicator ? undefined : 0,
tabIndex: focusable ? undefined : 0,
Copy link
Member

Choose a reason for hiding this comment

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

Am i reading this wrong or does this feel opposite of what it says?

when focusable=true, we remove tabIndex with tabIndex: undefined and
when focusable=false, we set tabIndex to 0 (which would make it focusable?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is mainly adding an additional conditional and placing it in focusable.

(disabled && !inferredItemRole) || showInactiveIndicator instead of disabled || showInactiveIndicator

This change will only apply if the container is an ActionMenu or a SelectPanel, otherwise the functionality will remain the same. So if an item inside of an ActionMenu is disabled, it will receive 0.

Let me know if we should opt for a different name here, or if it's still confusing!

'aria-labelledby': `${labelId} ${slots.trailingVisual ? trailingVisualId : ''} ${
slots.inlineDescription ? inlineDescriptionId : ''
}`,
Expand Down
56 changes: 56 additions & 0 deletions packages/react/src/ActionMenu/ActionMenu.features.stories.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -282,3 +282,59 @@ export const Submenus = () => (
</ActionMenu.Overlay>
</ActionMenu>
)

export const DisabledItems = () => (
<ActionMenu>
<ActionMenu.Button>Open menu</ActionMenu.Button>
<ActionMenu.Overlay width="auto">
<ActionList>
<ActionList.Item disabled={true}>
Workflows
<ActionList.LeadingVisual>
<WorkflowIcon />
</ActionList.LeadingVisual>
</ActionList.Item>
<ActionList.Item onSelect={() => alert('Archived items clicked')}>
Archived items
<ActionList.LeadingVisual>
<ArchiveIcon />
</ActionList.LeadingVisual>
</ActionList.Item>
<ActionList.LinkItem href="/">
Settings
<ActionList.LeadingVisual>
<GearIcon />
</ActionList.LeadingVisual>
</ActionList.LinkItem>
<ActionList.Item disabled={true}>
Make a copy
<ActionList.LeadingVisual>
<CopyIcon />
</ActionList.LeadingVisual>
</ActionList.Item>
<ActionList.Divider />
<ActionList.Group>
<ActionList.GroupHeading>Github projects</ActionList.GroupHeading>
<ActionList.LinkItem href="/">
What&apos;s new
<ActionList.LeadingVisual>
<RocketIcon />
</ActionList.LeadingVisual>
</ActionList.LinkItem>
<ActionList.LinkItem href="/">
Give feedback
<ActionList.LeadingVisual>
<CommentIcon />
</ActionList.LeadingVisual>
</ActionList.LinkItem>
<ActionList.LinkItem href="/">
GitHub Docs
<ActionList.LeadingVisual>
<BookIcon />
</ActionList.LeadingVisual>
</ActionList.LinkItem>
</ActionList.Group>
</ActionList>
</ActionMenu.Overlay>
</ActionMenu>
)
Loading