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

Allow to use any HTML Element as menu item #325

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+10 −9
Open

Allow to use any HTML Element as menu item #325

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
13 changes: 7 additions & 6 deletions src/use-dropdown-menu.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,11 @@ export interface ButtonProps<ButtonElement extends HTMLElement>

// Create interface for item properties
export interface ItemProps {
onKeyDown: (e: React.KeyboardEvent<HTMLAnchorElement>) => void;
onKeyDown: (e: React.KeyboardEvent<HTMLElement>) => void;
tabIndex: number;
role: string;
ref: React.RefObject<HTMLAnchorElement>;
// eslint-disable-next-line @typescript-eslint/no-explicit-any
ref: React.RefObject<any>;
Comment on lines +18 to +19
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 •
edited
Loading

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.

}

// A custom Hook that abstracts away the listeners/controls for dropdown menus
Expand Down Expand Up @@ -43,8 +44,8 @@ export default function useDropdownMenu<ButtonElement extends HTMLElement = HTML

// Create refs
const buttonRef = useRef<ButtonElement>(null);
const itemRefs = useMemo<React.RefObject<HTMLAnchorElement>[]>(
() => Array.from({ length: itemCount }, () => createRef<HTMLAnchorElement>()),
const itemRefs = useMemo<React.RefObject<HTMLElement>[]>(
() => Array.from({ length: itemCount }, () => createRef<HTMLElement>()),
[itemCount]
);

Expand Down Expand Up @@ -171,7 +172,7 @@ export default function useDropdownMenu<ButtonElement extends HTMLElement = HTML
};

// Create a function that handles menu logic based on keyboard events that occur on menu items
const itemListener = (e: React.KeyboardEvent<HTMLAnchorElement>): void => {
const itemListener = (e: React.KeyboardEvent<HTMLElement | HTMLAnchorElement>): void => {
// Destructure the key property from the event object
const { key } = e;

Expand All @@ -189,7 +190,7 @@ export default function useDropdownMenu<ButtonElement extends HTMLElement = HTML
setIsOpen(false);
return;
} else if (key === 'Enter' || key === ' ') {
if (!e.currentTarget.href) {
if (!('href' in e.currentTarget && e.currentTarget.href)) {
e.currentTarget.click();
}

Expand Down
6 changes: 3 additions & 3 deletions website/docs/design/return-object.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,10 @@ This Hook returns an object of the following shape:
};
itemProps: [
{
onKeyDown: (e: React.KeyboardEvent<HTMLAnchorElement>) => void;
onKeyDown: (e: React.KeyboardEvent<HTMLElement>) => void;
tabIndex: -1;
role: 'menuitem';
ref: React.RefObject<HTMLAnchorElement>;
ref: React.RefObject<any>;
};
...
];
Expand All @@ -45,4 +45,4 @@ This Hook returns an object of the following shape:
- **ref:** A React ref applied to each menu item, used to manage focus.
- **isOpen:** A boolean value indicating if the menu is open or closed. The developer should use this value to make the menu visible or not.
- **setIsOpen:** A function useful for allowing the developer to programmatically open/close the menu.
- **moveFocus:** A function that moves the browser’s focus to the specified item index.
- **moveFocus:** A function that moves the browser’s focus to the specified item index.