feat: #235 v5 Tooltip component #239
Conversation
Coverage summary from CodacySee diff coverage on Codacy
Coverage variation details
Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: Diff coverage details
Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: See your quality gate settings Change summary preferencesCodacy stopped sending the deprecated coverage status on June 5th, 2024. Learn more Footnotes
|
ksolanki7
changed the title
There was a problem hiding this comment.
Thanks @ksolanki7 👏 This is nicely implemented. Only a few comments, the main one being whether the current API allows us to correctly wire up the a11y props.
| import { render } from '@testing-library/react' | ||
| import { ToolTip } from '../index' | ||
|
|
||
| describe('ToolTip', () => { |
There was a problem hiding this comment.
nit-pick: Tooltip is one word, not two, so I don't think Tip should be capitalised
| @@ -0,0 +1,132 @@ | |||
| import { styled } from '@linaria/react' | |||
|
|
|||
| export const ElToolTipContainer = styled.div` | |||
There was a problem hiding this comment.
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.
| overflow: visible; | ||
| ` | ||
|
|
||
| export const ElToolTipChild = styled.div` |
There was a problem hiding this comment.
suggestion: I think this should be ElTooltip since this represents the actual presentational "tooltip" atom.
| letter-spacing: var(--letter-spacing-xs); | ||
| text-align: left; | ||
| text-underline-position: from-font; | ||
| text-decoration-skip-ink: none; |
There was a problem hiding this comment.
question: where are these underline and decoration properties coming from? Figma?
There was a problem hiding this comment.
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.
| } | ||
|
|
||
| export default meta | ||
|
|
There was a problem hiding this comment.
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 interface ToolTipProps extends HTMLAttributes<HTMLDivElement> { | ||
| children: ReactNode | ||
| label?: string | ||
| tip: string |
There was a problem hiding this comment.
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.
| onMouseLeave={() => setVisible(false)} | ||
| onFocus={() => setVisible(true)} | ||
| onBlur={() => setVisible(false)} | ||
| aria-describedby={tooltipId} |
There was a problem hiding this comment.
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
| ) | ||
| } | ||
|
|
||
| export const ToolTipChild: FC<ToolTipChildProps> = ({ children, position, maxWidth, id }) => { |
There was a problem hiding this comment.
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?
| aria-describedby={tooltipId} | ||
| > | ||
| {children} | ||
| {visible && ( |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
@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
There was a problem hiding this comment.
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 👍
Description
Demo for Tooltip Story
Screen.Recording.2024-12-06.at.6.00.18.pm.mov