Skip to content

tooltip dispose:removing only own event handler#28896

Merged
Johann-S merged 2 commits into
twbs:masterfrom
david-lallement:tooltip-dispose
Jun 13, 2019
Merged

tooltip dispose:removing only own event handler#28896
Johann-S merged 2 commits into
twbs:masterfrom
david-lallement:tooltip-dispose

Conversation

@david-lallement

Copy link
Copy Markdown
Contributor

If the tooltip component is in a modal, only the listeners created by the tooltip component should to be removed in the dispose method.
For example, if we subscribe to the hide.bs.modal event of the modal component and call the dispose method of a tooltip component inside, the hide.bs.modal event subscription is lost.

Using of event namespace feature of jquery ensures that only the handler created in _setListeners function is removed in dispose function.

@david-lallement david-lallement requested a review from a team as a code owner June 12, 2019 07:33
@Johann-S

Copy link
Copy Markdown
Member

Hi @david-lallement,

Can you create a live demo of the problem via CodePen/JS Bin or Stackblitz ?

Because we already use event namespace of jQuery

@david-lallement

Copy link
Copy Markdown
Contributor Author

Hi Johann,
a codepen to reproduce the issue : https://codepen.io/david-lallement/pen/KjwoYx .

@Johann-S

Copy link
Copy Markdown
Member

Nice catch @david-lallement 👍

It seems your change broke our unit tests, please can you fix that ?

@david-lallement

Copy link
Copy Markdown
Contributor Author

Hi Johann,
I fixed the issue in new commit.
The use of jquery event namespace worked in release 4.3.1 but not in the master branch.
I replaced it with a named function.

@XhmikosR

Copy link
Copy Markdown
Member

@Johann-S: when you merge this please adapt it for v4 and push it to my v4-dev-xmr branch

@Johann-S Johann-S merged commit 0829dec into twbs:master Jun 13, 2019
@Johann-S

Copy link
Copy Markdown
Member

Now it's fixed thanks @david-lallement 👍 you can see it here: https://codepen.io/Johann-S/pen/zVGmzZ

and it'll be available in our next v4 release

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants