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] Added the ability to provide a selected index to tabs as a prop at initialization. #465

Closed
wants to merge 1 commit into from

Conversation

jdinard
Copy link

@jdinard jdinard commented Mar 26, 2015

This allows you to change the active tab in a render function. It opens the door to easily changing the active tab programatically, and allows you to initialize tabs to an active tab index other than 0

…This allows you to change the active tab in a render function. This opens the door to easily changing the active tab programatically, and allows you to initialize tabs to an active tab index other than 0
@matthewoates
Copy link
Contributor

Isn't having props in your state an anti-pattern?

@ferrannp
Copy link

ferrannp commented Apr 5, 2015

Hi! I was looking for something like this... Couldn't we make selectedIndex as a prop instead of state? My user case is the following: I have some tabs with routes (using react-router), when clicking a tab, it goes to a new route and the tab becomes selected (good), but if I click back, my page goes to previous route but tab doesn't move (inconsistent).

In the way is done now, you can only change the tab with a user click (not programatically...), if we make selectedIndex a prop instead, I believe we could handle both cases. An example would be:

tabRoutes = {home: 0, docs: 1, contact: 2}
<MUITabs selectedIndex={_tabRoutes[currentRoute]} />

In this way, it would be consistent on refreshing the page and on navigation (clicking, going back, etc). Of course, code from @jdinard makes this to work.

@hai-cea
Copy link
Member

hai-cea commented Apr 15, 2015

@jdinard Currently, you could set the intial tab by using the InitialSelectedIndex prop. However, I do see value in being able to set he tabs programatically. @gabdallah222 What do you think about providing a way to set tabs programmatically?

@ghost
Copy link

ghost commented Apr 15, 2015

@hai-cea I can definitely see the need for providing a way to set tabs programmatically - namely in the use case @ferrannp just described.

@jdinard
Copy link
Author

jdinard commented Apr 15, 2015

@hai-cea At the time I wrote that, that prop didn't exist, or myself and the guys I work with all completely overlooked it.

I had a similar use case to @ferrannp, and made the change. Sorry if its a bit of anti-pattern by having the props in the state.

If you wouldn't do it by using ComponentWillReceiveProps to set the active index state, how would you suggest doing it? @matthewoates

@hai-cea hai-cea changed the title Added the ability to provide a selected index to tabs as a prop at initialization. [Tabs] Added the ability to provide a selected index to tabs as a prop at initialization. Jun 19, 2015
@hai-cea
Copy link
Member

hai-cea commented Jul 8, 2015

I think the Tabs component should have a value and also each individual Tab should have a value prop as well. This is how I implemented the new menus. So here's how you would make the second tab selected:

<Tabs value="2" onChange={this._handleChange}>
  <Tab label="Item One" value="1">...</Tab>
  <Tab label="Item Two" value="2">...</Tab>
  <Tab label="Item Three" value="3">...</Tab>
</Tabs>

@hai-cea
Copy link
Member

hai-cea commented Jul 21, 2015

Replaced by #1232

@hai-cea hai-cea closed this Jul 21, 2015
@ferrannp
Copy link

Yes! That will work very nicely with routing, thank you :)

@zannager zannager added the component: tabs This is the name of the generic UI component, not the React module! label Mar 14, 2023
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!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants