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.

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

feat: #235 v5 Tooltip component #239

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
// Vitest Snapshot v1, https://vitest.dev/guide/snapshot.html

exports[`ToolTip > should match snapshot 1`] = `
<DocumentFragment>
<div
aria-describedby="test-static-id"
class="mocked-styled-0 el-tool-tip-container"
>
Test
</div>
</DocumentFragment>
`;
9 changes: 9 additions & 0 deletions src/components/tooltip/__tests__/tooltip.test.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
import { render } from '@testing-library/react'
import { ToolTip } from '../index'

describe('ToolTip', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit-pick: Tooltip is one word, not two, so I don't think Tip should be capitalised

test('should match snapshot', () => {
const { asFragment } = render(<ToolTip tip="Tooltip Text">Test</ToolTip>)
expect(asFragment()).toMatchSnapshot()
})
})
2 changes: 2 additions & 0 deletions src/components/tooltip/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
export * from './styles'
export * from './tooltip'
132 changes: 132 additions & 0 deletions src/components/tooltip/styles.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,132 @@
import { styled } from '@linaria/react'

export const ElToolTipContainer = styled.div`
Copy link
Contributor

Choose a reason for hiding this comment

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

nit-pick: Again, I don't think Tip should be capitalised; i.e. ElTooltipContainer. Looks like there's a lot of this throughout the PR.

position: relative;
display: inline-block;
overflow: visible;
`

export const ElToolTipChild = styled.div`
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: I think this should be ElTooltip since this represents the actual presentational "tooltip" atom.

width: max-content;
background: var(--fill-default-darkest, #222b33);
color: var(--text-white);
padding: var(--spacing-2) var(--spacing-3) var(--spacing-2) var(--spacing-3);
border-radius: var(--corner-default);
font-family: var(--font-family);
font-size: var(--font-size-xs);
font-weight: 400; // To do : Need to replace with variable
line-height: var(--line-height-xs);
letter-spacing: var(--letter-spacing-xs);
text-align: left;
text-underline-position: from-font;
text-decoration-skip-ink: none;
Copy link
Contributor

Choose a reason for hiding this comment

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

question: where are these underline and decoration properties coming from? Figma?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this is coming from Figma. I'll double-check with Andrei to find a solution for non-required CSS to be in a separate section if not explicitly required for elements.

position: absolute;
transition: opacity 0.2s;
pointer-events: none;
white-space: normal;
word-wrap: break-word;
z-index: 99999; // Adding additional CSS to position tooltip on top of everything

// To do (In Future): Tooltip Positioning to be replaced with css anchor positioning feature
// Currently not supported by wider browser group
// See: https://developer.mozilla.org/en-US/docs/Web/CSS/position-anchor#browser_compatibility

/* Top Position */
&[data-position='top'] {
bottom: 100%;
left: 50%;
transform: translateX(-50%);
margin-bottom: var(--spacing-1);
}

/* Top Start Position */
&[data-position='topStart'] {
bottom: 100%;
left: 0;
margin-bottom: var(--spacing-1);
}

/* Top End Position */
&[data-position='topEnd'] {
bottom: 100%;
right: 0;
margin-bottom: var(--spacing-1);
}

/* Bottom Position */
&[data-position='bottom'] {
top: 100%;
left: 50%;
transform: translateX(-50%);
margin-top: var(--spacing-1);
}

/* Bottom Start Position */
&[data-position='bottomStart'] {
top: 100%;
left: 0;
margin-top: var(--spacing-1);
}

/* Bottom End Position */
&[data-position='bottomEnd'] {
top: 100%;
right: 0;
margin-top: var(--spacing-1);
}

/* Right Position */
&[data-position='right'] {
left: 100%;
top: 50%;
transform: translateY(-50%);
margin-left: var(--spacing-1);
}

/* Right Start Position */
&[data-position='rightStart'] {
left: 100%;
top: 0;
margin-left: var(--spacing-1);
}

/* Right End Position */
&[data-position='rightEnd'] {
left: 100%;
bottom: 0;
margin-left: var(--spacing-1);
}

/* Left Position */
&[data-position='left'] {
right: 100%;
top: 50%;
transform: translateY(-50%);
margin-right: var(--spacing-1);
}

/* Left Start Position */
&[data-position='leftStart'] {
right: 100%;
top: 0;
margin-right: var(--spacing-1);
}

/* Left End Position */
&[data-position='leftEnd'] {
right: 100%;
bottom: 0;
margin-right: var(--spacing-1);
}
`

export const ElToolTipLabel = styled.span`
font-family: var(--font-family);
font-size: var(--font-size-xs);
font-weight: 600; // To do : Need to replace with variable
line-height: var(--line-height-xs);
letter-spacing: var(--letter-spacing-xs);
text-align: left;
text-underline-position: from-font;
text-decoration-skip-ink: none;
`
11 changes: 11 additions & 0 deletions src/components/tooltip/tooltip.mdx
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
import { Meta, Canvas, Controls, Description } from '@storybook/blocks'
import { RenderHtmlMarkup } from '../../storybook/render-html-markup'
import * as ToolTipStories from './tooltip.stories'

<Meta of={ToolTipStories} />

# Tool Tip

<Controls />

<Canvas of={ToolTipStories.BasicUsage} />
47 changes: 47 additions & 0 deletions src/components/tooltip/tooltip.stories.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
import { Meta } from '@storybook/react'
import { ToolTip } from './tooltip'
import { Button } from '../button'

const meta: Meta<typeof ToolTip> = {
title: 'Components/ToolTip',
component: ToolTip,
argTypes: {
tip: {
control: 'text',
description: 'Defines the tip of the tooltip.',
},
label: {
control: 'text',
description: 'Defines the label of the tooltip',
},
children: {
table: {
disable: true,
},
},
},
// This is to style the story, as the story parent container has overflow hidden.
decorators: [
(Story) => (
<div
style={{
display: 'flex',
justifyContent: 'center',
alignItems: 'center',
height: '20vh',
}}
>
<Story />
</div>
),
],
}

export default meta

Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: I think it would be valuable to show an additional story or two for the ElTooltip atom directly. That way it's easy to see the visual appearance of the atom without first needing to hover or focus some other element.

export const BasicUsage = {
args: {
tip: 'Tooltip text',
children: <Button variant="primary">Hover me</Button>,
},
}
66 changes: 66 additions & 0 deletions src/components/tooltip/tooltip.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
import React, { FC, HTMLAttributes, useState, ReactNode } from 'react'
import { ElToolTipChild, ElToolTipContainer, ElToolTipLabel } from './styles'
import { useId } from '#src/storybook/random-id'

export interface ToolTipProps extends HTMLAttributes<HTMLDivElement> {
children: ReactNode
label?: string
tip: string
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: Figma uses description as the prop name. I think that's probably more appropriate as I'd think "tip" includes the label and the description.

maxWidth?: string
position?:
| 'top'
| 'bottom'
| 'right'
| 'left'
| 'topStart'
| 'topEnd'
| 'bottomStart'
| 'bottomEnd'
| 'rightStart'
| 'rightEnd'
| 'leftStart'
| 'leftEnd'
}

export interface ToolTipChildProps extends HTMLAttributes<HTMLDivElement> {
position: ToolTipProps['position']
maxWidth?: string
}

export const ToolTip: FC<ToolTipProps> = ({ children, label, tip, maxWidth = '400px', position = 'top' }) => {
const [visible, setVisible] = useState(false)
const tooltipId = useId()

return (
<ElToolTipContainer
onMouseEnter={() => setVisible(true)}
onMouseLeave={() => setVisible(false)}
onFocus={() => setVisible(true)}
onBlur={() => setVisible(false)}
aria-describedby={tooltipId}
Copy link
Contributor

Choose a reason for hiding this comment

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

issue: I'm not sure placing aria-describedby on the parent is correct. In the case where the children is a <button>, I believe the aria-describedby should be on the <button> element itself, not it's parent 🤔

I suspect we need something a little more akin to Menu.Trigger

>
{children}
{visible && (
Copy link
Contributor

@kurtdoherty kurtdoherty Dec 10, 2024

Choose a reason for hiding this comment

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

issue (non-blocking): We should really be animating the tooltip in and out, so I think the visibility is best handled in CSS via a data-visible attribute.

In future, when we upgrade to React 19 (hopefully we can do this soon), we'll get access to the new Popover API, which will allow us to push the visibility into the DOM (e.g. https://codesandbox.io/p/sandbox/jp8njk)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kurtdoherty The rationale behind this approach is to address scenarios where the table contains a large number of items (e.g., 100 rows), each requiring a tooltip. Using conditional rendering ensures better performance and saves memory as we wont' load unnecessary tooltip.

I am happy to discuss and get it updated to be handled by CSS

Copy link
Contributor

@kurtdoherty kurtdoherty Dec 11, 2024

Choose a reason for hiding this comment

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

Using conditional rendering ensures better performance and saves memory as we wont' load unnecessary tooltip.

Have you profiled this? I only ask because I wouldn't have expected there to be much difference at the moment 🤔 If the tooltip was measuring its trigger's bounding box or something like that, then I could see there being a noticeable performance impact, but it seems like a pretty cheap element for browsers to render 🤷‍♂️

If performance does become an issue in future, there's CSS properties (e.g. content-visibility) that could potentially help us either way.

I don't mind having this go through as is. It may actually be possible we can just as easily animate the entry/exit of these DOM elements via @starting-style regardless of whether we're handling visibility in React component state or CSS (I'd forgotten about this new @-rule, though it's not supported by Firefox, so maybe animation needs to wait either way 🤷)

note: I've update my original comment to make this a non-blocking issue 👍

<ToolTipChild id={tooltipId} position={position} maxWidth={maxWidth}>
{label && <ElToolTipLabel>{label}: </ElToolTipLabel>}
{tip}
</ToolTipChild>
)}
</ElToolTipContainer>
)
}

export const ToolTipChild: FC<ToolTipChildProps> = ({ children, position, maxWidth, id }) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

question: is the main purpose of this component to handle maxWidth being passed in via style? The other props seem like they could just be explicitly a part of ElToolTipChild's prop interface and thus avoid the need for this extra layer?

return (
<ElToolTipChild
id={id} // To support a11y for screen reader to pass to aria-describedby
role="tooltip"
data-position={position}
style={{ maxWidth: maxWidth }}
aria-hidden={!id} // hidden if no id is present
aria-live="assertive" // Announce content dynamically
>
{children}
</ElToolTipChild>
)
}
1 change: 1 addition & 0 deletions src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ export * from './components/menu'
export * from './components/dialog'
export * from './components/avatar'
export * from './components/skeleton'
export * from './components/tooltip'

export * from './hooks/use-portal'
export * from './hooks/use-snack'
Expand Down
Loading