FIX: hide tooltips when scrolling on mobile #23098
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
AndrewPrigorshnev
force-pushed
the
feature/hide-d-tooltip-when-scrolling-on-mobile
branch
4 times, most recently
from
1f781f9
to
fb4aabe
Compare
AndrewPrigorshnev
changed the title
|
This pull request has been mentioned on Discourse Meta. There might be relevant details there: https://meta.discourse.org/t/custom-status-message-in-front-of-by-header-on-scroll/273320/11 |
AndrewPrigorshnev
force-pushed
the
feature/hide-d-tooltip-when-scrolling-on-mobile
branch
from
29a484b
to
fd7b4f3
Compare
jjaffeux
approved these changes
Aug 23, 2023
|
As I told you I will most likely replace most of this soon, but that's a good use case for me to check. |
AndrewPrigorshnev
force-pushed
the
feature/hide-d-tooltip-when-scrolling-on-mobile
branch
from
fd7b4f3
to
4226a5e
Compare
|
This pull request has been mentioned on Discourse Meta. There might be relevant details there: https://meta.discourse.org/t/custom-status-message-in-front-of-by-header-on-scroll/273320/17 |
AndrewPrigorshnev
deleted the
feature/hide-d-tooltip-when-scrolling-on-mobile
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This fixes the problem reported in https://meta.discourse.org/t/custom-status-message-in-front-of-by-header-on-scroll/273320.
This problem can be reproduced with any tooltip created using the DTooltip component or the createDTooltip function.
The problem happens because the trigger for tooltip on mobile is click, and for tooltip to disappear the user has to click outside the tooltip. This is the default behavior of tippy.js – the library we use under the hood.
Note that this PR fixes the problem in topics, but not in chat. I'm going to investigate and address it in chat in a following PR.
To fix it for tooltips created with the createDTooltip function, I had to make a refactoring. We had a somewhat not ideal solution there, we were leaking an implementation detail by passing tippy instances to calling sides, so they could then destroy them. With this fix, I would have to make it more complex, because now we need to also remove
onScroolhandlers, and I would need to leak this implementation detail too. So, I firstly refactored the current solution in 5a4af05 and then addedonScrollhandlers in fb4aabe (cc @jancernik).When refactoring this, I was running locally some temporarily skipped flaky tests. Turned out they got a bit outdated, so I fixed them. Note that I'm not unskipping them in this commit, we'll address them separately later.