Additional click handler

[naorunaoru]

I've added an ability to pass custom click handler to options. Initial behaviour wasn't working for me because I had both onClick and href on the options so I decided to extend the functionality but preserve the initial API.

Also:

• moved moveFocus to a useCallback hook to avoid redeclaring every render, also this helps with dynamically loaded content
• formatted some conditionals for the sake of readability
• shuffled some types around in an attempt to make them more clear because I've had problems with different option types, but this required explicit type argument because I have no idea how to make it resolve automagically

[corymharper]

Hi, thanks for looking to contribute to our package, a few things that cover multiple lines:

This pull request's scope is too broad. The title indicates the changes made should add an "Additional click handler" but various other unrelated changes are introduced in this merge request that aren't needed in order to accomplish that. If you want to introduce some of these unrelated changes, that should be done in separate pull requests. We also tend to want pull requests to address existing issues, so if you have an idea that you would like to make a pull request for, its probably good to first open an issue where you describe what you are going to try to address in your pull request, then mark your pull request as closing that issue. You can read about some of these things in our contributing guide: https://github.com/sparksuite/react-accessible-dropdown-menu-hook/blob/master/CONTRIBUTING.md#1-choose-something-to-contribute.

Just for an example of why this is important, as it stands now I'm not sure why we need an additional click handler at all, it would be good if an issue existed outlining why we might want to add this behavior.

That aside, I will address some broader issues with the changes introduced here:

This pull request completely removes the generic type for the menu button element and replaces it everywhere with HTMLButtonElement, this isn't something we want to do, it would revert the changes from https://github.com/sparksuite/react-accessible-dropdown-menu-hook/pull/290. It looks like this was done in order to generically type the menu items, but we don't need to make the menu button not generically typed in order to make the menu items generically typed, we should just add another generic type.

This pull request also moves moveFocus into a useCallback, and updated the dependencies to include all the dependencies of the useEffect, thats a good change! However, it also belongs in a separate pull request, and I'd suggest that if we are going to move that function into a memoized function, we should also move the other returned functions into a memoized function as well.

Lastly, you changed some conditions from else if to if, and while the likelihood this would have any unintended consequences is low, the argument of readability isn't a very strong reason to change the lines from what they are, as they aren't particularly difficult to read, and the change otherwise offers no benefit, its just a code style preference.

After you narrow the changes made in this pull request, and either explain why we should make the changes in the pull request description or open an issue related to this pull request, I'll give this a review!

[naorunaoru]

Sorry for not answering earlier.

You're right, I should split this.

as it stands now I'm not sure why we need an additional click handler at all

Well, that's one of solutions to my problem, I'll try to expand on it further in a separate PR. Maybe there's a way to avoid complicating the API by introducing some small changes to enter/space handling logic.

As for other issues you've noted I guess you're right, I'll keep that in mind.

[corymharper]

I'm going to close this pull request, it's pretty stale, and the scope of the changes is too broad. It isn't clear that changes made here are needed or beneficial to users of the package.