Skip to content

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