-
Notifications
You must be signed in to change notification settings - Fork 645
Style KeybindingHint within Button context
#7143
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
base: main
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: fdaf12b 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 |
|
👋 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! |
KeybindingHint within Button context
|
👋 Hi from github/github-ui! Your integration PR is ready: https://github.com/github/github-ui/pull/6543 |
|
🟢 ci completed with status |
|
👋 Hi, there are new commits since the last successful integration test. If you are GitHub staff, test these changes with github/github-ui using the integration workflow. Or, apply the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds styling support for buttons containing KeybindingHint components as trailing visuals. The changes enable proper visual integration of keyboard shortcut hints within button components across all button variants and states.
Key Changes:
- Introduces
data-kbd-chordattribute to KeybindingHint components for targeted styling - Adds comprehensive button styling rules for keybinding hints across all variants (default, primary, danger, invisible) and states (rest, hover, active, disabled, inactive)
- Updates @primer/primitives dependency from 10.7.0 to 11.3.0 to support new design tokens
- Removes unnecessary stylelint disable comments from StateLabel and ProgressBar components
Reviewed Changes
Copilot reviewed 8 out of 45 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| packages/react/src/KeybindingHint/components/Chord.tsx | Adds data-kbd-chord attribute for CSS targeting |
| packages/react/src/Button/ButtonBase.module.css | Comprehensive styling for buttons with keybinding hints including padding adjustments and state-specific colors |
| packages/react/src/Button/Button.features.stories.tsx | Adds multiple story examples demonstrating keybinding hints with buttons |
| packages/react/src/KeybindingHint/KeybindingHint.examples.stories.tsx | Removes redundant DangerButton story |
| packages/react/src/StateLabel/StateLabel.module.css | Removes unnecessary stylelint disable comments |
| packages/react/src/ProgressBar/ProgressBar.module.css | Removes unnecessary stylelint disable comment |
| e2e/components/Button.test.ts | Adds VRT test cases for keybinding hint integration |
| package-lock.json | Updates @primer/primitives dependency |
| .playwright/snapshots/* | Adds visual regression test snapshots |
| .changeset/lovely-files-invent.md | Documents the minor version change |
| setIsLoading(true) | ||
| setTimeout(() => { | ||
| setIsLoading(false) | ||
| }, 1000000) |
Copilot
AI
Nov 7, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The timeout value of 1000000 milliseconds (approximately 16.7 minutes) appears to be excessively long for a loading state demonstration. This is likely unintentional. Consider using a more reasonable timeout value such as 2000-3000 milliseconds (2-3 seconds) for better user experience in the story.
Closes https://github.com/github/primer/issues/6070
Part of https://github.com/github/github-ui/issues/6414
Changelog
Buckle up!
Today,
KeybindingHintis used as atrailingVisualfor buttons. The component has it's own set of variants likeonPrimarywhich is only used once in dotcom and is only ever intended to be used in Button. I think this is generally problematic but my goal is to find a solution to the visual problem without having to do much reconstruction for now 😄Since button has state the current implementation is sometimes broken, especially in danger buttons. I added new tokens specifically to handle this and rather than adding it to the
Chordcomponent I'm nesting it within the button context where it belongs. Now, when someone passes it intoButtonthey don't need to set a variant (and if they do nothing will happen.)I specifically added a data attribute instead of CSS class so that I could impact existing implementations and not add a new slot prop to
Button.New
KeybindingHintrenders inside a buttonChanged
ChordcomponentRemoved
Rollout strategy
Testing & Reviewing
Merge checklist