Component: Tooltip by mdespuits · Pull Request #155 · cision/rover-ui

mdespuits · 2020-04-02T20:21:37Z

Finally, a way to add a basic tooltip for text as well as the configurability to display any content we want.

mdespuits · 2020-04-02T20:27:41Z

+    if (direction === 'left' || direction === 'right') {
+      return {
+        top: (childrenHeight - tooltipHeight) / 2,
+      };
+    }


This is the magic sauce that makes sure that tooltips positioned on the left and right are centered to the original content

pixelbandito

Sorry, it's a lot of feedback for a small component! Thanks for doing this.

I got to the point where I was speculating too much, I'll have another look after we resolve some of these questions and comments.

pixelbandito · 2020-04-03T18:10:15Z

+  children: PropTypes.node.isRequired,
+  closeable: PropTypes.bool,
+  closeIcon: PropTypes.string,
+  content: PropTypes.oneOfType([


Just PropTypes.node handles strings, elements, arrays, arrays of nodes, etc.

Nice. I did not know this. Thanks!

pixelbandito · 2020-04-03T18:11:05Z

+  hover: PropTypes.bool,
+  onClose: PropTypes.func,
+  open: PropTypes.bool,
+  text: PropTypes.string,


Do you need both content and text? The only difference I see is a conditional class, and you could just check typeof content === 'string' to do that.

Yeah, I did do that. I switched over to this one, but I wasn't convinced either way. I didn't like doing the switch in either case. I'm happy to go back to a typeof check

pixelbandito · 2020-04-03T18:12:06Z

+  onClose: PropTypes.func,
+  open: PropTypes.bool,
+  text: PropTypes.string,
+  tooltipOpts: PropTypes.shape({


Would this be clearer as just tooltipProps? afaik that's the common pattern, across libraries beyond just ours.

Also, it should maybe just be PropTypes.object, since there's really no limit to what the consumer might pass down, nor any hard dependencies on className or style

pixelbandito · 2020-04-03T18:15:17Z

+  direction: PropTypes.oneOf(directions),
+  hover: PropTypes.bool,
+  onClose: PropTypes.func,
+  open: PropTypes.bool,


Maybe defaultIsOpen instead?
Adding default makes it clearer that the component will control the isOpen once it mounts.
Adding is just helps disambiguate the verb / adjective.

pixelbandito · 2020-04-03T18:37:08Z

+Tooltip.propTypes = {
+  children: PropTypes.node.isRequired,
+  closeable: PropTypes.bool,
+  closeIcon: PropTypes.string,


Would it make sense to force the default closeIcon and drop this prop? Seems like a fine place for consistency.

pixelbandito · 2020-04-03T18:37:33Z

+
+  const TooltipContent = (text || tooltipContent) && (
+    <div ref={tooltipRef} {...tooltipProps}>
+      <div style={tooltipStyle} className={styles.tooltipInnerWrapper}>


You can probably collapse these 2 divs, right?

Actually, no. To accomplish the tooltip arrow's "shadow", the z-index needed to be defined on an element nested within the outer ref. I'd have to walk through it again in a video to remind myself of why that was, but it wasn't possible without an extra nesting layer ☹️

pixelbandito · 2020-04-03T18:38:09Z

+            <Icon name={closeIcon} />
+          </button>
+        )}
+        <div


If the user provides a React node, you don't need the .textContent div at all, yeah?

pixelbandito · 2020-04-06T19:55:45Z

+  );
+};
+
+EasyRichTooltip.propTypes = {


Are you planning for a non-rich version? If not, would EasyTooltip work?

EasyTooltip would be just for text, and just with text can be done just using Tooltip by itself. All of the other props are mostly to make the rich content and interaction possible

pixelbandito · 2020-04-06T20:15:27Z

+
+Tooltip.propTypes = {
+  children: PropTypes.node.isRequired,
+  closeable: PropTypes.func,


How about the naming convention onClose?

I just commented about that, I should have refreshed the PR but I was in the middle of the review 😱

pixelbandito · 2020-04-06T20:19:28Z

🌼🥪

sebastianvera

Pretty cool work, it looks solid!

sebastianvera · 2020-04-06T20:07:34Z

+    }
+  };
+
+  const handleEscape = e => {


Since this is only being used within the following useEffect why not moving it to the if block inside?

useEffect(() => { if (closeOnEscape) { const handleEscape = e => { if (closeable && e.key === 'Escape') { closeable(); } }; document.addEventListener('keydown', handleEscape); return () => { document.removeEventListener('keydown', handleEscape); }; } return () => {}; }, []);

Good point. 👍

sebastianvera · 2020-04-06T20:13:27Z

+    return () => {};
+  }, []);
+
+  useEffect(() => {


should we use useLayoutEffect instead, so we set the height before the browser's paint step? I've found that by using useLayoutEffect, for these cases where we need the content's width/height, we avoid flickering on the screen (we might need it if the tooltip is opened when rendering)

Oh, that's actually something I hadn't considered. useLayoutEffect is still unknown to me, so time to dive in!

sebastianvera · 2020-04-06T20:45:39Z

+  border: none;
+  cursor: pointer;
+  color: var(--rvr-gray-lite-2);
+  margin: 0;


should this be removed?

Maybe? I'd have to look at my CSS again (since I haven't looked at it in a week)

Ah, I didn't see the line right after the comment. Thanks!

sebastianvera · 2020-04-06T20:46:26Z

+  position: absolute;
+  right: 5px;
+  top: 5px;
+  z-index: 51;


I'm wondering why this is 51, does it have any special meaning?

This was just to make sure that the tooltip content, the white arrow, and the arrow shadow had correct z-index relative to each other.

This is a bit of a magical number. I should put it with the other tooltip CSS custom properties

sebastianvera · 2020-04-06T20:49:31Z

 export { default as ExpansionPanel } from './components/ExpansionPanel';
 export { default as Accordion } from './components/Accordion';
 export { default as Avatar } from './components/Avatar';
+export { default as Tooltip, UncontrolledTooltip } from './components/Tooltip';


Wrong component name here, I think I prefer UncontrolledTooltip over EasyRichTooltip, but I'm ok with either 🚀

We have the Easy* prefixed used in other components. It's simply a pattern we've adopted for Rover. Not attached to it, but worth considering

And yes, I'll fix the export name :)

sebastianvera · 2020-04-06T20:55:52Z

+  const [hovered, setHovered] = useState(false);
+  const [tooltipHeight, setTooltipHeight] = useState(0);
+
+  const closeFunc = useCallback(onClose || function() {}, []);


I think we don't need useCallback here, this is:

not being used as a dependency for a hook

not being pass as a prop down the tree, it's just being used in the button on line 119, so having a different reference no every render will not compromise performance or generate re-renders

Let me know if maybe I missed something 🙂

I generally try to avoid useCallback myself. I think I added this thinking "I don't want to recreate an empty function endlessly". But I could just use an empty function const outside of the component or just use lodash/noop. Either way, I can drop the overhead of useCallback

The only place you're passing closeFunc, it's already wrapped in a condition for onClose being truthy.
Just drop the closeFunc entirely, and you should be fine.

mdespuits commented Apr 2, 2020

View reviewed changes

mdespuits requested review from dkordik, kcj002, pixelbandito and sebastianvera April 2, 2020 21:40

pixelbandito suggested changes Apr 3, 2020

View reviewed changes

mdespuits requested a review from pixelbandito April 3, 2020 21:22

pixelbandito reviewed Apr 6, 2020

View reviewed changes

Comment thread src/components/Tooltip/index.js

pixelbandito reviewed Apr 6, 2020

View reviewed changes

cision deleted a comment from mdespuits Apr 6, 2020

pixelbandito approved these changes Apr 6, 2020

View reviewed changes

sebastianvera reviewed Apr 6, 2020

View reviewed changes

Matthew Wells added 16 commits April 7, 2020 13:39

WIP: Tooltip

6e59903

More Tooltip additions and cleanup

44d1887

Better local variable name after extraction

be0317e

Remove unnecessary style spreading

78f9ddf

Cleanup of interface and avoid re-render issues in useEffect

19d5b8f

Adding Tooltip to examples App

a93b643

Updating README and adding to storybook

8f40fac

Pull request feedback followups

d23d8c4

More followup to pull request feedback

993e00b

Removes wrapping <div> for non-text content

6e15f1e

Cleaner left and right positioning logic (thanks Chris G)

5da7f67

Removing duplicated CSS props for tooltip

242a38b

Tooltip prop renaming

9b92e40

Close tooltips with Escape key

083adc5

Improves a11y for Tooltips using aria properties

706620d

Adds EasyRichTooltip to provide functional children

abf4074

Matthew Wells added 7 commits April 7, 2020 13:39

Adds <EasyRichTooltip /> for an uncontroller component

3dd4eb3

Rename: <Tooltip> closeable => onClose

00c005c

Removes useCallback need from Tooltip

f9fa56a

useLayoutEffect and remove duplicate margin

c457e4e

Move handleEscape inside of useEffect

8dd4445

Cleanup duplicated and unnecessary CSS properties

c63a04a

Adding base font family to Tooltip content

9a7132e

mdespuits force-pushed the component/Tooltip-wip branch from 6316942 to 9a7132e Compare April 7, 2020 18:46

mdespuits merged commit 4ff12f5 into master Apr 7, 2020

mdespuits deleted the component/Tooltip-wip branch April 7, 2020 20:00

Conversation

mdespuits commented Apr 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pixelbandito left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pixelbandito commented Apr 6, 2020

Uh oh!

sebastianvera left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mdespuits commented Apr 2, 2020 •

edited

Loading