Conversation
mdespuits
requested review from
dkordik,
nvmusoke,
pixelbandito and
sebastianvera
| className: customClassName, | ||
| active = false, | ||
| children, | ||
| onClick = () => {}, |
There was a problem hiding this comment.
@pixelbandito @sebastianvera @dkordik As I opened this PR, I noticed that this onClick handler is simply dropping any ability to listen to click events on this component. Seems like a bug. Thoughts on removing this?
There was a problem hiding this comment.
does seem odd that this prop is given a default value if it's not actually used in the component... seems safe to remove as it's unused. (I wonder if the blame would reveal anything interesting- maybe it was formerly used and never cleaned up?)
pixelbandito
left a comment
pixelbandito
left a comment
There was a problem hiding this comment.
Looks ok so far, but I'd like to give it some more attention Monday.
|
Can you verify that tab content missing |
| ...props | ||
| }) => { | ||
| const className = classNames(styles.Item, customClassName, { | ||
| [styles.active]: active || children, |
There was a problem hiding this comment.
this will now set the active class if "children" is truthy? will that not give false positives for inactive tabs? (where "children" would contain the node with the name of the inactive tab?)
There was a problem hiding this comment.
maybe a good first step would be to keep the functionality as is for the TS conversion, while adding some ?. to that section so it no longer errors when there is no child node with a className prop?
if (React.isValidElement(child)) {
return child.props?.className?.indexOf('active') >= 0;
}
4 participants
Accomplishes two things:
TabMenucomponents to TSactiveas a way to change styling rather than rely on the existing property.I also add an
Examplesection to the TabMenu storybookFixes #194