[ListItem] Improve ListItemSecondaryAction DX #14350
Conversation
eps1lon
added
docs
eps1lon
force-pushed
the
feat/core/ListItemSecondaryAction/improve-dx
branch
from
c001c7b
to
42bbcdc
Compare
| children: chainPropTypes(PropTypes.node, props => { | ||
| const children = React.Children.toArray(props.children); | ||
|
|
||
| // React.Children.toArray(props.children).findLastIndex(isListItemSecondaryAction) |
There was a problem hiding this comment.
What about .findIndex() and replace it once we drop IE 11?
There was a problem hiding this comment.
I was just asking about the commented out code :) The below code looks okay to me
There was a problem hiding this comment.
Yeah. I agree, I would leave something about IE 11 so we don't forget to refactor the code, in 1-2 years from now.
There was a problem hiding this comment.
comment explains the intent of the code
There was a problem hiding this comment.
But with <ListItem><Action /><Text /><Action /></ListItem> ListItem is correctly detecting that it has a secondary action.
Displaying this warning would not be correct with that markup. I fail to see the value in discussing this implementation detail that would add false positives to the warning message. If you want an additional warning against multiple Actions then I'm happy to add it. Your proposal implies one step forward and one step back for DX.
There was a problem hiding this comment.
<ListItem><Action /><Text /><Action /></ListItem> is invalid. The existing warning is good enough IMHO to start with minimal overhead. We can refine it later. At least, we would have data (optimize for learning). If we don't do this change, people will nest two buttons. From a user perpective, it will look like this: "the action should be the last child. Wtf it's already the last child. Oh but I have another action, OK it should be the last child too. Wait, I can only have one child. Ok so I can only use one action."
There was a problem hiding this comment.
I'm wondering if React don't already warn about button > button. It would just add another hint.
There was a problem hiding this comment.
Anyway, it's up to you. It's not truly important. We shouldn't invest too much time here.
There was a problem hiding this comment.
Wait, I can only have one child
You're making my argument here. The current warning with your implementation proposal would not warn about "multiple children detected". It would create exactly the scenario you described.
We never warned explicitly about multiple actions. I don't know of any issue that described confusion about multiple actions. Maybe people are already using multiple actions just fine. If you want to warn against multiple actions feel free to open a separate PR.
ListItemSecondaryActionis not the last child of aListItemCloses #13916