ActionList: Allow items to remain focusable when disabled #4481
Conversation
🦋 Changeset detectedLatest commit: 39e8c3f The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
size-limit report 📦
|
siddharthkp
added
the
integration-tests: recommended
|
Should require only one change in Dotcom (a modification to a test), other than that it appears that the tests related to this change pass. |
That's great! It looks like we can only make the dotcom change after merging this PR, correct? |
|
@siddharthkp, yup that should be it! |
|
This PR should be ready for a review! cc: @primer/engineer-reviewers |
siddharthkp
changed the title
| 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, |
There was a problem hiding this comment.
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?)
There was a problem hiding this comment.
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!
|
Hi! This pull request has been marked as stale because it has been open with no activity for 60 days. You can comment on the pull request or remove the stale label to keep it open. If you do nothing, this pull request will be closed in 7 days. |
|
👋 Hi, this pull request contains changes to the source code that github/github depends on. If you are GitHub staff, we recommend testing these changes with github/github using the integration workflow. Thanks! |
TylerJDev
temporarily deployed
to
github-pages
GitHub Actions
Inactive
|
👋🏻 @TylerJDev just checking in here, as I see this one has been open for a while. What's the remaining effort to get this over the line? Is this likely to be a MAS C* blocker? |
|
Hey @lesliecdubs! The effort remaining is small. I paused on this temporarily, but should have enough capacity to get this through. I don't believe it will be a MAS*C blocker, but is definitely an improvement for usability & accessibility. |
|
Hi! This pull request has been marked as stale because it has been open with no activity for 60 days. You can comment on the pull request or remove the stale label to keep it open. If you do nothing, this pull request will be closed in 7 days. |
|
👋 Hi from github/github! Your integration PR is ready: https://github.com/github/github/pull/357125 |
|
🔴 golden-jobs completed with status |
github-actions
bot
added
integration-tests: failing
Allows
disableditems inActionMenuandSelectPanelto remain focusable. This stems from feedback we've received from the Accessibility Team.Changelog
Integration test PR: https://github.com/github/github/pull/324056
Passes with small modifications (minus flakey tests), requires only one change to a test in Dotcom.
Changed
ActionList.Itemto keep focus if it's within anActionMenuorSelectPanelcontext.Rollout strategy
Testing & Reviewing
Merge checklist