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: Tooltips not shown on IconButtons with Dotip childs #8170

Merged
merged 1 commit into from Aug 28, 2018

Conversation

Projects
None yet
6 participants
@jorgefilipecosta
Member

jorgefilipecosta commented Jul 24, 2018

Fixes: #7510

Description

Icon button had a condition where tooltips were not shown if the child's existed.
This caused tooltips to not show when DotTip components were added inside some IconButtons. Besides, that DotTips cannot be nested inside Tooltips or tooltips will render while the user interects with tooltips and the events to disable tooltips may not be handled. This PR adds a dotTip prop to IconButton, so the component can render tooltip and dotTips as siblings.

How has this been tested?

Created an empty post.
Verified a tooltip appears on side inserter (at the side of "Write your story").
Verified that a tooltip appears on sidebar toggle.

@gziolo gziolo requested a review from afercia Jul 25, 2018

@gziolo gziolo added this to the 3.4 milestone Jul 25, 2018

@gziolo gziolo requested a review from noisysocks Jul 25, 2018

@noisysocks

This comment has been minimized.

Show comment
Hide comment
@noisysocks

noisysocks Jul 25, 2018

Member

I'm seeing something a little weird. When I click on to the next tip in the guide, two tooltips are shown on my screen at once.

screen shot 2018-07-25 at 15 28 47

Member

noisysocks commented Jul 25, 2018

I'm seeing something a little weird. When I click on to the next tip in the guide, two tooltips are shown on my screen at once.

screen shot 2018-07-25 at 15 28 47

@jorgefilipecosta

This comment has been minimized.

Show comment
Hide comment
@jorgefilipecosta

jorgefilipecosta Jul 25, 2018

Member

Nice catch @noisysocks, I updated mechanism I think now things are working well.

Member

jorgefilipecosta commented Jul 25, 2018

Nice catch @noisysocks, I updated mechanism I think now things are working well.

@afercia

This comment has been minimized.

Show comment
Hide comment
@afercia

afercia Jul 25, 2018

Contributor

Thanks for looking into this. Quick question: seems to me the Warning: validateDOMNesting(...): <button> cannot appear as a descendant of <button>. is still there, correct?

To my understanding, the warning happens because before the popover "portal" moves the tip in the DOM, the tip (which contains a couple buttons) is actually within the inserter and settings buttons.

Contributor

afercia commented Jul 25, 2018

Thanks for looking into this. Quick question: seems to me the Warning: validateDOMNesting(...): <button> cannot appear as a descendant of <button>. is still there, correct?

To my understanding, the warning happens because before the popover "portal" moves the tip in the DOM, the tip (which contains a couple buttons) is actually within the inserter and settings buttons.

@pento pento modified the milestones: 3.4, 3.5 Jul 30, 2018

@gziolo gziolo modified the milestones: 3.5, 3.6 Aug 8, 2018

@youknowriad youknowriad modified the milestones: 3.6, 3.7 Aug 15, 2018

@afercia

This comment has been minimized.

Show comment
Hide comment
@afercia

afercia Aug 27, 2018

Contributor

Worth noting #9223 moved the first tip to the toolbar, as a consequence the button tooltip doesn't appear any longer.

Contributor

afercia commented Aug 27, 2018

Worth noting #9223 moved the first tip to the toolbar, as a consequence the button tooltip doesn't appear any longer.

@jorgefilipecosta

This comment has been minimized.

Show comment
Hide comment
@jorgefilipecosta

jorgefilipecosta Aug 27, 2018

Member

This PR was rebased. With the merge of #9223 we can now use the approach of the wrapper div everwhere as referred by @noisysocks. This PR was updated to just use wrapper divs so in the dom we don't parent-child relationships of tooltips and dottips. We also don't have now any dotTip props.

Member

jorgefilipecosta commented Aug 27, 2018

This PR was rebased. With the merge of #9223 we can now use the approach of the wrapper div everwhere as referred by @noisysocks. This PR was updated to just use wrapper divs so in the dom we don't parent-child relationships of tooltips and dottips. We also don't have now any dotTip props.

@noisysocks

Thanks for keeping on top of this @jorgefilipecosta! Looks great to me :shipit:

@jorgefilipecosta jorgefilipecosta merged commit c92f502 into master Aug 28, 2018

2 checks passed

codecov/project 50.58% remains the same compared to 18f45f9
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jorgefilipecosta jorgefilipecosta deleted the fix/tooltips-not-shown-because-iconbutton-has-dottip branch Aug 28, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment