fix(components): getMenuTriggerProps in use-dropdown.ts returns origi…#2451
fix(components): getMenuTriggerProps in use-dropdown.ts returns origi…#2451kuri-sun wants to merge 1 commit into
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. |
|
|
||
| return { | ||
| ...mergeProps(otherMenuTriggerProps, {isDisabled}, originalProps), | ||
| ...mergeProps(otherMenuTriggerProps, {isDisabled: props.isDisabled}, originalProps), |
There was a problem hiding this comment.
@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.
@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.
@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.
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.
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.
Hey guys, I fixed it here https://github.com/nextui-org/nextui/pull/2452/files
kuri-sun
deleted the
getMenuTriggerProps-in-use-dropdown.ts-returns-origina
branch
4 participants
…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)
isDisableddoes not disable dropdown.🚀 New behavior
props.isDisabledto take theisDisabledprop from the Dropdown component in effect as well.originalProps=[Object Object].dropdonw.mov
💣 Is this a breaking change (Yes/No):
No.
📝 Additional Information
No.