Repository navigation
chore: Styling v2 for button dropdown and button group - #5187
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5187 +/- ##
==========================================
- Coverage 97.73% 97.73% -0.01%
==========================================
Files 996 998 +2
Lines 32219 32252 +33
Branches 11906 11924 +18
==========================================
+ Hits 31490 31522 +32
+ Misses 723 685 -38
- Partials 6 45 +39 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
579232a to
7ac7242
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Upload focus rings ignore the new styling tokens, and the committed Style API documentation snapshots need updating.
2 open issues
What changed in this PR
Extends Style API v2 support to ButtonDropdown, ButtonGroup, and Divider, reusing the existing button and dropdown styling infrastructure.
Changes:
- Adds styling slots and forwards classes through button-group item types.
- Adds divider tokens and renames the internal dropdown slot to
container. - Adds slot-forwarding tests and styling demo pages.
| File | Description |
|---|---|
| src/toggle-button/internal.tsx | Forwards button styling classes. |
| src/internal/styles/style-api.scss | Makes the documentation forwarding slot optional. |
| src/file-input/internal.tsx | Forwards styling classes to the upload button. |
| src/dropdown/styles.scss | Renames the documented slot to container. |
| src/dropdown/internal.tsx | Applies the renamed container slot. |
| src/dropdown/__tests__/style-api.test.tsx | Updates tests for the container slot. |
| src/divider/styles.scss | Adds line-color and line-width tokens. |
| src/divider/internal.tsx | Applies the root styling class. |
| src/divider/index.tsx | Forwards root styling configuration. |
| src/divider/__tests__/style-api.test.tsx | Tests root classes and DOM prop filtering. |
| src/button-group/styles.scss | Adds divider tokens and forwarded slot documentation. |
| src/button-group/menu-dropdown-item.tsx | Styles menu triggers and panels. |
| src/button-group/item-element.tsx | Resolves item classes and distributes styling slots. |
| src/button-group/internal.tsx | Forwards slots and styles dividers. |
| src/button-group/internal-interfaces.ts | Defines group styling slots. |
| src/button-group/index.tsx | Forwards styling configuration. |
| src/button-group/icon-toggle-button-item.tsx | Forwards toggle-button styling. |
| src/button-group/icon-button-item.tsx | Forwards icon-button styling. |
| src/button-group/file-input-item.tsx | Forwards upload-button styling. |
| src/button-group/copy-to-clipboard-item.tsx | Forwards copy-button styling. |
| src/button-group/__tests__/style-api.test.tsx | Tests item callbacks and styling slots. |
| src/button-dropdown/styles.scss | Documents forwarded button and panel slots. |
| src/button-dropdown/internal.tsx | Applies trigger, main-action, and panel classes. |
| src/button-dropdown/internal-interfaces.ts | Defines dropdown styling slots. |
| src/button-dropdown/index.tsx | Forwards styling configuration. |
| src/button-dropdown/__tests__/style-api.test.tsx | Tests standard and split-button slots. |
| pages/dropdown/style-api-v2.scss | Uses the renamed container mixin. |
| pages/dropdown/style-api-v2.page.tsx | Uses the renamed container slot. |
| pages/divider/style-api-v2.scss | Defines divider styling examples. |
| pages/divider/style-api-v2.page.tsx | Demonstrates labeled and oriented dividers. |
| pages/button-group/style-api-v2.scss | Defines item, divider, and menu styles. |
| pages/button-group/style-api-v2.page.tsx | Demonstrates styling across item types. |
| pages/button-dropdown/style-api-v2.scss | Defines trigger and panel styles. |
| pages/button-dropdown/style-api-v2.page.tsx | Demonstrates standard, split, and icon variants. |
Review effort: Balanced
7ac7242 to
f26f8ce
Compare
| <Divider {...({ styleClassNames: { root: 'a', unknown: 'b' } } as any)} /> | ||
| <Divider {...({ styleClassNames: { root: 'c' } } as any)}>Label</Divider> |
There was a problem hiding this comment.
nit: Should we rename the classes to be more descriptive, e.g., plain-root, labelled-root, unknown-key (Same things in the other tests)
There was a problem hiding this comment.
I'd keep it a/b/c to be honest - as we consistently use it in many other similar tests already - this includes 9 already merged components and 10+ not merged ones.
|
|
||
| export default function () { | ||
| const [pinned, setPinned] = useState(false); | ||
| const items: ButtonGroupProps.ItemOrGroup[] = [ |
There was a problem hiding this comment.
nit: Should we move this out to the file itself, not the function? Similar to how we do it in the button-dropdown to be consistent? Same for the styleClassNames
There was a problem hiding this comment.
We cannot move items in this case because it depends on state (pinned). I will apply the change to styleClassNames.
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
5 open issues
The tests needas anyto passstyleClassNames, which suggests the publicDividerPropstype… · New The tests needas anyto passstyleClassNames, which suggests the publicDividerPropstype… · New This forwardsstyleClassNamesvia a type assertion onrest, which bypasses the public… · New This uses a type assertion to readstyleClassNamesfromprops. To keep the public API type-safe… · New ForwardingstyleClassNamesvia a cast makes this harder to maintain and weakens type-safety.… · New
2 resolved since last review
Review effort: Lite



Description
New APIs are reflected in the tracker: P3d34KdbCd6o.
How has this been tested?
Unit tests and test pages
Review checklist
The following items are to be evaluated by the author(s) and the reviewer(s).
Correctness
CONTRIBUTING.md.CONTRIBUTING.md.Security
checkSafeUrlfunction.Testing
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.