Skip to content

Conversation

@TylerJDev
Copy link
Member

@TylerJDev TylerJDev commented Feb 6, 2025

https://github.com/github/primer/issues/4613

Changelog

Changed

  • Improves disabled support on ActionBar.IconButton
  • Ensures that the overflow menu respects the disabled state

Rollout strategy

  • Patch release
  • Minor release
  • Major release; if selected, include a written rollout or migration plan
  • None; if selected, include a brief description as to why

Testing & Reviewing

Merge checklist

@changeset-bot
Copy link

changeset-bot bot commented Feb 6, 2025

🦋 Changeset detected

Latest commit: b626615

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 Minor

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

@github-actions
Copy link
Contributor

github-actions bot commented Feb 6, 2025

👋 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!

@github-actions github-actions bot added the integration-tests: recommended This change needs to be tested for breaking changes. See https://arc.net/l/quote/tdmpakpm label Feb 6, 2025
@github-actions
Copy link
Contributor

github-actions bot commented Feb 6, 2025

size-limit report 📦

Path Size
packages/react/dist/browser.esm.js 105.91 KB (-0.03% 🔽)
packages/react/dist/browser.umd.js 106.35 KB (+0.05% 🔺)

@TylerJDev
Copy link
Member Author

@andrejusk, quick implementation question! Are you expecting the IconButton not to be actionable when disabled, or are you handling this in your implementation? Right now we're using aria-disabled, which is more accessible and follows the same pattern we use in the overflow menu. This means that users can still click/trigger the button even when disabled. We could block onClick events when aria-disabled is true, but wanted to hear about your use case first.

@github-actions github-actions bot temporarily deployed to storybook-preview-5666 February 7, 2025 00:06 Inactive
@andrejusk
Copy link
Member

👋 @TylerJDev our current implementation always sets the onClick prop. I think what you propose makes sense, where the user can interact with the button and see that it is disabled, but triggering it does nothing.

@TylerJDev TylerJDev force-pushed the tylerjdev/add-disabled-to-actionbar branch from f7125a9 to 9cf870a Compare March 2, 2025 16:54
@github-actions github-actions bot requested a deployment to storybook-preview-5666 March 2, 2025 17:05 Abandoned
@github-actions github-actions bot temporarily deployed to storybook-preview-5666 March 2, 2025 17:18 Inactive
Comment on lines 266 to 274
const clickHandler = useCallback(
(event: React.MouseEvent<HTMLButtonElement>) => {
if (disabled) {
return
}
onClick?.(event)
},
[disabled, onClick],
)
Copy link
Member Author

Choose a reason for hiding this comment

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

Added this to block the onClick that a consumer might attach to the button. The button could still be activated on an onKeyDown event, but I don't think that's as likely to be added here.

Curious on any thoughts on this method, and if it just be adjusted or not 👀

Copy link
Member

Choose a reason for hiding this comment

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

Should we still add a keydown eventhandler then? 🤔

Copy link
Member Author

@TylerJDev TylerJDev Mar 7, 2025

Choose a reason for hiding this comment

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

After doing a bit more testing, I realize we probably don't need to add one for keydown. onClick will handle most cases that we'd expect when utilizing a keyboard. We also probably don't want consumers using onKeyDown instead of onClick. I adjusted the tests to reflect this!

@TylerJDev TylerJDev marked this pull request as ready for review March 4, 2025 16:54
@TylerJDev TylerJDev requested a review from a team as a code owner March 4, 2025 16:54
@TylerJDev TylerJDev requested a review from francinelucca March 4, 2025 16:54
Comment on lines 266 to 274
const clickHandler = useCallback(
(event: React.MouseEvent<HTMLButtonElement>) => {
if (disabled) {
return
}
onClick?.(event)
},
[disabled, onClick],
)
Copy link
Member

Choose a reason for hiding this comment

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

Should we still add a keydown eventhandler then? 🤔

@TylerJDev
Copy link
Member Author

Should we still add a keydown eventhandler then? 🤔

Doesn't hurt! I can go ahead and add it just in case

@github-actions github-actions bot temporarily deployed to storybook-preview-5666 March 6, 2025 15:28 Inactive
@github-actions github-actions bot requested a deployment to storybook-preview-5666 March 7, 2025 04:59 Abandoned
@TylerJDev TylerJDev added this pull request to the merge queue Mar 7, 2025
Merged via the queue into main with commit 7400549 Mar 7, 2025
45 checks passed
@TylerJDev TylerJDev deleted the tylerjdev/add-disabled-to-actionbar branch March 7, 2025 05:40
@primer primer bot mentioned this pull request Mar 7, 2025
hectahertz pushed a commit that referenced this pull request Mar 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

integration-tests: recommended This change needs to be tested for breaking changes. See https://arc.net/l/quote/tdmpakpm

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants