Add delay functionality to tooltips #6889
Conversation
🦋 Changeset detectedLatest commit: 52f36a8 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
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
bot
added
staff
|
👋 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! |
|
|
||
| export const DelayedTooltip = () => ( | ||
| <Tooltip text="This is a tooltip with 600ms delay" delay={600}> | ||
| <IconButton icon={HeartIcon} aria-label="HeartIcon" /> |
There was a problem hiding this comment.
Instead of adding another prop to the IconButton, I thought we can use external tooltip if we want to delay the tooltip, as it feels like it is not as common as changing directions where we have a dedicated prop on icon button -- but totally open to feedback 🙌🏻
| * Delay in milliseconds before showing the tooltip | ||
| * @default 50 | ||
| */ | ||
| delay?: number |
There was a problem hiding this comment.
I really think this is a risky thing to do.
Here's my preference (in order), we should get primer designers to weigh in
- If we can, have the delay baked into the component instead of instance
- If we do have to add a delay on an instance, it's one fixed number, not something you have to decide. (
delayed: booleaninstead ofdelay: number)
There was a problem hiding this comment.
Yeah totally fair. I am not comfortable to have this as a free text field either, just wanted to push something as we are not super clear about the direction 😄
…0ms else 50ms" This reverts commit f021444.
github-actions
bot
requested a deployment
to
storybook-preview-6889
Tooltip now supports delay functionality with options for instant, medium, and long delays.
There was a problem hiding this comment.
Pull Request Overview
Adds delay functionality to tooltips with three predefined options: instant (50ms, default), medium (400ms), and long (1200ms). This enhancement provides better control over tooltip timing, particularly useful for icon buttons where immediate tooltips might be distracting when users are simply moving their mouse across elements.
- Added a new
delayprop to the Tooltip component with enum values for different timing options - Implemented delay mapping logic that converts string values to milliseconds
- Created Storybook examples demonstrating the new delay functionality
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| packages/react/src/TooltipV2/Tooltip.tsx | Added delay prop, mapping object, and implementation of delay functionality |
| packages/react/src/TooltipV2/Tooltip.features.stories.tsx | Added stories demonstrating medium and long delay options |
| packages/react/src/Button/IconButton.features.stories.tsx | Added example showing long delay tooltip usage with IconButton |
| .changeset/famous-jobs-applaud.md | Changeset documenting the new minor feature |
Co-authored-by: Copilot <[email protected]>
| * long 1200ms | ||
| */ | ||
| delay?: number | ||
| delay?: 'instant' | 'medium' | 'long' |
There was a problem hiding this comment.
nit: instant delay sounds strange? how about short delay, goes well with short | medium | long
There was a problem hiding this comment.
Is 50ms considered instant? in this case I'm pro 'short'. Otherwise, if it's exactly 0, then maybe 'none' would be most appropriate?
There was a problem hiding this comment.
This is a good point. Since we have 50ms as default, none or instant would be misleading. I'll update this to be short
| </div> | ||
| ) | ||
|
|
||
| export const OcticonPicker = { |
There was a problem hiding this comment.
I don't mind keeping this story in Tooltip.examples.stories, especially with a comment of which instance in dotcom does this represent
There was a problem hiding this comment.
I was tempted to leave this but wasn't sure if that was very helpful. I'll tidy up and push it back 🚀
| * @default 50 | ||
| * @default instant 50ms | ||
| * medium 400ms | ||
| * long 1200ms |
There was a problem hiding this comment.
Can we leave a comment on how we picked these numbers?
They feel arbitrary in code so it would be nice to have the context (or a link to the context) so that, in the future when someone wants to change these, they have more information
There was a problem hiding this comment.
Yes, good idea! I'll add some context.
We need tooltip delay functionality on icon buttons for mouse pointers. Slack reference (Internal access only)
We are adding a flexibility to delay the tooltip.
Changelog
New
I added a
delayprop with an enum:instant(50ms) as default ,medium(400ms),long(1200ms)Rollout strategy
Testing & Reviewing
You can view the stories below:
For medium delay: https://primer-83c142dcae-13348165.drafts.github.io/storybook/?path=/story/components-tooltipv2-features--with-medium-delay
For long delay: https://primer-83c142dcae-13348165.drafts.github.io/storybook/?path=/story/components-tooltipv2-features--with-long-delay
You can also view this story to observe delay functionality for Icon Button: https://primer-83c142dcae-13348165.drafts.github.io/storybook/?path=/story/components-iconbutton-features--long-delayed-tooltip
Merge checklist