-
Notifications
You must be signed in to change notification settings - Fork 204
feat: implement style api for toggle button component #4025
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
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4025 +/- ##
==========================================
- Coverage 97.26% 96.99% -0.27%
==========================================
Files 861 862 +1
Lines 25469 25227 -242
Branches 9095 9100 +5
==========================================
- Hits 24772 24469 -303
- Misses 650 711 +61
Partials 47 47 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
e198728 to
ddcbf7d
Compare
2adaabf to
1c92886
Compare
src/toggle-button/internal.tsx
Outdated
| nativeButtonAttributes={nativeButtonAttributes} | ||
| nativeButtonAttributes={{ | ||
| ...nativeButtonAttributes, | ||
| style: { ...nativeButtonAttributes?.style, ...getToggleButtonStyles(style) } as React.CSSProperties, |
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 internal button component accepts the style prop already - why do we merge it with the native attributes instead?
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 toggle button has a pressed state, the button doesn't. We would need to apply the pressed state styling to the native attributes and could pass the other styles to the style prop. That adds more complexity than necessary. In general, there is no mechanism yet for nested component styling.
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.
Well, I would rather do it as you said: pass part of the style using style, and the rest using native attributes. Then add a code comment documenting why do we need that second part. This makes the separation explicit and benefits future refactoring once we have a good approach for nested component styling.
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.
Ok, I will change it to handle that 👍
src/toggle-button/style.tsx
Outdated
| [customCssProps.styleFocusRingBorderWidth]: style?.root?.focusRing?.borderWidth, | ||
| }; | ||
|
|
||
| return Object.fromEntries(Object.entries(properties).filter(([, value]) => value !== undefined)); |
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.
Why do we need to filter out undefined props?
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.
If we don't filter them out, we rely on the guarantee that style properties with undefined values don't cause unintended side effects (like resetting styles). By filtering them out, we ensure this behavior remains consistent even if implementation details change in the future.
Also, without filtering, we'd return an object containing all properties, including those with undefined values:
{
borderRadius: '10px',
borderWidth: undefined,
paddingBlock: undefined,
paddingInline: undefined,
...
};
Filtering produces a cleaner, more focused result:
{
borderRadius: '10px',
};
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.
I understand the diff, but disagree with the approach. Besides, there is a performance penalty when doing this: not only we need to iterate over all properties, we also need to construct a new object from that.
It is not very big impact, but it grows with the number of properties and components on the page. Imagine a future table with lots of nested styles, propagated to its internal checkboxes, etc. Then imagine that in every row there are a few inline buttons with styles. Currently, all these styles are not handled separately (e.g. in some style provider), but applied in the component directly. This can account for some ~10ms of processing time or even more on lower-performing devices for this transformation alone.
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.
As this PR intention is to add styles for toggle buttons - I would remove this transformation. We can solve it separately and make sure to include all existing components with style API, which is best to be done with a shared utility.
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.
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.
I agree, thanks for pointing that out. I'll remove this logic and think of a utility function in the next step that will be used by all styled components when refactoring.
| }); | ||
|
|
||
| // Should NOT contain non-pressed states (these are handled by InternalButton) | ||
| expect(result.nativeButtonStyles).not.toHaveProperty('borderRadius'); |
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.
Isn't the check above enough, why do we explicitly check for these props to not be present?
| }); | ||
|
|
||
| describe('state properties', () => { | ||
| STATE_PROPERTIES.forEach(property => { |
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.
Isn't that already verified with the "extracts only pressed states to nativeButtonStyles" test?

Description
Extends the toggle button component with a style API for customization, allowing developers to override default component styles.
Related links, issue #, if available: n/a
How has this been tested?
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.