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

allow manual toggle of tooltip #629

Merged
merged 2 commits into from
Sep 4, 2018
Merged

allow manual toggle of tooltip #629

merged 2 commits into from
Sep 4, 2018

Conversation

browne0
Copy link
Contributor

@browne0 browne0 commented Sep 4, 2018

Related issues

Fixes https://meetup.atlassian.net/browse/MG-2364

Description

Accidentally removed the ability to click on the x in the tooltip to close it. This adds it back and allows the ability to close the tooltip by clicking outside of it.

@browne0 browne0 requested a review from Mikodin September 4, 2018 15:46
Copy link

@Mikodin Mikodin left a comment

Choose a reason for hiding this comment

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

1 question and a nitpick, other than that lgtm 👍

src/interactive/Tooltip.jsx Show resolved Hide resolved
if (nextProps.manualToggle && prevState.isActive !== nextProps.isActive) {
return { isActive: nextProps.isActive };
}
return prevState;
Copy link

Choose a reason for hiding this comment

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

Is this the same as return null ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, but this way imo is more explicit of what it is actually doing 😄

Copy link

Choose a reason for hiding this comment

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

No doubt, cool with me 🗻 . Just wanted to be sure haha

Copy link
Contributor

@jaclynj jaclynj Sep 4, 2018

Choose a reason for hiding this comment

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

Interesting. I disagree though, see here and comments related: reactjs/rfcs#26 (comment) "Returning null is slightly more performant (since React won't have to merge the state with itself). It also more clearly signals the intent of not updating anything."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changing in a follow up

@browne0 browne0 merged commit 5ffda82 into master Sep 4, 2018
@chenrui333 chenrui333 deleted the fix_closing_mwc_tooltip branch September 16, 2019 22:08
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.

3 participants