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.

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

Add tooltip to progress meters #47

Closed
wants to merge 4 commits into from
Closed

Add tooltip to progress meters #47

wants to merge 4 commits into from

Conversation

at-the-vr
Copy link

Info unrelated to the PR: During local setup, I receive this Could not load the "sharp" module using the win32-x64 runtime error.

Astro                    v4.0.0
Node                     v20.10.0
System                   Windows (x64)
Package Manager          pnpm
Output                   static
Adapter                  none
Integrations             @astrojs/tailwind

commit/PR itself are done via Gitpod in the meantime🙏

@vercel Vercel
Copy link

vercel bot commented Mar 9, 2024 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
astro-badge ✅ Ready (Inspect) Visit Preview Mar 11, 2024 10:07am

delucis
delucis reviewed Apr 9, 2024
View reviewed changes
Copy link
Owner

@delucis delucis left a comment

Choose a reason for hiding this comment

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

Sorry for the delay reviewing this @at-the-vr! Thank you for tackling it 💜

Left one note about the DOM structure. Here are a couple of other considerations:

  1. Currently the tooltip reveals on hover, but it would be good to also make it appear when the link is focused if possible. That way the tooltip would be more accessible to keyboard users.

  2. Small nit, but it should be possible to position the tooltip centrally relative to the bar. Currently it aligns the left edge to the center:
    example of the current tooltip alignment
    Using a -translate-x-1/2 class should do that I think.

src/components/achievements/Stat.astro
Comment on lines 50 to +56
<span class="sr-only">{repoCount} {type}</span>
<span
aria-hidden="true"
class="invisible absolute z-50 -mt-8 rounded bg-gray-100 p-1 text-neutral-700 opacity-0 transition-opacity duration-300 ease-in-out group-hover:visible group-hover:opacity-100"
>
{repoCount.toString()}
</span>
Copy link
Owner

Choose a reason for hiding this comment

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

I think it might be good to use the existing sr-only element here instead of adding an extra one as the current approach impacts accessibility: a screenreader would read for example something like “starlight 22 reviews 22” instead of the current “starlight 22 reviews”.

Tailwind has a not-sr-only class for undoing sr-only styles, so you could probably use a similar approach to what you have now, but applying that class when the hover kicks in.

@at-the-vr
Copy link
Author

Hey Chris, the logic behind making the tooltip appear with link on focus is something I am unable to achieve. Every other suggestion was tried locally and it works but without the focus feature its just getting blocked. I will close the PR on that note

@at-the-vr at-the-vr closed this Apr 16, 2024
@delucis
Copy link
Owner

delucis commented Apr 16, 2024

No problem @at-the-vr! Also, don’t hesitate to mention when you get stuck on something — maybe someone else can help out.

To give you a hint: I probably would have tried moving the group class up to the <li> containing both the link and the bar. And then used the group-focus-within: Tailwind modifier the same way you did group-hover:. That way when the link is focused with the keyboard, the tooltip would be revealed (I think — haven’t tested).

But no pressure to spend time on this if you already feel like you invested enough energy — we only have so much time 💜

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@delucis delucis delucis left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Suggestion: Add mouseover tooltip to the meters on the bottom
2 participants
@at-the-vr @delucis