-
Notifications
You must be signed in to change notification settings - Fork 6.3k
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: Introducing avatar tooltip #7143
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Does it work on mobile? I've tried from my iphone and it didn't work from the preview. |
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.
First look, it's great but maybe have separate type for avatar/ frontmatter/intermediate objects
apps/site/components/Common/AvatarGroup/Avatar/index.module.css
Outdated
Show resolved
Hide resolved
After some test on desktop few point:
|
This is the design choice of the radix tooltip component I use, and IMO it is the right choice (radix-ui/primitives#955 (comment)) Here, as an alternative, it may be better to redirect directly when a profile image is clicked on mobile (like GitHub), but the current profile images are too small for mobile resolution, I will check the accessibility and try to implement a better solution👍 |
Lighthouse Results
|
IMO, double can do the job and increase little bit the size of the avatar for mobile |
@canerakdas can you resolve the conflicts? 👀 |
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've finally done the review :)
Co-authored-by: Claudio W <[email protected]> Signed-off-by: Caner Akdas <[email protected]>
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!
Awesome work here, @canerakdas, thank you so much!! <3 <3 |
yeah it's super cool ! thanks @canerakdas ! |
* feat: tooltip component and avatar tooltip * chore: metabar story props updated * chore: self review * chore: self review * refactor: class names * refactor: horizontal margin added * feat: accessible avatars on mobile * feat: default author url * refactor: review updates * chore: self review * refactor: design and review updates * fix: Avatars in MetaBar story * refactor: review updates * fix: opening the tooltip portal within the dialog * fix: adjusting visible avatar count * refactor: review updates * Update apps/site/util/authorUtils.ts Co-authored-by: Claudio W <[email protected]> Signed-off-by: Caner Akdas <[email protected]> * refactor: enhancing code readability * refactor: review update --------- Signed-off-by: Caner Akdas <[email protected]> Co-authored-by: Claudio W <[email protected]>
Description
I saw it in Slack (OpenJS Foundation) threads and thought it was a good way to support contributors
With this PR, the
Tooltip
component was created and minor refactors were made toAvatar
andAvatarGroup
components. In addition, theWithAvatarGroup
component was created to make avatars easier to manage to simplify the processI took care to use the elements/components in the existing design system as a design, I was inspired by (Search typing) in the Figma for the Tooltip component;
https://www.figma.com/design/pu1vZPqNIM7BePd6W8APA5/Node.js?node-id=464-6397&node-type=frame&t=dj6OGYBZLLo5cp4z-0
Validation
authors.json
whose website field is existExample screenshot;
Related Issues
Related to #7141
Check List
npm run format
to ensure the code follows the style guide.npm run test
to check if all tests are passing.npx turbo build
to check if the website builds without errors.