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

IBX-4005: Broken tooltips.hideAll() function #599

Merged
merged 4 commits into from
Oct 14, 2022

Conversation

GrabowskiM
Copy link
Contributor

Question Answer
Tickets https://issues.ibexa.co/browse/IBX-4005
Bug fix? yes
New feature? no
BC breaks? no
Tests pass? yes
Doc needed? no
License GPL-2.0

In case of testing it on main branch, it requires new installation with fixed tag for admin-ui-assets (wrong BS version installed)

Checklist:

  • Coding standards ($ composer fix-cs)
  • Ready for Code Review

@@ -132,6 +132,8 @@
</div>`,
});

tooltipNode.title = '';
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you explain how it works? :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We were always using [title] selector to get all nodes for tooltips, both for parsing and hiding all tooltips. So far Bootstrap was using .setAttribute('title', '') in its constructor, but in 5.2.0 they introduced .removeAttribute('title') instead, which removed title parameter, I set it back here, so that I could still use same selector for getting all tooltips

Copy link
Contributor

Choose a reason for hiding this comment

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

Really appreciate your explanation! 😊

Copy link
Contributor

Choose a reason for hiding this comment

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

If you parse a second time the same tooltipNode and title is the empty string will bootstrap change the tooltip to empty string?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not anymore, after last commit ;)


tooltipNode.dataset.originalTitle = tooltipNode.title;
tooltipNode.dataset.originalTitle = tooltipInstance ? tooltipInstance._getTitle() : tooltipNode.title;

new bootstrap.Tooltip(tooltipNode, {
Copy link
Contributor

@tischsoic tischsoic Oct 12, 2022

Choose a reason for hiding this comment

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

We are creating a tooltip instance even in a case in which tooltipInstance returned by getInstance is not null? Isn't this the duplication of instances? 🤔

@GrabowskiM GrabowskiM requested a review from tischsoic October 13, 2022 08:05
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@bogusez bogusez self-assigned this Oct 14, 2022
@dew326 dew326 merged commit 14b1f21 into 4.2 Oct 14, 2022
@dew326 dew326 deleted the IBX-4005-fix-after-bs-update-to-tooltips branch October 14, 2022 10:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants