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.

Sign up for GitHub

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

Jump to bottom

Force tooltip and popover to recreate content every time it opens #35679

Merged
merged 1 commit into from Jun 27, 2022
+18 −16
Merged

Force tooltip and popover to recreate content every time it opens #35679

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
28 changes: 14 additions & 14 deletions js/src/tooltip.js
View file
Open in desktop
Expand Up @@ -115,6 +115,7 @@ class Tooltip extends BaseComponent {
this._activeTrigger = {}
this._popper = null
this._templateFactory = null
this._newContent = null

// Protected
this.tip = null
Expand Down Expand Up @@ -205,6 +206,12 @@ class Tooltip extends BaseComponent {
return
}

// todo v6 remove this OR make it optional
if (this.tip) {
this.tip.remove()
this.tip = null
}

const tip = this._getTipElement()

this._element.setAttribute('aria-describedby', tip.getAttribute('id'))
Expand All @@ -219,7 +226,7 @@ class Tooltip extends BaseComponent {
if (this._popper) {
this._popper.update()
} else {
this._createPopper(tip)
this._popper = this._createPopper(tip)
}

tip.classList.add(CLASS_NAME_SHOW)
Expand Down Expand Up @@ -305,7 +312,7 @@ class Tooltip extends BaseComponent {

_getTipElement() {
if (!this.tip) {
this.tip = this._createTipElement(this._getContentForTemplate())
this.tip = this._createTipElement(this._newContent || this._getContentForTemplate())
}

return this.tip
Expand Down Expand Up @@ -335,17 +342,11 @@ class Tooltip extends BaseComponent {
}

setContent(content) {
let isShown = false
if (this.tip) {
isShown = this._isShown()
this._newContent = content
if (this._isShown()) {
this.tip.remove()
Copy link
Member

Choose a reason for hiding this comment

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

Do we need those two lines (l.347 and l.348) since show does it on its own ?

Copy link
Member Author

Choose a reason for hiding this comment

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

I am afraid in case we do not remove tip and set null into the variable, tooltip will not be re-created

Copy link
Member

Choose a reason for hiding this comment

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

I see your point, but my point is that the show function already does it l.211 and l.212 and the function is called every time we get in this case. I hope I didn't miss anything but this.tip isn't called before those lines. Nothing changed when I removed those on my side.

this.tip = null
}

this._disposePopper()
this.tip = this._createTipElement(content)

if (isShown) {
this._disposePopper()
this.show()
}
}
Expand Down Expand Up @@ -373,7 +374,7 @@ class Tooltip extends BaseComponent {
}

_getTitle() {
return this._config.title
return this._resolvePossibleFunction(this._config.title) || this._config.originalTitle
}

// Private
Expand All @@ -394,7 +395,7 @@ class Tooltip extends BaseComponent {
this._config.placement.call(this, tip, this._element) :
this._config.placement
const attachment = AttachmentMap[placement.toUpperCase()]
this._popper = Popper.createPopper(this._element, tip, this._getPopperConfig(attachment))
return Popper.createPopper(this._element, tip, this._getPopperConfig(attachment))
}

_getOffset() {
Expand Down Expand Up @@ -592,7 +593,6 @@ class Tooltip extends BaseComponent {
}

config.originalTitle = this._element.getAttribute('title') || ''
config.title = this._resolvePossibleFunction(config.title) || config.originalTitle
if (typeof config.title === 'number') {
config.title = config.title.toString()
}
Expand Down
6 changes: 4 additions & 2 deletions js/tests/unit/tooltip.spec.js
View file
Open in desktop
Expand Up @@ -185,7 +185,7 @@ describe('Tooltip', () => {
const tooltipEl = fixtureEl.querySelector('a')
const tooltip = new Tooltip(tooltipEl)

expect(tooltip._config.title).toEqual('Another tooltip')
expect(tooltip._getTitle()).toEqual('Another tooltip')
})
})

Expand Down Expand Up @@ -848,7 +848,7 @@ describe('Tooltip', () => {
}, 100)

setTimeout(() => {
expect(insertedFunc).toHaveBeenCalledTimes(1)
expect(insertedFunc).toHaveBeenCalledTimes(2)
resolve()
}, 200)
}, 0)
Expand Down Expand Up @@ -1166,6 +1166,7 @@ describe('Tooltip', () => {
tooltip.setContent({ '.tooltip-inner': 'foo' })

expect(tip()).not.toHaveClass('show')
tooltip.show()
expect(tip().querySelector('.tooltip-inner').textContent).toEqual('foo')
})

Expand Down Expand Up @@ -1229,6 +1230,7 @@ describe('Tooltip', () => {
})

tooltip.setContent({ '.tooltip': { 0: childContent, jquery: 'jQuery' } })
tooltip.show()

expect(childContent.parentNode).toEqual(tooltip._getTipElement())
})
Expand Down