-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
fix(components): getMenuTriggerProps in use-dropdown.ts returns origi… #2451
fix(components): getMenuTriggerProps in use-dropdown.ts returns origi… #2451
Conversation
…nalProps as Object
🦋 Changeset detectedLatest commit: 18de527 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
@kuri-sun is attempting to deploy a commit to the NextUI Inc Team on Vercel. A member of the Team first needs to authorize it. |
@@ -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), |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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
(
isDisabled = false, |
So I was worrying about this situation: the trigger Button component was disabled however the still is enabled.
isDisabled
(Current)
children.mov
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! ❤️
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
…nalProps as Object
Closes #2448
📝 Description
As per request from @wingkwong for the review of this PR, since my last change was affecting this matter.
⛳️ Current behavior (updates)
isDisabled
does not disable dropdown.🚀 New behavior
props.isDisabled
to take theisDisabled
prop from the Dropdown component in effect as well.originalProps=[Object Object]
.dropdonw.mov
💣 Is this a breaking change (Yes/No):
No.
📝 Additional Information
No.