-
Notifications
You must be signed in to change notification settings - Fork 150
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
Only render tooltip on hover #368
Conversation
Codecov Report
@@ Coverage Diff @@
## master #368 +/- ##
=======================================
Coverage 19.64% 19.64%
=======================================
Files 11 11
Lines 2708 2708
=======================================
Hits 532 532
Misses 2138 2138
Partials 38 38 Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks for the backwards compatible check
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested and passed.
- Works as expected when corresponding webapp PR is in place
- No regressions found on older server versions
See mattermost/mattermost-webapp#6492 fore details.
LGTM!
Summary
Make use of the
show
prop added in mattermost/mattermost-webapp#6492 by only rendering the tooltip on hover. This means data is not longer pre-fetched. Therefore a small delay is visible when hovering. Please note, on re-hovering the link the tooltip is shown imminently. Switching channel caused the data to be lost and it's required to fetch is again. I opted for this approach because storing the data in redux would mean that we would have to clean it up from time to time and it's hard to figure out what data should be cleaned up.The change is backward compatible to
I did also simplify the existing code a bit.
Ticket Link
https://mattermost.atlassian.net/browse/MM-28591