Skip to content

Commit

Permalink
Improve performance of Tooltip and remove TooltipProvider (#1974)
Browse files Browse the repository at this point in the history
<!--
  How to write a good PR title:
- Follow [the Conventional Commits
specification](https://www.conventionalcommits.org/en/v1.0.0/).
  - Give as much context as necessary and as little as possible
  - Prefix it with [WIP] while it’s a work in progress
-->

## Self Checklist

- [x] I wrote a PR title in **English** and added an appropriate
**label** to the PR.
- [x] I wrote the commit message in **English** and to follow [**the
Conventional Commits
specification**](https://www.conventionalcommits.org/en/v1.0.0/).
- [x] I [added the
**changeset**](https://github.com/changesets/changesets/blob/main/docs/adding-a-changeset.md)
about the changes that needed to be released. (or didn't have to)
- [x] I wrote or updated **documentation** related to the changes. (or
didn't have to)
- [x] I wrote or updated **tests** related to the changes. (or didn't
have to)
- [x] I tested the changes in various browsers. (or didn't have to)
  - Windows: Chrome, Edge, (Optional) Firefox
  - macOS: Chrome, Edge, Safari, (Optional) Firefox

## Related Issue
<!-- Please link to issue if one exists -->

- Fixes #1732

## Summary
<!-- Please brief explanation of the changes made -->

Improve performance of `Tooltip` and remove `TooltipProvider`

## Details
<!-- Please elaborate description of the changes -->

- #1732 를 참고해주세요.
- 초기에 floating-ui를 사용하여 구현했으나, radix-ui floating element간의 사용성(예 - esc키를
눌렀을 경우 가장 상위에 floating element만 사라짐) & 적은 Breaking change를 고려하여
TooltipProvider를 내부에 위치시키는 방식으로 변경했습니다. 생각보다 floating-ui로 구현했을 때와 성능상
차이는 거의 없었습니다.
- JSDoc을 업데이트했습니다. 기존 문의가 많았던 radix 관련 ref forwarding, spread props에 대한
내용을 추가했습니다.
- 툴팁 바깥을 눌렀을 때 툴팁이 dismiss되는 케이스에 대한 단위 테스트를 추가했습니다.

### Measure Performance

**AS-IS:** production 기준
**TO-BE:** 채널 데스크 앱에 bezier-react@2.0.0-alpha.7 + 해당 PR 변경 사항 적용. local
+ production target 기준

Tooltip이 굉장히 많이 존재하는 고객 태그 관리 페이지에서, 마우스가 호버되고나서 툴팁이 렌더되기 전까지 퍼포먼스를
측정했습니다.
**Scripting 소요 시간(노란색)이 크게 감소한 걸 확인할 수 있습니다.**

| AS-IS | TO-BE |
|:---:|:---:|
| <img width="976" alt="스크린샷 2024-02-04 오후 4 48 35"
src="https://github.com/channel-io/bezier-react/assets/58209009/35a897c0-7584-454a-828c-d5b567a56bbc">
| <img width="1072" alt="스크린샷 2024-02-05 오후 3 05 34"
src="https://github.com/channel-io/bezier-react/assets/58209009/c1563692-9fa6-46ac-b887-5baafe5cd981">
|
| 154373 ms | 13017 ms (-91.57%) |

### Breaking change? (Yes/No)
<!-- If Yes, please describe the impact and migration path for users -->

Yes. TooltipProvider를 더 이상 제공하지 않으며, delayShow 속성의 기본값이 300 -> 0으로
변경됩니다. TooltipProvider의 딜레이 시간 공유 기능이 제거되면서, UX 유지를 위한 변경입니다.

## References
<!-- Please list any other resources or points the reviewer should be
aware of -->

-
https://github.com/radix-ui/primitives/blob/c31c97274ff357aea99afe6c01c1c8c58b6356e0/packages/react/tooltip/src/Tooltip.tsx#L75
  • Loading branch information
sungik-choi authored Feb 6, 2024
1 parent 6bfaa1d commit a086f0f
Show file tree
Hide file tree
Showing 6 changed files with 101 additions and 230 deletions.
12 changes: 12 additions & 0 deletions .changeset/curvy-dragons-rest.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
---
"@channel.io/bezier-react": major
---

**Breaking Changes: Remove `TooltipProvider` and Property updates in `Tooltip` component**

- No longer support `TooltipProvider` and `TooltipProviderProps`. `Tooltip` component was implemented via radix-ui's Tooltip, which required the use of a `TooltipProvider`, which caused all subcomponents to be re-rendered and caused a performance hit. We decided that the ability to share hover delay time between `Tooltip` components via `TooltipProvider` was not a feature we needed, even with the performance penalty. Also, by providing `TooltipProvider` built-in to `AppProvider`, we were unnecessarily importing modules from our library usage that didn't require `Tooltip`.
- `Tooltip` component now contains a `TooltipProvider` inside it.

**Minor Changes:**

- Change the default value of `delayShow` prop from `300` to `0`.
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ import { window as defaultWindow } from '~/src/utils/dom'

import { FeatureProvider } from '~/src/components/FeatureProvider'
import { TokenProvider } from '~/src/components/TokenProvider'
import { TooltipProvider } from '~/src/components/Tooltip'
import { WindowProvider } from '~/src/components/WindowProvider'

import { type AppProviderProps } from './AppProvider.types'
Expand Down Expand Up @@ -50,9 +49,7 @@ export function AppProvider({
<WindowProvider window={window}>
<FeatureProvider features={features}>
<TokenProvider themeName={themeName}>
<TooltipProvider>
{ children }
</TooltipProvider>
{ children }
</TokenProvider>
</FeatureProvider>
</WindowProvider>
Expand Down
68 changes: 8 additions & 60 deletions packages/bezier-react/src/components/Tooltip/Tooltip.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -8,14 +8,8 @@ import userEvent from '@testing-library/user-event'

import { render } from '~/src/utils/test'

import {
Tooltip,
TooltipProvider,
} from './Tooltip'
import {
type TooltipProps,
type TooltipProviderProps,
} from './Tooltip.types'
import { Tooltip } from './Tooltip'
import { type TooltipProps } from './Tooltip.types'

describe('Tooltip', () => {
const renderTooltip = ({
Expand Down Expand Up @@ -74,6 +68,12 @@ describe('Tooltip', () => {
expect(queryByRole('tooltip')).not.toBeInTheDocument()
})

it('When the tooltip content is visible, clicking outside the tooltip should hide the tooltip content.', async () => {
const { queryByRole } = renderTooltip({ defaultShow: true, content: 'tooltip content' })
await user.click(document.body)
expect(queryByRole('tooltip')).not.toBeInTheDocument()
})

it('When the `allowHover` property is true, the tooltip content should be hoverable.', async () => {
const { getByRole } = renderTooltip({ allowHover: true, content: 'tooltip content' })
await user.hover(getByRole('button'))
Expand Down Expand Up @@ -150,55 +150,3 @@ describe('Tooltip', () => {
})
})
})

describe('TooltipProvider', () => {
const renderTooltipProvider = ({
children,
...rest
}: TooltipProviderProps = {}) => render(
<TooltipProvider
delayShow={0}
{...rest}
>
<Tooltip content="tooltip content">
<button type="button">
Trigger
</button>
</Tooltip>
</TooltipProvider>,
)

let user: ReturnType<typeof userEvent.setup>

beforeEach(() => {
user = userEvent.setup()
})

describe('User Interactions', () => {
it('If the `allowHover` property is true, the tooltip content should be hoverable.', async () => {
const { getByRole } = renderTooltipProvider({ allowHover: true })
await user.hover(getByRole('button'))
await user.hover(getByRole('tooltip'))
expect(getByRole('tooltip')).toBeInTheDocument()
})

it('If the `delayShow` property is greater than 0, the tooltip should be delayed by that number of ms before appearing.', async () => {
const { getByRole, queryByRole } = renderTooltipProvider({ delayShow: 1000 })
await user.hover(getByRole('button'))
expect(queryByRole('tooltip')).not.toBeInTheDocument()
await waitFor(() => {
expect(queryByRole('tooltip')).toBeInTheDocument()
}, { timeout: 1000 })
})

it('If the `delayHide` property is greater than 0, the tooltip should be delayed by that number of ms before disappearing.', async () => {
const { getByRole, queryByRole } = renderTooltipProvider({ delayHide: 1000 })
await user.hover(getByRole('button'))
await user.unhover(getByRole('button'))
expect(queryByRole('tooltip')).toBeInTheDocument()
await waitFor(() => {
expect(queryByRole('tooltip')).not.toBeInTheDocument()
}, { timeout: 1000 })
})
})
})
204 changes: 76 additions & 128 deletions packages/bezier-react/src/components/Tooltip/Tooltip.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,7 @@ import React, {

import * as TooltipPrimitive from '@radix-ui/react-tooltip'

import { createContext } from '~/src/utils/react'
import {
isBoolean,
isEmpty,
} from '~/src/utils/type'
import { isEmpty } from '~/src/utils/type'

import {
Icon,
Expand All @@ -27,7 +23,6 @@ import { useWindow } from '~/src/components/WindowProvider'
import {
TooltipPosition,
type TooltipProps,
type TooltipProviderProps,
} from './Tooltip.types'

import styles from './Tooltip.module.scss'
Expand Down Expand Up @@ -105,61 +100,16 @@ function getSideAndAlign(
}
}

const [
/**
* NOTE: Custom context use because the radix-ui doesn't support `delayHide`.
*/
TooltipGlobalContextProvider,
useTooltipGlobalContext,
] = createContext<Required<Pick<TooltipProviderProps, 'delayHide'>> | null>(null, 'TooltipProvider')

/**
* `TooltipProvider` is used to globally provide props to child tooltips.
* @example
*
* ```tsx
* <TooltipProvider allowHover delayShow={1000}>
* <Tooltip />
* </TooltipProvider>
* ```
*/
export function TooltipProvider({
children,
allowHover = false,
delayShow = 300,
delayHide = 0,
skipDelayShow = 500,
}: TooltipProviderProps) {
const contextValue = useMemo(() => ({
delayHide,
}), [delayHide])

return (
<TooltipPrimitive.Provider
delayDuration={delayShow}
skipDelayDuration={skipDelayShow}
disableHoverableContent={!allowHover}
>
<TooltipGlobalContextProvider value={contextValue}>
{ children }
</TooltipGlobalContextProvider>
</TooltipPrimitive.Provider>
)
}

/**
* `Tooltip` is a component that shows additional information when the mouse hovers or the keyboard is focused.
* If you want to compose another component with `Tooltip`, **it must spread props and forward ref.**
* @example
*
* ```tsx
* // Anatomy of the Tooltip
* <TooltipProvider>
* <Tooltip />
* </TooltipProvider>
*
* // Example of a Tooltip with a button
* // Your component must spread props and forward ref.
* const Button = React.forwardRef((props, forwardedRef) => (<button {...props} ref={forwardedRef} />))
* // Then, you can use `Tooltip` with your component.
* <Tooltip content="Ta-da!">
* <button>Hover me</button>
* <Button>Hover me</Button>
* </Tooltip>
* ```
*/
Expand All @@ -175,22 +125,18 @@ export const Tooltip = forwardRef<HTMLDivElement, TooltipProps>(function Tooltip
icon,
placement = TooltipPosition.BottomCenter,
offset = 4,
container: givenContainer,
container: containerProp,
keepInContainer = true,
allowHover,
delayShow,
delayHide: delayHideProp,
allowHover = false,
delayShow = 0,
delayHide = 0,
...rest
}, forwardedRef) {
const { rootElement } = useWindow()
const [show, setShow] = useState<boolean>(defaultShow ?? false)
const timeoutRef = useRef<NodeJS.Timeout>()

const { delayHide: globalDelayHide } = useTooltipGlobalContext('Tooltip')
const delayHide = delayHideProp ?? globalDelayHide

const defaultContainer = rootElement
const container = givenContainer ?? defaultContainer
const { rootElement } = useWindow()
const container = containerProp ?? rootElement

const shouldBeHidden = useMemo(() => (
disabled || isEmpty(content)
Expand Down Expand Up @@ -257,76 +203,78 @@ export const Tooltip = forwardRef<HTMLDivElement, TooltipProps>(function Tooltip
])

return (
<TooltipPrimitive.Root
open={show}
defaultOpen={defaultShow}
delayDuration={delayShow}
disableHoverableContent={isBoolean(allowHover) ? !allowHover : undefined}
onOpenChange={onOpenChange}
>
<TooltipPrimitive.Trigger asChild>
{ children }
</TooltipPrimitive.Trigger>
<TooltipPrimitive.Provider skipDelayDuration={0}>
<TooltipPrimitive.Root
open={show}
defaultOpen={defaultShow}
delayDuration={delayShow}
disableHoverableContent={!allowHover}
onOpenChange={onOpenChange}
>
<TooltipPrimitive.Trigger asChild>
{ children }
</TooltipPrimitive.Trigger>

<TooltipPrimitive.Portal container={container}>
<InvertedThemeProvider>
<TooltipPrimitive.Content
{...rest}
{...getSideAndAlign(placement)}
asChild
ref={forwardedRef}
sideOffset={offset}
avoidCollisions={keepInContainer}
collisionPadding={8}
hideWhenDetached
>
<HStack
spacing={4}
className={styles.Tooltip}
<TooltipPrimitive.Portal container={container}>
<InvertedThemeProvider>
<TooltipPrimitive.Content
{...rest}
{...getSideAndAlign(placement)}
asChild
ref={forwardedRef}
sideOffset={offset}
avoidCollisions={keepInContainer}
collisionPadding={8}
hideWhenDetached
>
<div className={styles.TooltipContainer}>
{ title && (
<HStack
spacing={4}
className={styles.Tooltip}
>
<div className={styles.TooltipContainer}>
{ title && (
<Text
typo="13"
bold
marginBottom={2}
color="txt-black-darkest"
>
{ title }
</Text>
) }

<Text
typo="13"
bold
marginBottom={2}
color="txt-black-darkest"
className={styles.TooltipContent}
truncated={20}
typo="13"
>
{ title }
{ content }
</Text>
) }

<Text
color="txt-black-darkest"
className={styles.TooltipContent}
truncated={20}
typo="13"
>
{ content }
</Text>
{ description && (
<Text
typo="12"
color="txt-black-dark"
>
{ description }
</Text>
) }
</div>

{ description && (
<Text
typo="12"
color="txt-black-dark"
>
{ description }
</Text>
{ icon && (
<Icon
size={IconSize.XS}
color="txt-black-darkest"
source={icon}
className={styles.Icon}
/>
) }
</div>

{ icon && (
<Icon
size={IconSize.XS}
color="txt-black-darkest"
source={icon}
className={styles.Icon}
/>
) }
</HStack>
</TooltipPrimitive.Content>
</InvertedThemeProvider>
</TooltipPrimitive.Portal>
</TooltipPrimitive.Root>
</HStack>
</TooltipPrimitive.Content>
</InvertedThemeProvider>
</TooltipPrimitive.Portal>
</TooltipPrimitive.Root>
</TooltipPrimitive.Provider>
)
})
Loading

0 comments on commit a086f0f

Please sign in to comment.