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.

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(Dropdown): visibility is controlled when undefined #202

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

C-Hess
Copy link

@C-Hess C-Hess commented Mar 1, 2022

Currently, react-component/dropdown checks the existance of the visibility key in props to determine whether or not the visibility prop is controlled or not. However, it should check for undefined instead. This will support the following behavior:

interface IMyDropdownWrapperProps {
   myProp1: string;
   visible?: boolean;
   onVisibleChange?: (visible: boolean) => void;
}


const MyDropdownWrapper: React.FC<IMyDropdownWrapperProps > = ({
  myProp1,
  visible,
  onVisibleChange
}) => {
  return (
    <Dropdown
      // ISSUE: visible is now ALWAYS controlled, even though it may be undefined, which is not standard
      visible={visible}
      onVisibleChange={onVisibleChange}
   >
     My Dropdown
   </Dropdown>
}

A workaround would be to use the spread operator or something similar to ensure that if visible is undefined, we don't pass down the visible key

Fix an issue where the visibility property becomes controlled if the
visible PROP is defined, but it's VALUE is undefined
@MadCcc
Copy link
Member

MadCcc commented Apr 12, 2022

Add a test case for this?

@C-Hess
Copy link
Author

C-Hess commented Apr 12, 2022

I modified an existing test case. Would you like me to create a separate case for this instead?

@MadCcc
Copy link
Member

MadCcc commented Apr 12, 2022

I modified an existing test case. Would you like me to create a separate case for this instead?

It's better to provide a separate case.

Created a separate test case for when visibility is undefined
@C-Hess
Copy link
Author

C-Hess commented Apr 13, 2022

@MadCcc , I went ahead and created a separate case

@MadCcc
Copy link
Member

MadCcc commented Apr 25, 2022

rc-trigger is updated, you need to update your dependencies and run snapshot again.

Updated the rc-trigger dependency per PR comment.
@C-Hess
Copy link
Author

C-Hess commented Apr 28, 2022

I updated the minimum version for rc-trigger, if that's what you meant by updating dependencies. Let me know if that's not what you meant

@afc163
Copy link
Member

afc163 commented Apr 29, 2022

It will be a breaking change.

Fix issues with tests. Removed accidentially commited snapshot and also
editing existing snapshot after updating rc-trigger
@@ -10,7 +10,7 @@ Array [
<div>
<div
class="rc-dropdown"
style="opacity: 0; pointer-events: none;"
Copy link
Author

Choose a reason for hiding this comment

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

I believe this was caused by updating the rc-trigger minor version.

@C-Hess
Copy link
Author

C-Hess commented May 3, 2022

@afc163

Ah, good catch. I honestly didn't think about that. I assume package updates are handled separately, or do I bump the version as part of this CR? What would you like me to do from here to get this through, given that it's a breaking change?

Or do we want to push this off until later? I think it's important to add for consistency's sake, and users can employ workarounds.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants