enable controlled component behavior

[mziwisky]

if you pass a selectedIndex prop, then that tab will be selected, no
matter what. it doesn't change automatically from user interaction. to
change in response to interaction, have the parent pass an onSelect
handler that updates the selectedIndex prop.

fixes #74

definitely warrants a major version bump

[mziwisky]

note that the CI failures here are due to a rackt-cli issue, ref'd by #79

[danez]

Thanks for the contribution, I'm a little bit worried that this is too much magic with having to set onSelect and selectedIndex to enable controlled component mode.

To make it more obvious to users what is happening I thought of splitting selectedIndex up into two props: initialTabIndex and selectedTabIndex, where the first one only sets the initial open tab and the second is basically controlling the tab index from the outside.

Another approach would be to have a prop like controlled: true/false which changes the behavior of selectedIndex, but not sure if I really like that.

What are your thoughts on this?

Sorry that it took so long long to look through the PR.

[joepvl]

TL;DR: great idea, love the added API clarity; I say let's really mirror controlled component behaviour and do proper warnings and similar prop naming conventions

---

My 2¢: I like this a lot, I think adding this would add clarity to react-tabs's API. I also think @danez's suggestion of separating the responsibility for setting the initial tab index (but letting it be controlled by the component's internals thereafter) & "hard-setting" the tab index like in React's own controlled components would be the way to go.

For naming, I would suggest going with defaultIndex (defaultSelectedIndex would work as well; clearer but more verbose) and selectedIndex, to more or less mirror React's controlled component interface. (Side note: I might have preferred @danez's suggestion of selectedTabIndex because it better describes the prop's actual function, but in my opinion that name would collide too much with the tabindex HTML attribute.)

Similarly mirroring React, I would suggest forcing users to choose between using defaultIndex and selectedIndex, but still allow onSelect to be specified in both cases. A warning or error could be thrown if both are passed, like the one React throws when you pass both value and defaultValue to an <input/>:

Warning: FooComponent is changing an uncontrolled input of type text to be controlled. Input elements should not switch from uncontrolled to controlled (or vice versa). Decide between using a controlled or uncontrolled input element for the lifetime of the component. More info: https://fb.me/react-controlled-components

BTW I don't think that warning is clear enough to people who don't know (yet) what a controlled component is, which would be relevant here as well, so I'd at least mention the specific attributes that are causing it to be thrown.

Another thing to consider is how React handles passing value but no onChange (this would be the equivalent of passing selectedIndex but no onSelect to the Tabs component):

Warning: Failed form propType: You provided a value prop to a form field without an onChange handler. This will render a read-only field. If the field should be mutable use defaultValue. Otherwise, set either onChange or readOnly. Check the render method of FooComponent.

Not sure if I'd go for having a readOnly prop for Tabs, but then again, for clarity it might be nice to have it + a warning similar to the above.

@mziwisky if you'd like some help on this, give me a shout. 😃

[mziwisky]

Thanks for the replies, guys.

I'm +1 on changing this interface to closer reflect that of builtin React components. Considering your feedback, here's my summary of the behavioral changes I think this PR needs:

• add a defaultSelectedIndex prop which is used to set the initial index of an uncontrolled component
• add a warning when both defaultSelectedIndex and selectedIndex are provided simultaneously
• add a warning when props change from including a defaultSelectedIndex and no selectedIndex to including a selectedIndex and no defaultSelectedIndex
• add a warning for vice-versa of above bullet
• do NOT warn when onSelect is provided but selectedIndex is not
• DO warn when selectedIndex is provided but onSelect is not (this is the current behavior, just noting it to be clear)

Is that the right set of changes? Am I missing anything? I do prefer defaultSelectedIndex over defaultIndex, as it mirrors the defaultValue and value props that an <input> accepts, and i never really shy away from long descriptive parameter names. But I am ultimately fine with either one if there are stronger opinions on the other side of the fence.

I don't know when I'd be able to get around to this. @joepvl if you've got the time and desire to make it happen, I'd gladly leave it in your hands and provide code reviews if you'd like.

[joepvl]

Just popping in to let you know I'm not ignoring this & haven't forgotten about it. I just have little time either atm but am definitely up for picking it up when that changes. Regrettably, not sure how long that will take, it may be a few weeks.
To get started, I pulled your branch into my fork. I guess I'll start by looking at updating the tests to the new methodology and work my way out from there, possibly rebasing things onto the current master branch. I haven't looked at it thoroughly, but on the surface it looks to me like you've already done most of the hard work while fixing the focus issue :).