Skip to content

Fix tooltip selector usage #36914

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
Merged
GeoSot merged 1 commit into from
Sep 14, 2022
Merged

Fix tooltip selector usage #36914

GeoSot merged 1 commit into from
Sep 14, 2022

Conversation

@GeoSot
Copy link
Member

@GeoSot GeoSot commented Aug 6, 2022 •
edited
Loading

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

@GeoSot GeoSot mentioned this pull request Aug 6, 2022
Merged
@ghost
Copy link

ghost commented Aug 9, 2022 •
edited by ghost
Loading

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

@ghost
Copy link

ghost commented Aug 9, 2022 •
edited by ghost
Loading

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 GeoSot force-pushed the gs/fix-tooltip-selector branch from f156f8c to c5194da Compare August 10, 2022 22:05
@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. 😞

@ghost
Copy link

ghost commented Aug 10, 2022 •
edited by ghost
Loading

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 GeoSot force-pushed the gs/fix-tooltip-selector branch from c5194da to 5cc297d Compare September 1, 2022 07:37
@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
@GeoSot GeoSot force-pushed the gs/fix-tooltip-selector branch from 5cc297d to 12e3e60 Compare September 6, 2022 21:57
@julien-deramond
Copy link
Member

julien-deramond commented Sep 7, 2022

@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
julien-deramond
julien-deramond approved these changes Sep 7, 2022
View reviewed changes
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:

  • #36895 is well fixed and represented by a visual test
  • Non-regression testing OK for tooltips and popovers in the documentation
  • Only read the source code that seems OK but haven't tested it in details. Sorry for that

@GeoSot GeoSot force-pushed the gs/fix-tooltip-selector branch from d7dcbbc to 8801dba Compare September 13, 2022 16:47
@GeoSot GeoSot force-pushed the gs/fix-tooltip-selector branch from 8801dba to 34f3e00 Compare September 14, 2022 13:13
@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

Reviewers

@julien-deramond julien-deramond julien-deramond approved these changes

Assignees

No one assigned

Labels

js v5

Projects

No open projects
v5.2.2
Status: Done

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

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

3 participants

@GeoSot @julien-deramond