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

Add option for Tooltip component to be controlled with isOpen prop #945

Merged

Conversation

JohnathanWeisner
Copy link
Contributor

This allows the Tooltip component to be a controlled component which is needed for https://github.com/narmi/banking/issues/33260.

A developer can pass an optional isOpen property to control if the Tooltip is open or not.

}) => {
const [isOpen, setIsOpen] = useState(false);
const isControlled = isOpen === true || isOpen === false;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
const isControlled = isOpen === true || isOpen === false;
const isControlled = isOpen !== undefined;

Copy link
Collaborator

Choose a reason for hiding this comment

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

Your expression is more type safe - disregard unless you like the simple one better (checks for presence of the prop rather than value)

Copy link
Collaborator

@akdetrick akdetrick left a comment

Choose a reason for hiding this comment

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

Looks good - left a suggestion for the isControlled expression and there's one lint error to fix before merging

@JohnathanWeisner JohnathanWeisner merged commit 7313674 into main Jun 26, 2023
5 checks passed
@narmirobot
Copy link

🎉 This PR is included in version 2.46.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

None yet

3 participants