segmentio / evergreen

🌲 Evergreen React UI Framework by Segment
https://evergreen.segment.com
MIT License
12.39k stars 832 forks source link

Ability to conditionally render Tooltip #1493

Closed brandongregoryscott closed 2 years ago

brandongregoryscott commented 2 years ago

There are cases where we want to display a Tooltip (or rather, have the ability to display a Tooltip on an element) based on a condition. The current isShown prop does not support this very well - it controls whether the Tooltip is always shown, not just when the child element is hovered. It would be nice to add a new prop to control whether the tooltip should be shown once the child element is hovered.

This CodeSandbox demos the current behavior with isShown (which should remain the same), and how you'd currently need to workaround it to fit the use-case.

brandongregoryscott commented 2 years ago

After digging into the implementation more, I think you can implement this by explicitly setting isShown={false} when you don't want to render the tooltip on hover, and not set that prop (i.e. isShown={undefined}) when it should maintain normal behavior (rendering on hover of the target element).

Forked the CodeSandbox from above to verify this: https://codesandbox.io/s/conditional-tooltip-example-forked-fbwjfv?file=/src/App.tsx:2280-2517

This isn't very obvious behavior, but perhaps we can update the JSDoc comment for that prop to call out this behavior instead of adding a new prop and adding complexity to the internal logic:

https://github.com/segmentio/evergreen/blob/41c0d710983f23abf4c6d6a90e18d1bdbb277eb2/src/tooltip/src/Tooltip.js#L116-L127

park-jemin commented 2 years ago

Coming back here to drop this for visibility: found an instance where tooltip looks like it's being used conditionally through isShown, but it's not obvious at all on reading the code (honestly got really confused for a moment), and there's a type error when I tried converting this component to typescript

Screen Shot 2022-07-12 at 7 55 15 AM

Gonna go ahead and stick to using ConditionalTooltip defined in Protocols for now, I would prefer to have that instead of relying on the built-in behavior, would make it more future proof for people working on this later.

For Evergreen's tooltip, possibly introduce a separate prop to such that it renders conditionally? Such as a condition, think it would improve readability and usability

brandongregoryscott commented 2 years ago

Copying this conversation over from Slack for visibility:

FWIW, the current behavior isn’t that out of the ordinary, but it I think it could be better documented. I’m thinking of it as the difference between a controlled/uncontrolled input component. Usually, you’re able to use a TextInput, Checkbox, etc component without explicitly sending in onChange and value props while the component visually shows the state changes. If you want to use those values, though, you usually pass them in, and it becomes controlled at that point. So isShown={true | false} deems it a controlled component whereas isShown={undefined | null} or not passing it at all leaves it uncontrolled, handled internally.

I was curious how other design systems do it, Mantine seems to do it the same way: https://codesandbox.io/s/conditional-tooltip-example-mantine-omd0i9?file=/src/App.tsx. Paste’s API is a little different because they use a hook and let you pass in an entire state object https://paste.twilio.design/components/tooltip/#using-state-hooks.

I'm hesitant to increase the prop surface area without adding any new functionality. I think a first step might be adding additional commentary in the JSDocs for that prop as well as an example on the doc site for how to use it conditionally. If we still feel like a new prop would be better, we’ll probably want to add it to the Popover component too, because it works the same way :stuck_out_tongue: https://codesandbox.io/s/conditional-tooltip-example-forked-w-popover-el4o8c