Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Menu item small refactor #492

Merged
merged 4 commits into from
Jan 27, 2022
Merged

Menu item small refactor #492

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b1c97aa
removed unused arg
laviomri Jan 26, 2022
302bae1
added forwardRef to MenuItem
laviomri Jan 26, 2022
f1cc454
Merge branch 'master' of github.com:mondaycom/monday-ui-react-core in…
laviomri Jan 26, 2022
1aca7d4
Merge branch 'master' of github.com:mondaycom/monday-ui-react-core in…
laviomri Jan 26, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion src/components/Menu/Menu/Menu.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,6 @@ const Menu = forwardRef(
onSetActiveItemIndexCallback,
isVisible,
ref,
resetOpenSubMenuIndex,
useDocumentEventListeners
);
useMouseLeave(resetOpenSubMenuIndex, hasOpenSubMenu, ref, onSetActiveItemIndexCallback);
Expand Down
1 change: 0 additions & 1 deletion src/components/Menu/Menu/hooks/useMenuKeyboardNavigation.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@ export default function useMenuKeyboardNavigation(
setActiveItemIndex,
isVisible,
ref,
resetOpenSubMenuIndex,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's unused

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can't remove it since it is not an object (it is in method params)
@sahariko

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

@sahariko sahariko Jan 26, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@laviomri laviomri Jan 26, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm good point, I think we can make this change then 🙂

useDocumentEventListeners
) {
const onArrowKeyEvent = useCallback(
Expand Down
Loading