Patternfly-react: SimpleListItem's isCurrent prop not behaving as expected

[wsamsnyder]

Describe the issue. What is the expected and unexpected behavior?
SimpleListItem accepts isCurrent that is described as "Indicates if the link is current/highlighted", allowing manual control. In practice though, isCurrent is only the initial state and is overwritten by SimpleListItem's internal state when something on the list is clicked.

Please provide the steps to reproduce. Feel free to link CodeSandbox or another tool.
An Item in a SimpleList's isCurrent prop can be hardcoded to true, making it highlighted, then click an item. It will behave as if the prop was never set and only the clicked on Item will be highlighted.

Is this a bug or enhancement? If this issue is a bug, is this issue blocking you or is there a work-around?
Bug, not blocking.

[KKoukiou]

@wsamsnyder nice catch - indeed I can reproduce. Also I noticed the same issue exists on the DataListItem with the isExpanded property. It's ignored after clicking on the toggle.

In my opinion, both these properties should not be exposed, but internally handled within the event handlers. We still need to expose a property for marking the initially selected/current item, but that should be a property of the parent component, in this case the SimpleList.

@kmcfaul I would like your input here, since you initially wrote this component, and if you agree I will deprecate this property and expose properties on the parent components, something like initiallySelected.

[TimoSta]

@KKoukiou If at all possible, I would ask you to _not_ deprecate the isCurrent prop, since we have use cases where we need to have the system set the currently selected item.

A good example is updating the list as a result of a route change. We have a SimpleList that allows the user to browse a set of items, and on selection, the current URL /item/123 is updated, propagating to other parts of the app. We need to support the reverse as well, where the user changes the URL, and the current item in the list should be updated accordingly.

That would be impossible without resetting the full list if the SimpleList's current selection becomes an uncontrolled property.

[kmcfaul]

I believe isCurrent was meant to act as a default current item for the list, which then gets updated. If we want the list to be uncontrolled, we should change the logic that determines whether the item is 'current' or not. Right now, isCurrent gets overridden by the SimpleList's state which is tracking its own current item. Adding a flag to SimpleList that makes an item always look to isCurrent should allow this and will prevent controlled SimpleLists from having multiple current items.

@TimoSta @KKoukiou

[TimoSta]

@kmcfaul If I understand you correctly, you're suggesting to change the implementation to disregard the internal handling for is current if isCurrent is set on any/all of the items?

If so, that would allow us to turn the simple list into an externally controlled component, and will suit our needs perfectly.

To define an initial current value that will then be handled internally, would it make sense to call the prop defaultIsCurrent, following the convention that React's input fields are using for setting default values on uncontrolled components via defaultValue?

[kmcfaul]

Yes, but using a new flag like isUncontrolled. Then if present, isCurrent would be the only thing determining if an item is current.

And I agree that we should update the prop name to defaultIsCurrent (well, technically add this prop because isCurrent will still be used) and then use that for the controlled SimpleList.

[TimoSta]

Excellent! That will work perfectly for us.

Thank you!

[wojta]

@tlabaj taking this one

[wojta]

As this is a beta component, I assume that the props can be changed. I propose to sync the behavior more with similar NavList and NavItem functionality. So there is isActive prop and component is uncontrolled.