Skip to content

fix(components): getMenuTriggerProps in use-dropdown.ts returns origi… #2451

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

Closed
harukikuri wants to merge 1 commit into from
Closed

fix(components): getMenuTriggerProps in use-dropdown.ts returns origi… #2451

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
5 changes: 5 additions & 0 deletions .changeset/lemon-elephants-shout.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@nextui-org/dropdown": major
---

a small support fix for getMenuTriggerProps in use-dropdown.ts returns originalProps as Object
2 changes: 1 addition & 1 deletion packages/components/dropdown/src/use-dropdown.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,7 @@ export function useDropdown(props: UseDropdownProps) {
const {onKeyDown, onPress, onPressStart, ...otherMenuTriggerProps} = menuTriggerProps;

return {
...mergeProps(otherMenuTriggerProps, {isDisabled}, originalProps),
...mergeProps(otherMenuTriggerProps, {isDisabled: props.isDisabled}, originalProps),
Copy link
Copy Markdown
Member

@jrgarciadev jrgarciadev Mar 5, 2024

Choose a reason for hiding this comment

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

@kuri-sun the isDisabled is already being extracted from the props please check this line https://github.com/nextui-org/nextui/pull/2451/files#diff-88cdde9752c6237b962186b11532b521dd8ed7fd98b684e90b050993124cf2c9R53

Copy link
Copy Markdown
Member

@wingkwong wingkwong Mar 5, 2024

Choose a reason for hiding this comment

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

@kuri-sun Ya I changed to {isDisabled} because I've noticed that it's from props so I refactored a bit. I was wondering the difference as well. Moreover, if I don't set isDisabled, then {isDisabled: props.isDisabled} would become isDisabled: undefined after merged. Taking {isDisabled} doesn't have such problem since by default we set it as false.

Copy link
Copy Markdown
Author

@harukikuri harukikuri Mar 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@jrgarciadev and @wingkwong
When the isDisabled does not get passed to the Dropdown component, it always becomes isDisabled=false(https://github.com/nextui-org/nextui/blob/9b27da544e950cdb2bccd2d1a57ead772fd10dc1/packages/components/dropdown/src/use-dropdown.ts#L53).
So I was worrying about this situation: the trigger Button component was disabled however the still is enabled.

  1. isDisabled (Current)
children.mov
  1. props.isDisabled (This one)
this-change.mov

By setting {isDisabled: props.isDisabled}, it becomes isDisabled: undefined, so that coming the Button's isDisabled can override that, if there is any isDisabled on the Button component.

However, if we can ignore the trigger Button component's isDisabled in this case, I would agree with going with it as it is! 😄

Sorry for the long explanation... I hope this makes sense! Thank you! ❤️

Copy link
Copy Markdown
Member

@wingkwong wingkwong Mar 5, 2024

Choose a reason for hiding this comment

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

Okay. I finally got what you mean. I can see why we need undefined here now. It wasn't that obvious though. @jrgarciadev Should we cover this case as well?

Copy link
Copy Markdown
Member

@jrgarciadev jrgarciadev Mar 5, 2024

Choose a reason for hiding this comment

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

Hey guys, I fixed it here https://github.com/nextui-org/nextui/pull/2452/files

ref: mergeRefs(_ref, triggerRef),
};
};
Expand Down