Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Navigation component: Fix item handling onClick twice #33286

Merged
merged 1 commit into from
Jul 16, 2021
Merged

Navigation component: Fix item handling onClick twice #33286

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions packages/components/src/navigation/item/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,14 +63,15 @@ export default function NavigationItem( props ) {
onClick( event );
};
const icon = isRTL() ? chevronLeft : chevronRight;
const baseProps = isText
const baseProps = children ? props : { ...props, onClick: undefined };
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I might be missing something, but if isText is false and children is true, couldn't we still have 2 onClick handlers registered?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think so 🤔 We don't apply the props on the children automatically. So in this case NavigationItemBase will get an onClick˛handler but nothing else. You can still put an onClick handler on your children in the userland code. But that would mean you are using the component wrong.

Correct way is to add your onClick to the NavigationItem:

<NavigationItem onClick={() => alert('Hello')}>
<div>There is only one event click handler</div>
</NavigationItem>

Wrong way

<NavigationItem onClick={() => alert('Hello')}>
<div onClick={() => alert('Hello children!')}>There is only one event click handler</div>
</NavigationItem>

Note, when we have children then the itemProps is not used at all.

Copy link
Contributor

@jeyip jeyip Jul 15, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note, when we have children then the itemProps is not used at all

Ahhh I think this is what's confusing me. Why use isText at all if we can infer component state from the presence or absence of children?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to make sure ItemUI is acting as a text in case of isText, otherwise we turn it into a button.

isText = true = We pass rest props, so it's up to the user to add extra props
isText = false = We default to a clickable navigation button

Copy link
Contributor

@jeyip jeyip Jul 15, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I see what's happening now 🤦 . I'm tracking. Thanks for the explanation @david-szabo97!

const itemProps = isText
? restProps
: { as: Button, href, onClick: onItemClick, ...restProps };

return (
<NavigationItemBase { ...props } className={ classes }>
<NavigationItemBase { ...baseProps } className={ classes }>
{ children || (
<ItemUI { ...baseProps }>
<ItemUI { ...itemProps }>
<NavigationItemBaseContent
title={ title }
badge={ badge }
Expand Down