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

Tooltip refactoring #32523

Merged
merged 5 commits into from Jan 27, 2021
Merged

Tooltip refactoring #32523

merged 5 commits into from Jan 27, 2021

Conversation

GeoSot
Copy link
Member

@GeoSot GeoSot commented Dec 18, 2020

some minor refactoring,plus a possible solution to focusIn glitch (as @XhmikosR pinpointed )

Preview : https://deploy-preview-32523--twbs-bootstrap.netlify.app/docs/5.0/components/tooltips/

@GeoSot GeoSot requested a review from a team as a code owner December 18, 2020 03:32
js/src/tooltip.js Outdated Show resolved Hide resolved
@XhmikosR
Copy link
Member

@GeoSot thank you very much for digging into this! That being said, can you please split the fix for #32372 to a separate PR?

While testing this patch I notice that indeed the #32372 issue is fixed, but there are cases the arrow momentarily shows up in the wrong direction:

2020-12-18_08-05-38

@GeoSot
Copy link
Member Author

GeoSot commented Dec 18, 2020

I've reverted the possible fix and left the rest here

@GeoSot GeoSot changed the title Tooltip refactorings and maybe focusIn solution Tooltip refactorings Dec 18, 2020
@XhmikosR XhmikosR added this to Inbox in v5.0.0-beta3 via automation Dec 19, 2020
@XhmikosR XhmikosR changed the title Tooltip refactorings Tooltip refactoring Dec 21, 2020
@XhmikosR XhmikosR requested review from rohit2sharma95 and removed request for a team January 13, 2021 20:25
Copy link
Collaborator

@rohit2sharma95 rohit2sharma95 left a comment

Choose a reason for hiding this comment

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

This PR decreases the size of the un-minified file (-80B) and increases the size of the minified file (+28B).

js/src/tooltip.js Outdated Show resolved Hide resolved
js/src/tooltip.js Outdated Show resolved Hide resolved
js/src/tooltip.js Outdated Show resolved Hide resolved
js/src/tooltip.js Outdated Show resolved Hide resolved
@GeoSot GeoSot force-pushed the tooltip-changes branch 2 times, most recently from 2b02d49 to 4c04ce3 Compare January 18, 2021 09:17
v5.0.0-beta3 automation moved this from Inbox to Approved Jan 18, 2021
@GeoSot
Copy link
Member Author

GeoSot commented Jan 18, 2021

@rohit2sharma95 I am glad, this request was approved. But can we ensure the 'this' scope thing? (maybe on another request)
We all know by theory, it has to be the same scope on arrow functions, but firefox debugger can be lying?

@rohit2sharma95
Copy link
Collaborator

True, wired behavior on firefox (Not getting the reference while hovering over this) 😄
But if you see the previous method in the call stack, you will find the context for this (in the dist file). Notice _this5 in the image below:

image

@XhmikosR
Copy link
Member

So, basically, the only logic change is that in toggle() method, context didn't have the extra check:

-context = Data.getData(event.delegateTarget, dataKey)
+context = context || Data.getData(event.delegateTarget, dataKey)

I don't suppose this could cause any issues?

@XhmikosR
Copy link
Member

@rohit2sharma95 any thoughts about #32523 (comment)?

@XhmikosR XhmikosR added this to Inbox in v5.0.0-beta2 via automation Jan 26, 2021
@XhmikosR XhmikosR removed this from Approved in v5.0.0-beta3 Jan 26, 2021
@XhmikosR XhmikosR moved this from Inbox to Approved in v5.0.0-beta2 Jan 26, 2021
@rohit2sharma95
Copy link
Collaborator

The second parameter is optional in _initializeOnDelegatedTarget method. So still the second expression (Data.getData(event.delegateTarget, dataKey)) is being used for the toggle method. LGTM 👍

@XhmikosR XhmikosR merged commit 5d7b51e into twbs:main Jan 27, 2021
v5.0.0-beta2 automation moved this from Approved to Done Jan 27, 2021
@GeoSot GeoSot deleted the tooltip-changes branch March 3, 2021 08:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
v5.0.0-beta2
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants