Skip to content

Hide tooltips on touchend event#5830

Merged
iansan5653 merged 11 commits into
mainfrom
hide-tooltips-on-tap
Apr 3, 2025
Merged

Hide tooltips on touchend event#5830
iansan5653 merged 11 commits into
mainfrom
hide-tooltips-on-tap

Conversation

@iansan5653

@iansan5653 iansan5653 commented Mar 27, 2025

Copy link
Copy Markdown
Contributor

Closes https://github.com/github/primer/issues/4431 (see issue for context)

Adds an event handler to TooltipV2 to hide the tooltip on touch end, so that tooltips automatically hide when the target element is tapped. This prevents tooltips from remaining visible after a button is interacted with, while still allowing touch device users to see tooltips via tapping and holding.

This required converting the onMouseEnter handler to an onMouseOverCapture handler so that it would run first so that the onTouchEnd handler could cancel the pending 50ms timeout. Alternatively I could have wrapped the onTouchEnd handler in its own timeout, but that felt hacky and like a race condition waiting to happen.

I confirmed that this fixes the problem on mobile devices (note I also confirmed that you can view the tooltips with an intentional long tap, but that was hard to capture on video):

screen-20250328-124320.mp4

And does not regress showing the tooltip on hover/focus:

Screen.Recording.2025-03-28.at.1.12.34.PM.mov

Changelog

New

Changed

  • Hide TooltipV2 tooltips on touch-end

Removed

Rollout strategy

  • Patch release
  • Minor release
  • Major release; if selected, include a written rollout or migration plan
  • None; if selected, include a brief description as to why

Testing & Reviewing

Merge checklist

@iansan5653 iansan5653 requested a review from a team as a code owner March 27, 2025 14:34
@iansan5653 iansan5653 requested a review from siddharthkp March 27, 2025 14:34
@changeset-bot

changeset-bot Bot commented Mar 27, 2025

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 5d4987d

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

This PR includes changesets to release 1 package
Name Type
@primer/react 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

@iansan5653 iansan5653 marked this pull request as draft March 27, 2025 14:34
@github-actions github-actions Bot added the integration-tests: recommended This change needs to be tested for breaking changes. See https://arc.net/l/quote/tdmpakpm label Mar 27, 2025
@github-actions

Copy link
Copy Markdown
Contributor

👋 Hi, this pull request contains changes to the source code that github/github depends on. If you are GitHub staff, we recommend testing these changes with github/github using the integration workflow. Thanks!

@github-actions

github-actions Bot commented Mar 27, 2025

Copy link
Copy Markdown
Contributor

size-limit report 📦

Path Size
packages/react/dist/browser.esm.js 104.93 KB (-0.05% 🔽)
packages/react/dist/browser.umd.js 105.37 KB (+0.03% 🔺)

@github-actions github-actions Bot requested a deployment to storybook-preview-5830 March 27, 2025 14:39 Abandoned
@github-actions github-actions Bot requested a deployment to storybook-preview-5830 March 27, 2025 14:44 Abandoned
@github-actions github-actions Bot temporarily deployed to storybook-preview-5830 March 27, 2025 14:59 Inactive
@iansan5653 iansan5653 force-pushed the hide-tooltips-on-tap branch from 4b9f5a3 to 072db09 Compare March 27, 2025 15:16
@github-actions github-actions Bot requested a deployment to storybook-preview-5830 March 27, 2025 15:20 Abandoned
@github-actions github-actions Bot temporarily deployed to storybook-preview-5830 March 27, 2025 15:34 Inactive
@iansan5653

Copy link
Copy Markdown
Contributor Author

I really tried hard to write tests for this, but I can't find a way to assert that the tooltip is open/closed in Jest in CI. I tried testing for toBeVisible, matches(":popover-open"), and toHaveClass(":popover-open") but none of them worked (the last did work locally but not in CI). I guess this is why there are no existing tests for popover visibility.

@github-actions github-actions Bot requested a deployment to storybook-preview-5830 March 31, 2025 17:15 Abandoned
@github-actions github-actions Bot requested a deployment to storybook-preview-5830 March 31, 2025 17:20 Abandoned
@github-actions github-actions Bot temporarily deployed to storybook-preview-5830 March 31, 2025 17:36 Inactive
@iansan5653 iansan5653 marked this pull request as ready for review March 31, 2025 17:53
@primer-integration

Copy link
Copy Markdown

👋 Hi from github/github! Your integration PR is ready: https://github.com/github/github/pull/370353

@primer-integration

Copy link
Copy Markdown

🟢 golden-jobs completed with status success.

@github-actions github-actions Bot added integration-tests: passing Changes in this PR do NOT cause breaking changes in gh/gh and removed integration-tests: recommended This change needs to be tested for breaking changes. See https://arc.net/l/quote/tdmpakpm labels Mar 31, 2025
Comment thread .changeset/chilly-jars-jog.md Outdated
onBlur={[Function]}
onClick={[MockFunction]}
onFocus={[Function]}
onMouseEnter={[Function]}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Not sure if this will be problematic in integrations in dotcom?

@francinelucca francinelucca left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM!

Co-authored-by: Jon Rohan <yes@jonrohan.codes>
@iansan5653 iansan5653 enabled auto-merge April 3, 2025 18:48
@github-actions

github-actions Bot commented Apr 3, 2025

Copy link
Copy Markdown
Contributor

👋 Hi, there are new commits since the last successful integration test. We recommend running the integration workflow once more, unless you are sure the new changes do not affect github/github. Thanks!

@github-actions github-actions Bot added the integration-tests: recommended This change needs to be tested for breaking changes. See https://arc.net/l/quote/tdmpakpm label Apr 3, 2025
@iansan5653 iansan5653 added this pull request to the merge queue Apr 3, 2025
@github-actions github-actions Bot temporarily deployed to storybook-preview-5830 April 3, 2025 19:02 Inactive
Merged via the queue into main with commit f8760fe Apr 3, 2025
@iansan5653 iansan5653 deleted the hide-tooltips-on-tap branch April 3, 2025 19:12
@primer primer Bot mentioned this pull request Apr 3, 2025
jonrohan added a commit that referenced this pull request Apr 10, 2025
Co-authored-by: Jon Rohan <yes@jonrohan.codes>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

integration-tests: passing Changes in this PR do NOT cause breaking changes in gh/gh integration-tests: recommended This change needs to be tested for breaking changes. See https://arc.net/l/quote/tdmpakpm

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants