-
Notifications
You must be signed in to change notification settings - Fork 205
feat: implement style api for tabs component #4027
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 #4027 +/- ##
=======================================
Coverage 96.99% 97.00%
=======================================
Files 861 863 +2
Lines 25207 25268 +61
Branches 9098 9141 +43
=======================================
+ Hits 24450 24511 +61
- Misses 710 751 +41
+ Partials 47 6 -41 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
e0ed207 to
1d4a190
Compare
79f2379 to
0c32f29
Compare
796dec3 to
82d2b70
Compare
130486a to
0c97814
Compare
e56b16f to
72e0c63
Compare
781e90d to
6fb2d13
Compare
|
|
||
| &, | ||
| & > button { | ||
| background-color: transparent; |
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.
Removing transparent because it overrides custom styles for invalid or disabled. Removal caused no visual regressions. Keeping this property would prevent custom styles from taking effect.
| fontWeight: '500', | ||
| paddingBlock: '12px', | ||
| paddingInline: '20px', | ||
| focusRing: { |
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 think focusRing color might need states, because it is displayed over the background color. So, for example, it might not be possible to find an accessible focus ring color that works over both default and active background colors
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 propose we add this to the existing components as well (e.g. button).
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.
For most components it appears outside the component itself so it's less of an issue as the "background" its displayed against isn't state-dependent. I guess the other alternative is to explore whether the ring could be moved outside the background here?
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.
Right, it's also displayed outside of the regular button component. That would make the API more streamlined across components, but I'm not sure if moving the ring will break something... I'll try to explore this further.
| } | ||
|
|
||
| export interface Style { | ||
| tabs?: { |
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 don't think the naming here is clear, I would suggest tabButton
| paddingBlock?: string; | ||
| paddingInline?: string; | ||
| }; | ||
| underline?: { |
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 think this should be part of the tabButton
| width?: string; | ||
| borderRadius?: string; | ||
| }; | ||
| divider?: { |
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 structure/naming here isn't clear: what's the difference between divider and headerDivider?
Description
Extends the tabs 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.