[Tabs] Let each tab set its own width #5415
Conversation
The Tab component has a new prop of type number: labelWidth. This number is the width of the label in pixels. The user must either set this prop to every tab or none. If this condition is not met, the ink bar is not drawn, and a warning is displayed in the console. The docs have also been updated with information and an example for this prop. This solves issue mui#4420. This approach was suggested in mui#5301.
|
|
||
| const TabsExampleLabelWidth = () => ( | ||
| <Tabs > | ||
| <Tab labelWidth={100} label="100 width" /> |
There was a problem hiding this comment.
What's the tradeoff behind explicating the tab width?
We could have make it work by computing the tab widths.
| @@ -28,6 +30,8 @@ const descriptions = { | |||
| 'and allowing tabs to be swiped on touch devices.', | |||
| icon: 'An example of tabs with icon.', | |||
| iconText: 'An example of tabs with icon and text.', | |||
| labelWidth: 'An example using the `labelWidth` property. Each tab label specifies its own fixed width regardless ' + | |||
There was a problem hiding this comment.
It could be a block comment before the demo example component declaration.
| @@ -87,6 +87,7 @@ class Tabs extends Component { | |||
|
|
|||
| state = {selectedIndex: 0}; | |||
|
|
|||
|
|
|||
| @@ -113,6 +114,29 @@ class Tabs extends Component { | |||
| this.setState(newState); | |||
| } | |||
|
|
|||
| calculateTabWidthAndInkbarPosition(tabs, selectedIndex, defaultWidth) { | |||
There was a problem hiding this comment.
We don't rely on the instance, it would be better to have this function as a module function.
| if (labelWidth && i < selectedIndex) | ||
| left += labelWidth; | ||
| else if (!labelWidth) | ||
| undefinedLabelWidths++; |
There was a problem hiding this comment.
No need to increment a counter. A simple boolean should be enough.
There was a problem hiding this comment.
A simple boolean is not enough. There are 3 possible scenarios in the calculateTabWidthAndInkbarPosition:
- Not a single Tab has defined
labelWidthprop. This is the default behavior. Each tab is assigned a percentage of the container's width. If have 2 tabs, each gets 50%. The inkbar position is also a percentage (first tab would be 0% and the second would be 50%). - Every single Tab has defined
labelWidthprop. Now we have to use pixels instead of percentages. Each tab is assigned a string with the value of thelabelWidthprop appendingpx. To calculate the inkbar position, we add thelabelWidths of every single component at the left of the selected tab. - Some Tabs have defined
labelWidthbut other's have not. This is an error. The user probably forgot about one or more tabs. It could be possible to fallback to using percentages for the tabs that are mssing thelabelWidthprop, but calculating the inkbar position would be impossible because you can't add undefined values. In this scenario inkbar is not displayed.
I'm aware that this sounds a bit complicated, but I could not find easier alternatives. Adding numbers is far easier than parsing strings with percentages.
I will move this function to a new module TabUtils.js and write a test for it before submitting a new PR to the next branch.
If you have any other suggestions or comments please let me know.
There was a problem hiding this comment.
Thanks for providing more details!
We have had quite some contribution tentatives on this component for the past 2 years.
E.g. #2861.
I'm not quite happy with the API decision here.
I'm gonna post a draft proposal on #4795 so we can bootstrap a conversion around it and find the best approach all together.
oliviertassinari
added
PR: needs revision
|
Actually, following #4795 (comment) I think that it would be better to drop any work on the |
|
@GAumala Thanks for pushing forward the |
The Tab component has a new prop of type number: labelWidth. This number
is the width of the label in pixels. The user must either set this prop
to every tab or none. If this condition is not met, the ink bar is not
drawn, and a warning is displayed in the console.
The docs have also been updated with information and an example for this
prop.
This solves issue #4420. This approach was suggested in #5301.