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 on #35679 #36668

Conversation

louismaximepiton
Copy link
Member

Re-open from #36585 (Sorry couldn't re-open on the other one) trying to fix #35679 discussions.

This PR tries to fix the #35679 (comment).

@louismaximepiton louismaximepiton requested a review from a team as a code owner July 4, 2022 10:25
@louismaximepiton louismaximepiton changed the title Fix on # Fix on #35679 Jul 4, 2022
@GeoSot GeoSot added this to In progress in v5.2.0-stable via automation Jul 4, 2022
@GeoSot
Copy link
Member

GeoSot commented Jul 4, 2022

thank you @louismaximepiton for your consistency and for opening again the PR.

May I ask one question? The change on line 347 & 448 depends on the other two?

Why, I am asking:

bootstrap/js/src/tooltip.js

Lines 376 to 378 in c1813ef

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

_getTitle is used to resolve this duplication, and in advance, it uses the _resolvePossibleFunction to resolve possible given function. And of course function is not null but maybe returns an empty string or null

So maybe we can initialize title & content in the end of _configAfterMerge(config) {
after the checks and check after the initialized properties? ?

bootstrap/js/src/tooltip.js

Lines 600 to 605 in c1813ef

if (typeof config.content === 'number') {
config.content = config.content.toString()
}
return config
}

@GeoSot GeoSot moved this from In progress to Review in progress in v5.2.0-stable Jul 4, 2022
@louismaximepiton
Copy link
Member Author

thank you @louismaximepiton for your consistency and for opening again the PR.

Sorry to re-open with so much delay 😖

May I ask one question? The change on line 347 & 448 depends on the other two?

No, it's an independent change for me. This is done to reduce the bundle but I might be wrong. The 2 same lines are called in show() and I couldn't spot any use of this.tip between the actual removal and the removal in show(). So this way, this.tip is still removed but if it is there for the comprehension of the code it might be kept here,

Why, I am asking:

bootstrap/js/src/tooltip.js

Lines 376 to 378 in c1813ef

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

_getTitle is used to resolve this duplication, and in advance, it uses the _resolvePossibleFunction to resolve possible given function. And of course function is not null but maybe returns an empty string or null

So maybe we can initialize title & content in the end of _configAfterMerge(config) { after the checks and check after the initialized properties? ?

I think I might have missed something here. You wanna try something like: config.title = this._getTitle() (or equivalent) to run the function only once per tip declaration and remove the other calls to getTitle?
So the function would be run once, put its result and we only need to check for existence? I have few more questions:

  • Do we need to change the _getContentForTemplate() function ?
  • Since it will be called with _getConfig() only, that is itself called by the constructor only, if the function send a dynamic result, would it be overridden by each time it recreates ?

Thinking about that, I don't know if this was your idea, but if with actual _isWithContent() implementation, we do something like:

this._config.title = this._getTitle()
if (!(this._isWithContent() && this._isEnabled)) { // l.197
  return
}

Then we might need to change _getContentForTemplate() to use this._config.title instead of this._getTitle(). I'll give it a try in the next commit to see if it could work.

@GeoSot
Copy link
Member

GeoSot commented Jul 5, 2022

Do we need to change the _getContentForTemplate() function ?

Probably not


Since it will be called with _getConfig() only, that is itself called by the constructor only, if the function send a dynamic result, would it be overridden by each time it recreates ?

And now we are getting closer to the real problem:
Options:

  1. resolve possible dynamic results on the constructor (getConfig)
    • pros : calls to dynamic calls are done only once, save calculation time
    • cons : possible dynamic calls are not updated
  2. resolve possible dynamic results every time we need them(getConfig)
    - pros : possible dynamic calls are always updated
    - cons : calls to dynamic calls are done and checked every time

So in each case you are facing a different issue, where NOBODY can be pleased . The question here is what is the best of the two available solutions.
I had picked the first, not knowing that there ware cases, devs made changes and wanted the tooltip to be updated automatically

After an issue, I was forced to return to the second option. But if we do not resolve in each check our possibly dynamic calls we will be never sure about their real result

@louismaximepiton
Copy link
Member Author

louismaximepiton commented Jul 5, 2022

So in each case you are facing a different issue, where NOBODY can be pleased . The question here is what is the best of the two available solutions. I had picked the first, not knowing that there ware cases, devs made changes and wanted the tooltip to be updated automatically

After an issue, I was forced to return to the second option. But if we do not resolve in each check our possibly dynamic calls we will be never sure about their real result

It took a long time for me but I think I finally get to your point (I guess 😅) I wonder if it exists a simple solution like: compute title once per rebuild, store the value and use it where it's needed in order to not compute the potential functions several times (as it is done in main). From what I've seen, it would introduce more complexity to the component and maybe increase bundle size. Do you want me to give a last try for a working sample on what I've in mind? Or should it be kept for v5.3.0+?

@GeoSot
Copy link
Member

GeoSot commented Jul 5, 2022

It wouldn't introduce more complexity, but we would fallback to the previous version, with a cached value (just the case I 'solved' in the previous PR). 😔
So for now I suggest just try the one change u proposed on 'setContent' and not something more . (After the release of course you may try any improvement 😉)

@louismaximepiton louismaximepiton force-pushed the force-tooltip-to-refresh-content-on-each-show-action-lmp-fix branch from 78e8db8 to f347907 Compare July 6, 2022 07:11
@GeoSot GeoSot force-pushed the force-tooltip-to-refresh-content-on-each-show-action-lmp-fix branch from 0a5420f to 28f849a Compare July 6, 2022 07:41
v5.2.0-stable automation moved this from Review in progress to Reviewer approved Jul 6, 2022
@GeoSot GeoSot force-pushed the force-tooltip-to-refresh-content-on-each-show-action-lmp-fix branch from 28f849a to 10c25d8 Compare July 11, 2022 08:03
@mdo mdo force-pushed the force-tooltip-to-refresh-content-on-each-show-action-lmp-fix branch from 10c25d8 to 5c96764 Compare July 11, 2022 18:55
@mdo mdo merged commit ed26906 into twbs:main Jul 11, 2022
v5.2.0-stable automation moved this from Reviewer approved to Done Jul 11, 2022
@louismaximepiton louismaximepiton deleted the force-tooltip-to-refresh-content-on-each-show-action-lmp-fix branch September 2, 2022 10:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
v5.2.0-stable
  
Done
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants