feat: implementing an overflow tooltip for the Text component #1819
Conversation
🦋 Changeset detectedLatest commit: 584bb3f The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
commit: |
|
Size Change: +1.04 kB (+0.2%) Total Size: 534 kB
ℹ️ View Unchanged
|
| /** Enable tooltip on text overflow */ | ||
| showTooltipOnOverflow?: boolean; | ||
| /** Custom tooltip content. If not provided, uses the text children as tooltip content */ | ||
| tooltipContent?: ReactNode; | ||
| /** Tooltip placement */ | ||
| tooltipPlacement?: TooltipProps['placement']; | ||
| /** Additional CSS class name for the tooltip */ | ||
| tooltipClassName?: string; |
There was a problem hiding this comment.
one thing I briefly considered here - this is all the functionality we'd need to show a tooltip for non-overflow cases as well. So technically we could be more flexible and say that if you pass tooltipContent but NOT showtooltiponoverflow, it will show a tooltip ALWAYS.
I haven't done that yet though because some part of me feels like that would be preemptively adding functionality we don't know if we want yet, and it almost feels like 'hiding' a tooltip at that point? plus in our app I can't really imagine us wanting a tooltip on plain text most of the time, we prefer little help icons and whatnot. open to thoughts though
There was a problem hiding this comment.
I don't necessarily know if it makes sense to introduce the tooltip component here.
We want to treat launchpad components as composable atoms, and introducing this logic here would break that.
Is it possible to create a TextTruncatorWithTooltip in launchpad-experimental that includes this logic, and update the props for the Text component to consume the hover/focus props?
There was a problem hiding this comment.
yeah that's what I had on this PR instead haha https://github.com/launchdarkly/gonfalon/pull/55722 mind giving that one a review instead if it's prefereable?
There was a problem hiding this comment.
@Zuzze I'm leaning towards the experimental component for the following reasons:
useTextOverflowis already available in gonfalon- Test should remain an atom, we shouldn't introduce external component dependencies
- The approach in Anthony's original PR can easily extend to other components like Label and Heading (as you called out!)
What do you think?
There was a problem hiding this comment.
@nhironaka @ld-ahartmann I'm fine with both, for better discoverability I was wondering might be convenient to have it in LP but also fine if you prefer to keep it separate as long as there is documentation how to use them together.
General side note to keep molecule component API tidy for future: It's usually a good practice to avoid picking the sub component props (tooltipContent, tooltipPlacement,...) separately to main API and instead have a prop object like tooltip?: TooltipProps which would then understand all tooltip props directly ie.
<Text tooltip={{label: "My tooltip", placement: "bottom"}>Text</Text>
This way:
- All props are inherited automatically
- If subcomponent prop API changes, no extra changes needed for molecules using it
- No additional booleans like
showTooltipOnOverflowneeded because the subcomponent is either defined or not
In @gonfalon/ui there is currently naming convention where extended components have syntax like <main-component>With<specifier> like MultiSelectWithSearch, SelectWithSearch but open to align on this one as well. Another common convention we could use is <specifier><main-component>
| const { isFocusVisible } = useFocusVisible(); | ||
|
|
||
| // Merge refs | ||
| const mergedRef = useCallback( |
There was a problem hiding this comment.
for the future: '@react-aria/utils' exports mergeRefs
| /** Enable tooltip on text overflow */ | ||
| showTooltipOnOverflow?: boolean; | ||
| /** Custom tooltip content. If not provided, uses the text children as tooltip content */ | ||
| tooltipContent?: ReactNode; | ||
| /** Tooltip placement */ | ||
| tooltipPlacement?: TooltipProps['placement']; | ||
| /** Additional CSS class name for the tooltip */ | ||
| tooltipClassName?: string; |
There was a problem hiding this comment.
@Zuzze I'm leaning towards the experimental component for the following reasons:
useTextOverflowis already available in gonfalon- Test should remain an atom, we shouldn't introduce external component dependencies
- The approach in Anthony's original PR can easily extend to other components like Label and Heading (as you called out!)
What do you think?
Summary
We were facing some overflow issues that were happening with the details section of the Metric Selection Menu tooltip by creating a new component in launchpad experimental - and while writing that PR I found some this new simpler approach.
Per feedback on this PR (https://github.com/launchdarkly/gonfalon/pull/55722) I've moved it to this launchpad component for simplicity and greater ease of use.
Screenshots (if appropriate):
(more details and screenshots in the storybook)
Testing approaches
New test file with relevant tests, added story files.