react-spectrum icon indicating copy to clipboard operation
react-spectrum copied to clipboard

fix: client-navigation on MenuItem

Open levrik opened this issue 1 year ago • 1 comments

I simply copied the code from the useLink hook. This was enough to unblock myself for now.

I wonder if there are more similar cases across the codebase and if it would make sense to extract this to a helper.

The most important part here is that e.preventDefault() is getting called which is only possible by hooking into the native event. Maybe client-side router integration is also something which could be handled as part of usePress already but I'm not entirely sure if this makes sense.

I'm open to suggestions to better generalize this fix!

✅ Pull Request Checklist:

  • [ ] Included link to corresponding React Spectrum GitHub Issue.
  • [ ] Added/updated unit tests and storybook for this change (for new code or code which already has tests).
  • [x] Filled out test instructions.
  • [ ] Updated documentation (if it already exists for this component).
  • [ ] Looked at the Accessibility Practices for this feature - Aria Practices

📝 Test Instructions:

Set href on a MenuItem wrapped inside RouterProvider. Full page load triggered before.

levrik avatar Jun 20 '24 13:06 levrik

Could you provide an example of the broken behavior you're seeing? I tried an example locally and saw client navigation working with menu items already.

devongovett avatar Jul 12 '24 21:07 devongovett

I indeed can't reproduce this anymore without the patch applied. Also not on the commit where I introduced this through pnpm patch. I guess ultimatively the issue has been caused by something else which probably never ended up in a commit. Closing.

levrik avatar Feb 27 '25 13:02 levrik