-
-
Notifications
You must be signed in to change notification settings - Fork 576
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
feat: show tag hover card when hovering cursor on hashtag links #2621
Conversation
Run & review this pull request in StackBlitz Codeflow. |
✅ Deploy Preview for elk-docs canceled.
|
✅ Deploy Preview for elk-zone ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
() => [props.tagName, targetIsVisible.value] satisfies WatcherType, | ||
async ([newTagName, newVisible]) => { | ||
if (newTagName) { | ||
tag.value = await fetchTag(newTagName) |
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.
Ensure you dont fetch it if not in screen
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.
the browser will hang
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.
Good catch! The original implementation was fetching API too aggressively.😅
I moved the fetchTag()
to TagCard
component to ensure that it's only called when showing the card.
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.
I'm going to take a look at this...
@patak-dev I adjusted the layout a bit. What do you think? |
@@ -32,13 +40,13 @@ function go(evt: MouseEvent | KeyboardEvent) { | |||
|
|||
<template> | |||
<div | |||
block p4 hover:bg-active flex justify-between cursor-pointer | |||
block p4 hover:bg-active flex justify-between cursor-pointer flex-gap-2 |
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.
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.
I think for long tags the start should be top aligned
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.
I'd like the tag to always be aligned with the text below. I proposed having the star to the right, but I see now that it isn't a good idea. Maybe we should have the star always at the top left, and then the tag and the text below to the right of it. Or the star could be at the right of the graph, kind of like the Follow button in the notification cards for people following you
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.
OK, I tried another layout by making the follow button an independent block and placing it on the left. This ensures both texts are aligned and more robust with a long hashtag.
For Hovercard, items-center
looks lined up better than the top. For Hashtag, both look fine.
@patak-dev Which position do you prefer if we use this layout?
Hovercard
align center (items-center
)
align top (items-start
)
Hashtag page
Hmm, I'm not sure why snapshot tests start revealing I wonder if it's safe to update the snapshot test. 🤔 |
We'll need to mock it, I'm going to fix it |
@shuuji3 your local deps are stale, run Try updating main and update your branch, then |
@shuuji3 the logic seems to be wrong, we should avoid fetching the tag until visible in the viewport, the browser will hang if lot of tags, the result is not deterministic since it depends on the current timeline content |
yes, but this is wrong, the tagName will not prevent to fetch the tag once set, you have the fetch with toplevel await in the TagCard script setup |
Thanks for playing with the design of the card, I think it looks a lot better now! 🧡 |
Updated to the new layout (used |
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.
Awesome!
I see it is still a draft, mark it as ready when you think we should merge it
Ah, actually, the draft change was intentional (close/reopen was my mistake 😅). Thanks to userquin, the issue where multiple tag fetch caused the race condition and hanging up timeline was solved, and also duplicated calls fr the same tag are properly cached now. However, it seems that the current This is problematic because this endpoint has 300 calls / 5 min. rate limit similar to other API endpoints so if the user scrolls in a short period and sees more than 300 tags, it will exceed the rate limit and the endpoint will start returning 429 (too many requests) during 5 min. window. So I need to find a way not to call the tag endpoint until the user hovers the cursor on the tag link. |
Now, tag-hovering.mp4 |
I had to update the same test snapshot again for strange |
The bdi content should be there I'm going to check what's wrong in the content-render module |
TODO: show spinner before receiving the actual tag data. |
Check #2632 |
There was a good skeleton component for the tag: https://github.com/elk-zone/elk/blob/main/components/tag/TagCardSkeleton.vue elk-taghover-skeleton.mp4 |
Also, there was a useful |
OK, I think it's ready to merge. elk-taghover-skeleton.mp4 |
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.
Amazing work! 👏🏼
This implements the hovercard feature for hashtags.
I think there are several benefits of tag hovercard:
Screenshots