ATab: selected prop is not always respected

[trent-boyd]

Describe the bug ATab does not always respect the value of the selected prop. It can be set to selected={true} and still have aria-selected="false" in the generated HTML.

To Reproduce Steps to reproduce the behavior:

1. Go to SecureX Int (https://securex.int.iroh.site/)
2. Click "Create Dashboard" to create a new Dashboard
3. Click "Save" to save the new Dashboard
4. Focus the dashboard, then click "Customize" to bring up the Edit Modal
5. Click "Delete Dashboard" to delete the Dashboard

Expected behavior The last dashboard in the index should be highlighted (with the blue bar under the tab).

Screenshots UI:

Generated HTML:

React Dev Tools Component view:

Environment (please complete the following information):

• Device: MacBook Pro 15"
• OS: macOS (latest)
• Browser: Chrome
• Version: latest

Additional context In the screenshots, notice the difference between the aria-selected value in the HTML and the selected value in React Dev Tools. They should sync up.

[frattaro]

To note current functionality (and we had already discussed this outside of GH), the docs don't reflect it but the selected prop is intended for initial-mount state only. Changing the selected value will reset the state.

I was looking around last night at other implementations. react-tabs has "modes" that are-- when you use one subset of props, it behaves one way, when you use a different subset of props, it behaves another way, based off of whether you want a controlled or uncontrolled component.

I hesitate to force the user to key/id each tab, which would be the natural result of an interface for a controlled component that looks like:

const [selectedTab, setSelectedTab] = useState(tabKey1);
<ATabGroup selectedTab={selectedTab} onChange={(clickedTabKey) => setSelectedTab(clickedTabKey)}>
  <ATab key={tabKey1}></ATab>
  <ATab key={tabKey2}></ATab>
   ...
</ATabGroup>

vs uncontrolled, which would require ref for programmatic manipulation:

const newRef = useRef(null);
<ATabGroup ref={newRef}>
  <ATab></ATab>
  <ATab defaultSelected={true}></ATab>
  ...
</ATabGroup>
<AButton onClick={() => newRef.current.clearSelection()}> Clear </AButton>

I considered the controlled version where instead of requiring keys it auto-keys them, but that makes it difficult to know the index of a default selected tab beforehand.

If we went with the controlled version only, it would be a breaking change -- if we supported "modes" similarly to react-tabs, it wouldn't be a breaking change but I feel like it would be redundant functionality.

I think the breaking change with controlled component is the right way to go. I need to inventory the breaking changes and get those bundled into a single release.

[trent-boyd]

That's a very confusing use of the selected props, especially considering the PropType comments. If a prop is used just once to set up some default state, it should be upfront about that. defaultSelected would make more sense here, and prepending default to a prop is a standard React paradigm in this situation.

It's a herculean effort to build something like this perfectly with the time and raw materials you were given. I wouldn't worry about breaking changes this early in the development cycle of the library, especially when it hasn't been fully tested yet. I don't think there are enough users to worry about a major version bump right now. Just note the change in the changelog and let people decide the upgrade path they need to take.

I agree that the first approach is best, although I would use tabKey instead of key to avoid confusion. You could make the keying optional by using the index of the child tab instead, but you'd have to strictly enforce that ATab was a direct descendent of ATabGroup. React.Children could be used effectively here, and you could throw warnings in dev mode if a child was a different element or a fragment.

There's a lot of flexibility that could be added here, but I think you're going the right way. :)