Skip to content

TooltipV2: Set the tooltip position when the popover open not when the toggle event is triggered#4327

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

Merged
broccolinisoup merged 15 commits into from
Mar 12, 2024
Merged

TooltipV2: Set the tooltip position when the popover open not when the toggle event is triggered #4327

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
f9516d2
do not set position if it was set before
broccolinisoup Feb 29, 2024
84c4bb3
console
broccolinisoup Feb 29, 2024
db335e9
only position set on tooltip show
broccolinisoup Feb 29, 2024
d9ffbe8
remove event listener with the same func signature
broccolinisoup Feb 29, 2024
1697c8b
just test
broccolinisoup Feb 29, 2024
b2f5576
calculate anchor position after popover-open class is added
broccolinisoup Feb 29, 2024
5ffe0bd
clean up
broccolinisoup Feb 29, 2024
1ca64c7
comment tidy up
broccolinisoup Feb 29, 2024
ba8bbe6
Merge branch 'main' into tooltip-position-wiggle
broccolinisoup Feb 29, 2024
1a4cd23
fix tests
broccolinisoup Mar 4, 2024
f67a6fb
changeset
broccolinisoup Mar 4, 2024
d68c967
Merge branch 'main' into tooltip-position-wiggle
broccolinisoup Mar 6, 2024
47f7569
move setting popover auto back in the render
broccolinisoup Mar 6, 2024
bcad60d
Merge branch 'main' into tooltip-position-wiggle
broccolinisoup Mar 8, 2024
78ff65b
update snapshots
broccolinisoup Mar 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/beige-meals-doubt.md
View file
Open in desktop
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
---
'@primer/react': patch
---

Tooltip v2: Set the tooltip position when popover-open attribute is added to the tooltip element not the toggle event is triggered
42 changes: 17 additions & 25 deletions packages/react/src/TooltipV2/Tooltip.tsx
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -23,6 +23,7 @@ const StyledTooltip = styled.span`
/* Overriding the default popover styles */
display: none;
&[popover] {
position: absolute;

@keithamuskeithamusMar 5, 2024

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.

AIUI this is the main thing that is doing all the heavy lifting. I think you could just apply this line and ignore the rest and it would still work?

@broccolinisoupbroccolinisoupMar 6, 2024

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Yeah definitely this line does all the heavy lifting and actually the rest is not directly related to this change but the existing issue* just became more visible when we added this line (no idea why 🤷🏻‍♀️ ).

*The existing issue (at least it is an issue in my opinion) is where we set the position.

Previously the flow was:

  1. hover/focus event is triggered
  2. tooltip.showPopover() function is called
  3. :popover-open attribute is added to the tooltip div
  4. Adding :popover-open triggers the toggle event as well as triggers the animation that changes the tooltip's opacity from 0 to 1
  5. Position is set in the toggle event's callback function.

I believe the wiggling issue (video in the PR description) happens because we set the position a little too late (step 5) but we make the tooltip visible in the step 3. So in this PR I am setting the position in Step 3 and that makes the toggle event redundant.

Let me know if you have any concern or suggestions.

padding: 0.5em 0.75em;
width: max-content;
margin: auto;
Expand DownExpand Up@@ -196,7 +197,22 @@ export const Tooltip = React.forwardRef(

const openTooltip = () => {
if (tooltipElRef.current && triggerRef.current && !tooltipElRef.current.matches(':popover-open')) {
tooltipElRef.current.showPopover()
const tooltip = tooltipElRef.current
const trigger = triggerRef.current
tooltip.showPopover()
/*
* TOOLTIP POSITIONING
*/
const settings = {
side: directionToPosition[direction].side,
align: directionToPosition[direction].align,
}
const {top, left, anchorAlign, anchorSide} = getAnchoredPosition(tooltip, trigger, settings)

@siddharthkpsiddharthkpMar 6, 2024

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.

This is perfect! We can't calculate boundingRect for an element with display:none, so this is the right timing.

If needed (race condition etc), we can wait for position to be set before animating the tooltip's opacity.

// This is required to make sure the popover is positioned correctly i.e. when there is not enough space on the specified direction, we set a new direction to position the ::after
const calculatedDirection = positionToDirection[`${anchorSide}-${anchorAlign}` as string]
setCalculatedDirection(calculatedDirection)
tooltip.style.top = `${top}px`
tooltip.style.left = `${left}px`
}
}
const closeTooltip = () => {
Expand DownExpand Up@@ -239,32 +255,8 @@ export const Tooltip = React.forwardRef(
}
}

/*
* TOOLTIP POSITIONING
*/
const tooltip = tooltipElRef.current
const trigger = triggerRef.current
tooltip.setAttribute('popover', 'auto')
const settings = {
side: directionToPosition[direction].side,
align: directionToPosition[direction].align,
}

const positionSet = () => {
const {top, left, anchorAlign, anchorSide} = getAnchoredPosition(tooltip, trigger, settings)

tooltip.style.top = `${top}px`
tooltip.style.left = `${left}px`
// This is required to make sure the popover is positioned correctly i.e. when there is not enough space on the specified direction, we set a new direction to position the ::after
const calculatedDirection = positionToDirection[`${anchorSide}-${anchorAlign}` as string]
setCalculatedDirection(calculatedDirection)
}

tooltip.addEventListener('toggle', positionSet)

return () => {
tooltip.removeEventListener('toggle', positionSet)
}
}, [tooltipElRef, triggerRef, direction, type])

return (
Expand Down
7 changes: 5 additions & 2 deletions packages/react/src/__tests__/ActionMenu.test.tsx
View file
Open in desktop
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
import {render as HTMLRender, waitFor} from '@testing-library/react'
import {render as HTMLRender, waitFor, act} from '@testing-library/react'
import userEvent from '@testing-library/user-event'
import {axe} from 'jest-axe'
import React from 'react'
Expand DownExpand Up@@ -360,7 +360,10 @@ describe('ActionMenu', () => {
),
)
const button = component.getByRole('button')
button.focus()
act(() => {
button.focus()
})

expect(component.getByRole('tooltip')).toBeInTheDocument()
})

Expand Down
2 changes: 2 additions & 0 deletions packages/react/src/__tests__/__snapshots__/TextInput.test.tsx.snap
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -1926,6 +1926,7 @@ exports[`TextInput renders trailingAction icon button 1`] = `
}

.c5[popover] {
position: absolute;
padding: 0.5em 0.75em;
width: -webkit-max-content;
width: -moz-max-content;
Expand DownExpand Up@@ -2920,6 +2921,7 @@ exports[`TextInput renders trailingAction text button with a tooltip 1`] = `
}

.c6[popover] {
position: absolute;
padding: 0.5em 0.75em;
width: -webkit-max-content;
width: -moz-max-content;
Expand Down