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.

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

Allow to use any HTML Element as menu item #325

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jirutka
Copy link

@jirutka jirutka commented Oct 12, 2024

The code works with any HTML element, there's nothing that requires specifically HTMLAnchorElement (<a>) except e.currentTarget.href which is already conditional. Thus the current limitation is only a typing issue introduced by unnecessarily specific ref type. We can make it a generic parameter, but it wouldn't bring any real benefit, just increase complexity and decrease flexibility.

Resolves #174

Copy link
Member

@WesCossick WesCossick left a comment

Choose a reason for hiding this comment

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

This PR would also need to update the itemProps definition in the docs on this page.

Comment on lines +18 to +19
// eslint-disable-next-line @typescript-eslint/no-explicit-any
ref: React.RefObject<any>;
Copy link
Member

Choose a reason for hiding this comment

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

We try to avoid using any within our codebases since it typically causes a loss of type safety and can more easily lead to bugs. That's why we use the @typescript-eslint/no-explicit-any ESLint rule. Can you look into designing this without any?

Copy link
Author

Choose a reason for hiding this comment

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

I did and as I said: „We can make it a generic parameter, but it wouldn't bring any real benefit, just increase complexity and decrease flexibility.“ Using any is perfectly fine in this case, it does not cause a loss of type safety for the users of this hook.

Copy link
Member

@WesCossick WesCossick Oct 17, 2024

Choose a reason for hiding this comment

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

I read what you wrote in your PR description; however, what you've stated is not true.

To demonstrate this, let's say a user of this hook needs to write a useEffect() hook with one of the item's ref objects. They might add this code:

useEffect(() => {
	// Do something
}, [itemProps[0].ref.current?.nonexistentProperty]);

Currently, if you simulate this by adding that code to the src/use-dropdown-menu.test.tsx file, this correctly throws a compile error:

Screenshot 2024-10-17 at 8 30 05 AM

The same useEffect() hook with your proposed changes gives no warning:

Screenshot 2024-10-17 at 8 31 22 AM

So using any in this case would cause a loss of type safety whenever a user of this package needs to work with a ref on one of the items.

The code works with any HTML element, there's nothing that requires
specifically `HTMLAnchorElement` (`<a>`) except `e.currentTarget.href`
which is already conditional. Thus the current limitation is only a
typing issue introduced by unnecessarily specific `ref` type. We can
make it a generic parameter, but it wouldn't bring any real benefit,
just increase complexity and decrease flexibility.

Resolves sparksuite#174
@jirutka
Copy link
Author

jirutka commented Oct 17, 2024

This PR would also need to update the itemProps definition in the docs on this page.

Done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Add support for new element types to be used as menu items
2 participants