abhijithvijayan / react-minimal-side-navigation

Minimal side navigation component for React
https://codesandbox.io/s/react-minimal-side-navigation-example-y299d?file=/src/components/NavSidebar.jsx
MIT License
67 stars 29 forks source link

Allow for hook/callback on expandable item click #9

Closed shennan closed 3 years ago

shennan commented 3 years ago

Unfortunately I can't use this library until I'm able to hook into an expandable menu item click. Currently, if an expandable item is clicked, no onSelect call is made for that itemId.

I've forked the library and implemented here:

https://github.com/shennan/react-minimal-side-navigation

In my case I've added an onExpandableSelect method but you may want to implement the same functionality by some other means. Tested on my codebase and it works nicely; allowing me to navigate to the main page of the expandable menu item.

If this library implements something similar I will be able to move back to this version of the library. Thanks.

abhijithvijayan commented 3 years ago

Will definitely consider this. Have to look into it n see if it changes the overall simplicity of the library though.

shennan commented 3 years ago

As you can see from my fork; it's a non-breaking change and adds very little lines of code.

abhijithvijayan commented 3 years ago

@shennan

This was fixed in v1.8.0 and the callback

onSelect={({itemId}) => {
  console.log(itemId);
}}

will be called everytime on clicking the main item or the expanded sub nav item

https://user-images.githubusercontent.com/34790378/108617946-39b7ae00-7440-11eb-9ddd-abc79904df09.mp4

shennan commented 3 years ago

Great, thanks.

The only thing I would say is that your patch is potentially a breaking change for those who are not expecting the onSelect method to be called for expanding items. Let's say they had added a generic method which was only supposed to handle submenu items; they may now be attempting to navigate to a page that doesn't exist. So perhaps a major version bump is needed or instead a flag option set for those who want to opt-in to this behaviour?

Just a thought.

This doesn't actually affect me as this is the first time I'm using the library.

abhijithvijayan commented 3 years ago

I have thought about it already. I will add this breaking-change to the readme.

There were multiple issues requesting for this change already

shennan commented 3 years ago

Ok, great. Just wanted to flag it in case it had been overlooked. Thanks again for the change/fix.