-
Notifications
You must be signed in to change notification settings - Fork 34
feat(button-group): add <rh-button-group> element
#2605
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: staging/eevee
Are you sure you want to change the base?
feat(button-group): add <rh-button-group> element
#2605
Conversation
🦋 Changeset detectedLatest commit: 7b9a33f 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 |
✅ Deploy Preview for red-hat-design-system ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
@adamjohnson please give this a ux review @coreyvickery you mentioned in internal chat some design guidelines, if it's not too much trouble, please add (publically accessible) links to those design files here |
|
I just grabbed spacing guidance from PatternFly 6. And here are their spacer token values. |
|
@bennypowers Should any docs updates be handled in this issue? |
|
@bennypowers @markcaron Does this need to be a separate element? Can it be a part of the |
|
@coreyvickery consider these examples (final attributes may change): <rh-button-group>
<rh-button>Shutdown</rh-button>
<rh-button>Restart</rh-button>
<rh-button>Logout</rh-button>
</rh-button-group>
<rh-button-group variant="radio">
<rh-button>Dark</rh-button>
<rh-button>Light</rh-button>
<rh-button>System</rh-button>
</rh-button-group>
<rh-button-group>
<rh-menu-dropdown>...</rh-menu-dropdown>
<rh-button>Manage Account</rh-button>
</rh-button-group>button group is a container for buttons |
|
Got it. Docs are in progress! 🙂 |
|
Thank you for the detailed review. |
|
@HadassahYelenik I can't see the demo, please fix when you see this so I can complete the docs. |
|
Some notes (not necessarily directly related to this pr) for your consideration which came from a meeting we had earlier. In a future version of Button Group, we hope to implement split dropdown in Button Group. I. E. split button where one of the buttons opens a dropdown. Although we're not sure we're going to be able to do it on this first round. |
|
@coreyvickery thanks for letting me know, I’m looking into the issue now and will update the branch once it’s fixed. |
coreyvickery
left a comment
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.
@HadassahYelenik Some changes requested:
- Space between buttons in a row is
--rh-space-lg(16px) - Space between buttons in a column is
--rh-space-md(8px) - For more information, please read through the docs
|
@adamjohnson Docs are here if you'll be working on them. |
@HadassahYelenik should be able to handle coding the docs up. Let us know if you have any questions. Reference other elements/files ala elements/rh-/docs/ to see the coding patterns we use. |
bd39b83 to
d8026ce
Compare
|
Fixed the CSS lint errors in rh-button-group.css. The remaining failures are unrelated — existing ESLint errors in _plugins and a Netlify build issue from the @patternfly/pfe-tools patch. All button group files are passing now. |
bennypowers
left a comment
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.
Please change the PR title to
feat(button-group): add `<rh-button-group>` element
<rh-button-group> element
|
@HadassahYelenik Below is the image for the The body text is:
cc @adamjohnson If you need to do this instead. |
Screenshot added to /elements/rh-button-group/docs/screenshot.png. Thanks! I see the body text would go in index.njk, so I’ll leave that part for @adamjohnson unless told otherwise. |
47f1198 to
db049e5
Compare
adamjohnson
left a comment
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.
Some things to think about:
- Possible API changes around
role="group"androle="toolbar" - Updates to docs imagery and content
- Possible additon of an orientation demo
- Possible changes to tests.
Also, instead of having the base branch based on main (or RedHat-UX:main), it should be based off of staging/eevee.
| </figure> | ||
|
|
||
| ### Orientation | ||
|
|
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.
| Button groups are available in both horizontal and vertical orientations. | |
Check the white space here, sometimes it's hard to get right when commenting on Github.
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.
Also, if the docs are calling out "vertical orientations", that likely means we'll have to properly handle aria-orientation and include demos for a vertical orientation too. (See discussion above).
| @@ -0,0 +1,17 @@ | |||
| <rh-button-group role="group"> | |||
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.
Instead of adding role="group" (and with regard to the considerations in the discussion a few lines below this one), could we handle this with the InternalsController? We generally use the InternalsController to define semantics like this. eg rh-navigation-link.
If we need to change the role, rh-icon has an example of that. That said, I don't know if that will be necessary if we decide to make <rh-button-group-toolbar> (or whatever it would be named).
elements/rh-button-group/README.md
Outdated
| <rh-button-group role="group"> | ||
| <button type="button">Save</button> | ||
| <button type="button">Cancel</button> | ||
| <button type="button">Delete</button> | ||
| </rh-button-group> |
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.
This example will likely change as we make API changes as discussed below.
| <rh-button-group role="group"> | ||
| <rh-button>Save</rh-button> | ||
| <rh-button>Cancel</rh-button> | ||
| <rh-button>Delete</rh-button> | ||
| </rh-button-group> |
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.
This example will likely change as we make API changes as discussed below.
| it('should set tabindex=0 for group role', async function() { | ||
| const element = await createFixture<RhButtonGroup>(html` | ||
| <rh-button-group role="group"> | ||
| <rh-button>Save</rh-button> | ||
| <rh-button>Cancel</rh-button> | ||
| </rh-button-group> | ||
| `); | ||
| await element.updateComplete; | ||
|
|
||
| const buttons = element.querySelectorAll('rh-button'); | ||
| buttons.forEach(button => { | ||
| expect(button.getAttribute('tabindex')).to.equal('0'); | ||
| }); | ||
| }); | ||
|
|
||
| it('should handle role="toolbar" with roving tabindex', async function() { | ||
| const element = await createFixture<RhButtonGroup>(html` | ||
| <rh-button-group role="toolbar"> | ||
| <rh-button>Edit</rh-button> | ||
| <rh-button>Copy</rh-button> | ||
| <rh-button>Delete</rh-button> | ||
| </rh-button-group> |
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.
Keep in mind, these tests may change if we change the API, as discussed above.
- Fix themable decorator to use @themable pattern consistently with other RHDS elements - Refactor slot change handling with cleaner method separation - Improve event binding using declarative @slotchange in template - Add comprehensive test coverage for role-based tabindex behavior - Add tests for group vs toolbar role functionality - Add E2E tests for DOM rendering validation - Ensure proper accessibility and roving tabindex controller testing
Addresses PR feedback from @bennypowers
Per @adamjohnson's guidance on @coreyvicery's feedback.
db049e5 to
5c2f7c5
Compare
|
Thanks for the detailed feedback @adamjohnson! I understand that role="toolbar" should wrap multiple button groups, and that role="group" semantics are better handled internally via InternalsController. A wrapper like How would you like me to proceed: In the meantime, I’ve: |
|
…tion property
- Add InternalsController to automatically set role='group'
- Remove RovingTabindexController and toolbar-related logic
- Add orientation property ('horizontal' | 'vertical') with default 'horizontal'
- Add CSS support for vertical orientation with flex-direction: column
- Organize demos into separate pages: index.html, horizontal.html, vertical.html
- Update tests to follow rh-navigation-link pattern using @open-wc/testing
- Remove manual role attributes from README and changeset examples
|
Thanks @adamjohnson! |
relates to issue #2558
What I did
Created a new rh-button-group web component.
Added styling (rh-button-group.css) to handle button layout and spacing.
Built a demo page (demo/index.html) to showcase component usage.
Implemented TypeScript functionality (rh-button-group.ts) for component behavior.
Testing Instructions
Open elements/rh-button-group/demo/index.html in a browser.
Verify that the buttons display correctly in a group layout.
Click the buttons to ensure any interactions work as expected.
Optional: Integrate the component into a test page in the repo to see it in context.
Notes to Reviewers
This is a new component, so no breaking changes to existing functionality.
Styles and behavior are confined to rh-button-group.
Feedback welcome on styling, accessibility, and any API improvements.