Skip to content

Add Tooltip component - #547

Merged
mperrotti merged 47 commits into
mainfrom
mp/tooltip
May 2, 2024
Merged

Add Tooltip component#547
mperrotti merged 47 commits into
mainfrom
mp/tooltip

Conversation

@mperrotti

Copy link
Copy Markdown
Contributor

Closes https://github.com/github/primer/issues/3060

Summary

Adds a Tooltip component for showing contextual information when focusing or hovering interactive elements. I made the decision to remove the caret shown in the screenshot in the issue's description. Other Primer designers were happy with this, and it's the same style we use in @primer/react's new Tooltip.

List of notable changes:

added Tooltip component

What should reviewers focus on?

Confirm that the Tooltip looks and behaves as expected.

Steps to test:

  1. Open the Tooltip component in the CI-deployed preview environment
  2. Check out all of the features demo'd in the "Features" directory or use Storybook's controls to play with the "Playground" story
  3. Verify that the Tooltip behaves as described in the following issue.

Contributor checklist:

  • All new and existing CI checks pass
  • Tests prove that the feature works and covers both happy and unhappy paths
  • Any drop in coverage, breaking changes or regressions have been documented above
  • New visual snapshots have been generated / updated for any UI changes
  • All developer debugging and non-functional logging has been removed
  • Related issues have been referenced in the PR description

Reviewer checklist:

  • Check that pull request and proposed changes adhere to our contribution guidelines and code of conduct
  • Check that tests prove the feature works and covers both happy and unhappy paths
  • Check that there aren't other open Pull Requests for the same update/change

Screenshots:

Please try to provide before and after screenshots or videos

Before After

N/A

Rewatch.Screen.Recording.-.2024-03-01.at.7.10.mp4

@changeset-bot

changeset-bot Bot commented Mar 6, 2024

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 66059ff

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 6 packages
Name Type
@primer/brand-primitives Minor
@primer/react-brand Minor
@primer/brand-e2e Minor
@primer/brand-fonts Minor
@primer/brand-config Minor
@primer/brand-storybook Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions

github-actions Bot commented Mar 6, 2024

Copy link
Copy Markdown
Contributor

🟢 No design token changes found

@github-actions

github-actions Bot commented Mar 6, 2024

Copy link
Copy Markdown
Contributor

⚠️ Visual differences found

Our visual comparison tests found UI differences.

Please review the differences by using the test artifacts to ensure that the changes were intentional.

Artifacts can be downloaded and reviewed locally.

Download links are available at the bottom of the workflow summary screen.

Example:

artifacts section of workflow run

If the changes are expected, please run npm run test:visual:update-snapshots to replace the previous fixtures.

Review visual differences

@nsolerieu

Copy link
Copy Markdown
Contributor

@mperrotti a few questions for ya:

  • Do we need the North East, North West, Southeast, Southwest - is there a reason why we'd not have only top/bottom/left/right?

  • Should we consider a min-width to match the target element width for top/bottom variants?

Screenshot 2024-03-07 at 11 43 11 AM
  • Have you considered adding a triangle/cursor?
Screenshot 2024-03-07 at 11 39 03 AM

@mperrotti

Copy link
Copy Markdown
Contributor Author

Do we need the North East, North West, Southeast, Southwest - is there a reason why we'd not have only top/bottom/left/right?

We don't strictly need those additional options, but I think it's nice to have them as options. If you and other designers want to limit the options, we can remove them.


Should we consider a min-width to match the target element width for top/bottom variants?

This is possible, but I'd rather not do this. I don't see what we'd gain by doing this, and I like that the tooltip is sized to "shrink-wrap" its content. I'm also concerned about having a noticeable performance hit on pages with many tooltips. Whenever you measure a DOM node, the browser needs to perform a reflow.


Have you considered adding a triangle/cursor?

Yes. We used to have that triangle in @primer/react's old Tooltip component, but we chose to remove it for aesthetic reasons. I personally prefer the version without the caret.

@nsolerieu

Copy link
Copy Markdown
Contributor

@mperrotti Thank you for your answers, that all make sense. I like the minimalism and flexibility of this new version.
LGTM 🟢

@mperrotti
mperrotti marked this pull request as ready for review March 12, 2024 15:44
@mperrotti
mperrotti requested a review from rezrah March 12, 2024 15:44
@nsolerieu

Copy link
Copy Markdown
Contributor

@mperrotti following up here this as I brought this the the broader team for review.

We have review and decided that the proposed component is too small for marketing contexts.
We understand that this is a product pattern trickling down to brand and would like to consider this difference of context in the definition of the attributes of the component.

We'd like the see a revised version including:

  • A caret for visual anchor
  • Larger font size and lateral padding
  • Limit the positions to top, left, right, bottom

@nsolerieu
nsolerieu self-requested a review March 13, 2024 20:48
@mperrotti

Copy link
Copy Markdown
Contributor Author

@nsolerieu - the screen recording is out of date, so the tooltip is actually larger. So the only updates will be:

  • add a caret
  • limit the positions to top, left, right, and bottom

I'll ping you when it's ready for another review.

@nsolerieu nsolerieu left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thank you for this new version @mperrotti

Screenshot 2024-03-14 at 11 13 27 AM

LGTM 🟢

@rezrah

rezrah commented Apr 22, 2024

Copy link
Copy Markdown
Collaborator

Thanks for the updated review @rezrah !

Can't see the a documentation page. Planning to add this now or after?

I think this should be a separate PR. Do we need to create an issue?

Maybe capture it on a tasklist on the tracking issue? We usually add it to the same PR, but if you need to add it in follow-up PR that's fine too 👍.

Btw, I'm referring to the React component docs, not the interface guidelines.

@mperrotti

Copy link
Copy Markdown
Contributor Author

Ahh the React docs. Yes I can add those here.

@mperrotti

Copy link
Copy Markdown
Contributor Author

@rezrah - I added React docs, but I'm getting this error:

ReferenceError: Tooltip is not defined

Can you confirm that I'm doing my exports from packages/react correctly?

Also, I'm having trouble running Gatsby locally to test my docs locally, so it's hard to debug.

Comment thread apps/docs/content/components/Tooltip.mdx Outdated
Comment on lines +44 to +48
```jsx live
<Tooltip text="Go fullscreen" type="label">
<Button hasArrow={false}><ScreenFullIcon /></Button>
</Tooltip>
```

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

i think we should go without the live previews here, as they don't work in Gatsby

Screenshot 2024-05-02 at 16 14 03

@rezrah rezrah left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

🙌 Nice one @mperrotti, this looks great.

Just some minor, additional feedback around docs but otherwise LGTM 👍 :shipit:

mperrotti and others added 2 commits May 2, 2024 11:34
Co-authored-by: Rez <13340707+rezrah@users.noreply.github.com>
@mperrotti
mperrotti merged commit 9cef031 into main May 2, 2024
@mperrotti
mperrotti deleted the mp/tooltip branch May 2, 2024 18:36
@primer-css primer-css mentioned this pull request May 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants