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 tooltip hide method when already hidden #31115

Merged
merged 7 commits into from
Sep 24, 2020
Merged

fix tooltip hide method when already hidden #31115

merged 7 commits into from
Sep 24, 2020

Conversation

Hiws
Copy link
Contributor

@Hiws Hiws commented Jun 19, 2020

Fixes the error displayed when closing a modal with a tooltip that was never shown.

Closes: #31110

@Hiws Hiws requested a review from a team as a code owner June 19, 2020 15:29
@XhmikosR XhmikosR changed the title Modal: fix tooltip error on close Modal: fix tooltip TypeError on close Jun 19, 2020
@XhmikosR
Copy link
Member

I wonder if we have more cases like this one?

js/src/tooltip.js Outdated Show resolved Hide resolved
Copy link
Member

@Johann-S Johann-S left a comment

Choose a reason for hiding this comment

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

Can you add a unit test please ?

@Johann-S
Copy link
Member

I'm more concerned about listening to a modal hiding in our tooltip component, for me we shouldn't do that @XhmikosR

@XhmikosR
Copy link
Member

I'm more concerned about listening to a modal hiding in our tooltip component, for me we shouldn't do that @XhmikosR

Also, true. Does this code come from v4? You rewrote most of v5's JS code :)

@Johann-S
Copy link
Member

Yep it comes from our v4

@Hiws
Copy link
Contributor Author

Hiws commented Jun 19, 2020

I'm more concerned about listening to a modal hiding in our tooltip component, for me we shouldn't do that @XhmikosR

It makes sense why it's there (if that's what you're questioning), since it's a bit awkward that if you open a popover/tooltip and then close the modal.
The popover/tooltip will be open the next time you open the modal.

but I'm not sure if it makes more sense to look for tooltips in the modal code, or listen for modal closing in the tooltip code.

@Johann-S
Copy link
Member

That's the reason yes, but is it really a good thing ? IMO it should be handled by our users to delete their tooltip if needed

@Hiws
Copy link
Contributor Author

Hiws commented Jun 19, 2020

Perhaps.
I guess the current implementation limits it so they can't keep it open. Even if they wanted to.
While if the code was removed, it would keep it open, while still allowing the users to specifically close them if they wish.

@Johann-S
Copy link
Member

Anyway you fix is good @Hiws but can you add a unit test please ?

@Hiws
Copy link
Contributor Author

Hiws commented Jun 19, 2020

Anyway you fix is good @Hiws but can you add a unit test please ?
@Johann-S

Tried writing this test for 2 hours, and it looks like it's working now.
However, I've never really written unit tests before, so not sure if it's correct or not.

@Hiws Hiws requested a review from Johann-S June 19, 2020 18:02
@XhmikosR
Copy link
Member

I think the test should be in modal not in tooltip :)

@Hiws
Copy link
Contributor Author

Hiws commented Jun 19, 2020

I think the test should be in modal not in tooltip :)

Ii feel like this case isn't specifically tied to modals, but the tooltip's hide method being run without the show method ever being ran.
Which is why i placed it under the tooltip tests and not modal.

@XhmikosR
Copy link
Member

The error your patch fixes is in modal.js so that's where it should be tested that we don't throw.

@XhmikosR
Copy link
Member

NVM, you are right, I need to sleep.

@Hiws
Copy link
Contributor Author

Hiws commented Jun 19, 2020

NVM, you are right, I need to sleep.

I'll take part of the blame for having a bad title 😉

@Johann-S
Copy link
Member

Johann-S commented Jun 21, 2020

just my last comment and we are good to go here @Hiws thanks for your work 👍

@Johann-S Johann-S changed the title Modal: fix tooltip TypeError on close fix tooltip hide method when already hidden Jun 21, 2020
@polRk
Copy link

polRk commented Jul 16, 2020

Before merge, or alpha 2, use on child component style="pointer-events: none;"

<button class="btn btn-light" data-toggle="tooltip" data-placement="top" title="Tooltip on top">
    <svg width="18" height="18" style="pointer-events: none;" ><use xlink:href="#heart" /></svg>
</button>

@mdo
Copy link
Member

mdo commented Sep 10, 2020

Sorry for being out of the loop, what's the status of this one?

@mdo
Copy link
Member

mdo commented Sep 14, 2020

Pulling from Alpha 2 given unclear status.

@Johann-S
Copy link
Member

We are waiting for @Hiws to do a last change and rebase his branch

@Hiws
Copy link
Contributor Author

Hiws commented Sep 24, 2020

Sorry for the delay.

I'm not quite sure what you mean with you should call it twice and except nothing.

@Johann-S
Copy link
Member

Johann-S commented Sep 24, 2020

You're right it was good like that thanks 👌

Edit: Since everything is good it should land in Alpha 2

@XhmikosR XhmikosR merged commit 43b4252 into twbs:main Sep 24, 2020
olsza pushed a commit to olsza/bootstrap that referenced this pull request Oct 3, 2020
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.

Modals that include tooltip/popover throws error on close
5 participants