Skip to content
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

[Tabs] Default 'width' to 'auto' instead of '100 / n%' #3807

Closed
petermikitsh opened this issue Mar 25, 2016 · 8 comments
Closed

[Tabs] Default 'width' to 'auto' instead of '100 / n%' #3807

petermikitsh opened this issue Mar 25, 2016 · 8 comments
Labels
component: tabs This is the name of the generic UI component, not the React module! new feature New feature or request

Comments

@petermikitsh
Copy link
Contributor

I propose considering that Tab components are not equal width by default. If you are putting content in the Tab elements of variable width, the tabs do not render gracefully:

In the situation below, my Tabs are in side an AppBar component.

Tabs with 'width' set to '100 / n%' (in this case, 20%):

screen shot 2016-03-25 at 11 55 50 am

Tabs with 'width' set to 'auto':

screen shot 2016-03-25 at 11 56 09 am

I'd be happy to open a PR if consensus is in favor of the change.

@oliviertassinari
Copy link
Member

This PR #2861 was going into this direction. You can read it if you are interested.

I propose considering that Tab components are not equal width by default.

I think that we should have a property to enable this feature 👍 .
I'm not sure that it should be the default.

@oliviertassinari oliviertassinari changed the title Tab: Default 'width' to 'auto' instead of '100 / n%' [Tab] Default 'width' to 'auto' instead of '100 / n%' Mar 25, 2016
@oliviertassinari oliviertassinari changed the title [Tab] Default 'width' to 'auto' instead of '100 / n%' [Tabs] Default 'width' to 'auto' instead of '100 / n%' Mar 25, 2016
@oliviertassinari oliviertassinari added the new feature New feature or request label Mar 25, 2016
@petermikitsh
Copy link
Contributor Author

I think it'd be a sensible choice to default the width to 'auto'. It would do no harm from a backwards compatibility perspective. Anyone currently using the Tabs component with Tabs that have same width would not be impacted, as it would render identically.

@mbrookes
Copy link
Member

@alitaheri is still working on a fork of #2861. Might be worth waiting until he's back from vacation to see whether this is already covered.

In any case, I understand what you're saying about auto by default being a non-breaking change, as min-width (I'm assuming) will make the tabs evenly spaced by default.

@alitaheri
Copy link
Member

I'm back 😁

auto is better in my opinion too. But it makes no sense if fillWidth is true. and same-size doesn't make sense if fillWidth is false. so i think this behavior should be controlled by that.

And Scrollable Tabs must have fillWidth set to false.

Is that ok? does that make sense?

@petermikitsh
Copy link
Contributor Author

I realized a new concern when working with tabs today.

The inkbar implementation is percentage-based. so if you create Tabs with the inkbar, but set the width of each Tab to auto, you get a visually undesirable output:

screen shot 2016-03-29 at 11 33 54 pm

In this particular case, the inkbar bleeds into the leftmost tab with width and left values of 33%. This is because the inkbar assumes equal with tabs.

@petermikitsh
Copy link
Contributor Author

I can make each tab the same width, but then the padding between each Tab becomes variable.
This can be problematic when the width of text of each tab is significantly different:

screen shot 2016-03-29 at 11 40 23 pm

@alitaheri
Copy link
Member

Yeah I'm working on a method to calculate the position the inkbar should be in using getBoundingRect. this way, the inkbar will be positioned correctly depending on hot each Tab is rendered, not on how it assumes it will be rendered.

@bpmckee
Copy link

bpmckee commented May 25, 2016

Any update on this?
On a desktop, I want the tabs to show up with 24px padding on either side
The spec (you'll need to scroll down to spec > desktop.

The only way to do this is to either try to calculate the width based off the text length (which is janky), or explicitly set the padding to what I want which screws up the ripple.

@oliviertassinari oliviertassinari added component: tabs This is the name of the generic UI component, not the React module! and removed component: tabs This is the name of the generic UI component, not the React module! labels Oct 19, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: tabs This is the name of the generic UI component, not the React module! new feature New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants