Menu item small refactor #492
Conversation
…to chore/omri/menu-keyboard-nav-refactor
| @@ -22,7 +22,6 @@ export default function useMenuKeyboardNavigation( | |||
| setActiveItemIndex, | |||
| isVisible, | |||
| ref, | |||
| resetOpenSubMenuIndex, | |||
| @@ -1,5 +1,5 @@ | |||
| /* eslint-disable react/jsx-props-no-spreading */ | |||
| import React, { useCallback, useRef, useLayoutEffect, useMemo, useEffect } from "react"; | |||
| import React, { useCallback, useRef, useLayoutEffect, useMemo, useEffect, forwardRef } from "react"; | |||
There was a problem hiding this comment.
I added forwardRef, and applied the prettier fixes
| @@ -22,7 +22,6 @@ export default function useMenuKeyboardNavigation( | |||
| setActiveItemIndex, | |||
| isVisible, | |||
| ref, | |||
| resetOpenSubMenuIndex, | |||
There was a problem hiding this comment.
you can't remove it since it is not an object (it is in method params)
@sahariko
There was a problem hiding this comment.
I'm not sure I understood you - there's only one place that references this function (in Menu.jsx), and I removed it too in this PR.. where do you think this may still be used?
There was a problem hiding this comment.
If this hook is somehow used outside of this package, this would be considered a breaking change, resulting in a major version bump.
Unless we're 100% sure there's no outside usage, I think that we can create a milestone and aggregate all of these breaking changes, but for now let's hold back on this.
There was a problem hiding this comment.
I'm not 100% (I don't think we can be 100% sure).
It is however a highly internal hook for the menu component, which isn't exposed directly from the package. If someone external imports this hook directly (e.g. require("dist/src/components....."), should we avoid changing it?
I have no problem with reverting this change if needed, but I'm pretty sure I'm missing something :)
There was a problem hiding this comment.
Hmmm good point, I think we can make this change then 🙂
…to chore/omri/menu-item-small-refactor
Small refactor, no actual change - mostly prettier. I split it to avoid big PRs.