Skip to content
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

Improve performance of Tooltip and remove TooltipProvider #1974

Merged
merged 4 commits into from
Feb 6, 2024
Merged

Improve performance of Tooltip and remove TooltipProvider #1974

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f0ba1de
feat(tooltip): move the TooltipProvider inside the Tooltip
sungik-choi Feb 5, 2024
34e4780
docs(tooltip): enhance JSDoc
sungik-choi Feb 5, 2024
ca56a95
test(tooltip): add test case about outside click & remove test cases …
sungik-choi Feb 5, 2024
5203189
chore(changeset): add changeset
sungik-choi Feb 5, 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
12 changes: 12 additions & 0 deletions .changeset/curvy-dragons-rest.md
View file
Open in desktop
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`.
5 changes: 1 addition & 4 deletions packages/bezier-react/src/components/AppProvider/AppProvider.tsx
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
Loading