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

Issue #364 - dangling tooltips #383

Merged
merged 10 commits into from Dec 3, 2018

Conversation

storrisi
Copy link
Contributor

Fixes #364

Seems like this is a knows issue of ant-design when there is a Tooltip on a disabled Button, as explained here: ant-design/ant-design#9581

I've tried to replace the Tooltip with the Bootstrap one, but it doesn't work on elements that are not buttons, like a DropDown for example.

As a solution, i've hidden the tooltip if a disabled prop is passed to the render component.

return (
<Tooltip placement="topLeft" title={tip} arrowPointAtCenter>
<Tooltip placement="topLeft" title={!disabled && tip} arrowPointAtCenter>
Copy link
Member

Choose a reason for hiding this comment

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

I think this is a nice workaround for the issue!

Can we read the disabled property of button or do we need a separate prop for this?

Copy link
Member

Choose a reason for hiding this comment

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

don't think that we can use the disabled prop of the supplied button. In the general case the value fo the button prop may not be an instance of the Button component so may not have a disabled prop. For example, it might be a Button wrapped in some other component (e.g., a Dropzone). So button would be an instance of Dropzone.

Copy link
Member

@jeffmcaffer jeffmcaffer left a comment

Choose a reason for hiding this comment

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

I am curious why the Bootstrap tooltip does not work. I'm quite sure that we use it in many other places (on other sites) that are not buttons. While the workaround is ok, it feels unsustainable to pass duplicate disabled props everywhere that we use tooltips.

@storrisi
Copy link
Contributor Author

Actually, i'm not so happy about this workaround as well.
I will investigate on the Bootstrap behaviour in order to find a different and more consistent solution.

@storrisi
Copy link
Contributor Author

storrisi commented Dec 3, 2018

I've found what was the issue with Bootstrap, so i've brought it back to the implementation of Tooltips.
Additionally, i've changed the button property to more understandable children.
Of course this means that instead of passing a button prop to the component, it will render any kind of child of himself.

@jeffmcaffer
Copy link
Member

@storrisi I am getting an error in ButtonWithTooltip when loading the defintiions page. It seems that children is coming in undefined. Was this working for you? Note that I did merge in the latest of master here. I'll continue to look but wanted to check. Perhaps my merge was bad

React.Children.only expected to receive a single React element child.

@storrisi
Copy link
Contributor Author

storrisi commented Dec 3, 2018

It's probably because of the merge of the #375 , i'm gonna update this one since something has changed

@storrisi
Copy link
Contributor Author

storrisi commented Dec 3, 2018

@jeffmcaffer fixed, now it should be fine, thanks for catching up

@jeffmcaffer jeffmcaffer merged commit 0cff7ca into clearlydefined:master Dec 3, 2018
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.

None yet

3 participants