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

feat: add tooltip component #2644

Merged
merged 12 commits into from
Feb 18, 2025
Merged

feat: add tooltip component #2644

merged 12 commits into from
Feb 18, 2025

Conversation

artursantiago
Copy link
Collaborator

@artursantiago artursantiago commented Jan 28, 2025 •
edited
Loading

What's the purpose of this pull request?

This pull request adds the Tooltip atom component. This component will be used in the ReviewCard for displaying reviews of a product in the new Reviews and Ratings section.

How does it work?

This PR introduces a new molecule component, Tooltip, which allows customization of its behavior and appearance. The component includes default values for some of its props: placement is "top", dismissable is false, and maxWidth is 300.

When dismissable is enabled, the tooltip uses an IconButton for the dismiss action.

How to test it?

Starters Deploy Preview

Preview

References

JIRA TASK: SFS-2070

Figma

image

@artursantiago artursantiago added the enhancement New feature or request label Jan 28, 2025
@artursantiago artursantiago self-assigned this Jan 28, 2025
@vercel Vercel
Copy link

vercel bot commented Jan 28, 2025 •
edited
Loading

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

Name Status Preview Updated (UTC)
faststore-site ✅ Ready (Inspect) Visit Preview Jan 28, 2025 5:45pm

@codesandbox-ci CodeSandbox CI
Copy link

codesandbox-ci bot commented Jan 28, 2025 •
edited
Loading

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

@artursantiago artursantiago mentioned this pull request Jan 28, 2025
Merged
@artursantiago artursantiago marked this pull request as ready for review January 30, 2025 17:43
@artursantiago artursantiago requested a review from a team as a code owner January 30, 2025 17:43
@artursantiago artursantiago requested review from eduardoformiga and lucasfp13 and removed request for a team January 30, 2025 17:43
@artursantiago artursantiago force-pushed the feat/add-tooltip-component branch from d454cc5 to 1269495 Compare February 4, 2025 18:36
@artursantiago artursantiago mentioned this pull request Feb 13, 2025
Draft
2 tasks
@Guilera Guilera force-pushed the feat/add-tooltip-component branch from e64bc11 to a8e2f93 Compare February 14, 2025 14:12
@artursantiago artursantiago force-pushed the feat/add-tooltip-component branch from a8e2f93 to d19ab92 Compare February 14, 2025 17:01
hellofanny
hellofanny reviewed Feb 17, 2025
View reviewed changes
packages/components/src/molecules/Tooltip/Tooltip.tsx
/**
* If the tooltip can be closed by a button.
*/
dismissible?: boolean
Copy link
Contributor

Choose a reason for hiding this comment

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

@renatamottam do we really need to have this dismissible prop?

The tooltip is expected to disappear when the focus is lost or on mouse out.
And according to this accessibility doc, it shouldn't contain interactive elements 🤔 So adding a button would be a problem (will add focus to the button). WDYT?

Ps: Add a big notice to the doc: this component should only be used for additional information and not for crucial content, as tooltip content may not be accessible to screen readers and could go unnoticed.

hellofanny
hellofanny requested changes Feb 17, 2025
View reviewed changes
Copy link
Contributor

@hellofanny hellofanny left a comment

Choose a reason for hiding this comment

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

I'm suggesting some a11y updates. It might be best to apply all the changes at once, test them locally, and then make any necessary adjustments ⭐ Thank you!

@artursantiago artursantiago force-pushed the feat/add-tooltip-component branch from 3dc1acf to 22635fd Compare February 17, 2025 20:13
hellofanny
hellofanny reviewed Feb 17, 2025
View reviewed changes
packages/components/src/molecules/Tooltip/Tooltip.tsx Outdated

return (
<div
data-fs-tooltip-wrapper
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
data-fs-tooltip-wrapper
data-fs-tooltip

I know this might be a bit confusing, but I think it's best to maintain the existing pattern. The outer element should be named data-fs-component-name. Can you please, rename this element to data-fs-tooltip, and rename the current data-fs-tooltip to data-fs-tooltip-wrapper? Also update in the stylesheet.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done ✅

@hellofanny
Copy link
Contributor

I've tested the focus and Escape key functionality, and they are working as expected. I didn't test all placement variants, but I tried a few, and they worked fine.

I'll just wait for this update before approving this PR!

image

@hellofanny hellofanny self-requested a review February 18, 2025 14:00
hellofanny
hellofanny approved these changes Feb 18, 2025
View reviewed changes
Copy link
Contributor

@hellofanny hellofanny left a comment

Choose a reason for hiding this comment

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

well done!! 🚀

@hellofanny hellofanny merged commit ed1826d into main Feb 18, 2025
8 of 9 checks passed
@hellofanny hellofanny deleted the feat/add-tooltip-component branch February 18, 2025 14:08
@hellofanny
Copy link
Contributor

@mariana-caetano JLYK I've created a doc request for this component. It would be better to double check this component in the main when creating the doc.

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

@pedromtec pedromtec pedromtec approved these changes

@hellofanny hellofanny hellofanny approved these changes

@eduardoformiga eduardoformiga Awaiting requested review from eduardoformiga eduardoformiga is a code owner automatically assigned from vtex/faststore

@lucasfp13 lucasfp13 Awaiting requested review from lucasfp13 lucasfp13 is a code owner automatically assigned from vtex/faststore

Assignees

@artursantiago artursantiago

Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@artursantiago @hellofanny @pedromtec