Allow subnav items to execute provided onClick handlers #4394
Conversation
size-limit report
|
CSilivestru
force-pushed
the
allow.subnav.onclick
branch
from
1f76d9b to
66921db
Compare
|
Talked to @BPScott and, even though the removal of MouseEvent in the click handler is a breaking change, we can put this into the 7.0.0 since this brings it back to Polaris Principles of not exposing DOM events :). |
kyledurand
left a comment
kyledurand
left a comment
There was a problem hiding this comment.
Minor comments on testing but I think they're worthwhile changes
| }, | ||
| }); | ||
|
|
||
| expect(spy).toHaveBeenCalled(); |
There was a problem hiding this comment.
Might be worth using toHaveBeenCalledTimes(1) here. I think nav item is recursive in rendering itself as a sub nav item.
| }, | ||
| }); | ||
|
|
||
| expect(spy).toHaveBeenCalled(); |
There was a problem hiding this comment.
My comment here didn't get included in the review but I think it makes sense adding it here as well
I think the onClick checks you added in the component cover this but I'd hate to see that regression
UNRELEASED.md
Outdated
| - Bring back borders on the `IndexTable` sticky cells ([#4150](https://github.com/Shopify/polaris-react/pull/4150)) | ||
| - Adjust `IndexTable` sticky z-index to avoid collisions with focused `TextField` ([#4150](https://github.com/Shopify/polaris-react/pull/4150)) | ||
| - Adjust `IndexTable` rows to have a grey hover state when unselected ([#4359](https://github.com/Shopify/polaris-react/pull/4359)) | ||
| - Properly support `selected` prop for `Navigation.Item` when `url` prop is not specified ([#4375](https://github.com/Shopify/polaris-react/pull/4375)) |
There was a problem hiding this comment.
Whoops, just noticed this as well. I think it can be removed
voiid
left a comment
voiid
left a comment
There was a problem hiding this comment.
LGTM. Do you know if there's impact for this change in web?
alex-page
force-pushed
the
v7.0.0-release
branch
from
8559933 to
d111db6
Compare
|
@voiid The subnav items in web do not currently use the onClick prop so there should be no effect :) @kyledurand sorry for the delay but I will make those changes asap! |
BPScott
force-pushed
the
v7.0.0-release
branch
from
d111db6 to
8559933
Compare
CSilivestru
force-pushed
the
allow.subnav.onclick
branch
from
66921db to
f87c0bf
Compare
- Support selected prop for navigation items without url - Update UNRELEASED.md
CSilivestru
force-pushed
the
allow.subnav.onclick
branch
from
f87c0bf to
98e755a
Compare
| }, | ||
| }); | ||
|
|
||
| expect(spy).toHaveBeenCalledTimes(2); |
There was a problem hiding this comment.
Because of the recursive nature, onNavigationDismiss seems to be called twice (once at the sub nav level and once at the item level). Is that correct, @kyledurand?
There was a problem hiding this comment.
Ok cool I think that makes sense 👍
4 participants
WHY are these changes introduced?
Similar to #4374, the sub-navigation items accept an
onClickproperty but do not actually use it. This PR allows a consumer to provide a custom click handler that will be executed when a subnav item is clicked.WHAT is this pull request doing?
This PR is to enable the subnav item to first execute
onNavigationDismissand then any other provided click handler, similar to the Item click handlerHow to 🎩
🎩 checklist
README.mdwith documentation changes