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 selector usage #36914

Merged
merged 1 commit into from Sep 14, 2022
Merged

Fix tooltip selector usage #36914

merged 1 commit into from Sep 14, 2022

Conversation

GeoSot
Copy link
Member

@GeoSot GeoSot commented Aug 6, 2022

As quick fix as many people seems to use old 'black magic' stuff.

NOTE: Please do not use data-bs-original-title .
It returned, just to support the strange cases where tooltip are created through selector
(explanation: a falsie tooltip is created on a wrapper and adds delegated event listeners),
but for some strange reason, new tooltips are supposed to show the same title as the first ones, or to be initialized from a data-attribute that shouldn't exist (title) after the initialization

closes #36895
ref #36813

@Rezyan
Copy link

Rezyan commented Aug 9, 2022

NOTE: Please do not use data-bs-original-title .

With this fix, we will able to keep selector: '[title]' but we should avoid selector: '[data-bs-original-title]', right? If so, thank you so much, it saves me! ❤

@GeoSot
Copy link
Member Author

GeoSot commented Aug 9, 2022

You may do a test on your code using this script https://deploy-preview-36914--twbs-bootstrap.netlify.app/docs/5.2/dist/js/bootstrap.bundle.min.js and feedback me. I am not so sure that you can use title as selector. Especially if you clone an already initialized instance.

Note: the only sure is that can be reverted just letting title empty, but it is not a valid way to leave the attribute empty

@Rezyan
Copy link

Rezyan commented Aug 9, 2022

You may do a test on your code using this script https://deploy-preview-36914--twbs-bootstrap.netlify.app/docs/5.2/dist/js/bootstrap.bundle.min.js and feedback me. I am not so sure that you can use title as selector. Especially if you clone an already initialized instance.

Note: the only sure is that can be reverted just letting title empty, but it is not a valid way to leave the attribute empty

I think this PR doesn't resolve #36813 (my issue). I setted up https://deploy-preview-36914--twbs-bootstrap.netlify.app/docs/5.2/dist/js/bootstrap.bundle.min.js without success.

Look at this code pen, it's exactly the code I use: https://codepen.io/BrokenSourceCode/pen/PoRBzKv.

screenshot

@GeoSot
Copy link
Member Author

GeoSot commented Aug 10, 2022

Thank you for your test. I reverted the change of title attribute (f156f8c) to almost its initial state, just to avoid inconsistencies, but have in mind to replace this usage as it is invalid. We keep an empty attribute instead of removing it. 😞

@Rezyan
Copy link

Rezyan commented Aug 10, 2022

Thank you for your test. I reverted the change of title attribute (f156f8c) to almost its initial state, just to avoid inconsistencies, but have in mind to replace this usage as it is invalid. We keep an empty attribute instead of removing it. 😞

@GeoSot I have already planned to replace my code. You don't need to revert previous commits for the issue I encountered, not for me.

@GeoSot
Copy link
Member Author

GeoSot commented Sep 1, 2022

@julien-deramond what is your opinion? May I drop 5cc297d or not?

@GeoSot GeoSot marked this pull request as ready for review September 1, 2022 13:08
@GeoSot GeoSot requested a review from a team as a code owner September 1, 2022 13:08
@julien-deramond
Copy link
Member

@julien-deramond what is your opinion? May I drop 5cc297d or not?

IMHO setting the title to an empty string is not a good practice and can lead to bad usage. Since the first commit fixes the 2 issues, I'd drop 5cc297d.

@GeoSot GeoSot force-pushed the gs/fix-tooltip-selector branch 2 times, most recently from 0fb6127 to d7dcbbc Compare September 7, 2022 09:06
Copy link
Member

@julien-deramond julien-deramond left a comment

Choose a reason for hiding this comment

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

Haven't had the time to do more than the following tests to review this PR so that it could be embedded in the v5.2.1:

@GeoSot GeoSot merged commit 3bd5756 into main Sep 14, 2022
@GeoSot GeoSot deleted the gs/fix-tooltip-selector branch September 14, 2022 13:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Tooltips with native HTML title attribute not working on dynamically created elements
3 participants