Tooltip component #27

jozefhruska · 2020-10-15T13:11:09Z

TODO:

Documentation
Tests
Better arrow positioning?

Closes #13.

packages/ui/src/Tooltip.tsx

packages/ui/src/Tooltip.module.scss

packages/ui/src/Tooltip.tsx

packages/ui/__stories__/utils.scss

packages/ui/src/Tooltip.tsx

aigoncharov · 2020-10-16T12:42:17Z

packages/ui/src/Tooltip.tsx

+  }, [])
+
+  return (
+    <div onMouseEnter={onMouseEnter} onMouseLeave={onMouseLeave} className={styles.container}>


Try setting this div's position to relative. It changes how the tooltip looks. We cannot control the parent layout and where the tooltip is going to be used. Should we try to work around it? I briefly tried strategy: fixed, but it was buggy during scrolling.

Should we try adding various edge cases to our stories?

What edge cases do you have in mind?

Just an idea, I'm not sure how doable is it in Storybook. Could we position a tooltip to the corners so that we make sure a tooltip opens in the visible part of the screen? This is the most common problem with it.

Isn't this just re-testing of something that Popper.js has already covered?

aigoncharov · 2020-10-16T12:55:17Z

Guys, rc-trigger goes above and beyond to get the underlying ref from children (see https://github.com/react-component/trigger/blob/master/src/index.tsx). I would be against excessive wrapping as it complicates the layout with some hidden components.
How would you feel if used a render prop instead?

<Tooltip>
  (triggerRef) => <Button ref={trigger} />
</Tooltip>

It is not that elegant, but it significantly simplifies the code and removes the need for extra layout.

To mitigate the disabled button issue if we see it in our stories, we could not really disable the button, but just make it look disabled.

danad02 · 2020-10-16T13:33:38Z

@aigoncharov I don't mind using a render props as a solution. As long as it simplifies the layout I'm in.

To mitigate the disabled button issue if we see it in our stories, we could not really disable the button, but just make it look disabled.

But I don't like this, it seems like we would start bending the rules set by standard. I would rather wrap disabled buttons and add a comment.

jozefhruska · 2020-10-16T14:51:39Z

Guys, rc-trigger goes above and beyond to get the underlying ref from children (see https://github.com/react-component/trigger/blob/master/src/index.tsx). I would be against excessive wrapping as it complicates the layout with some hidden components.
How would you feel if used a render prop instead?
<Tooltip>
  (triggerRef) => <Button ref={trigger} />
</Tooltip>
It is not that elegant, but it significantly simplifies the code and removes the need for extra layout.

To mitigate the disabled button issue if we see it in our stories, we could not really disable the button, but just make it look disabled.

Actually this was my first implementation of this Tooltip, but then I though you won't like this approach. I'm all for it! ❤️ Or maybe we can support both cases?

packages/ui/__stories__/Tooltip.stories.tsx

packages/ui/__tests__/Tooltip.test.tsx

packages/ui/src/TruncatedText.tsx

packages/ui/src/Tooltip.tsx

packages/ui/styles/constants/_sizes.scss

aigoncharov · 2020-10-30T16:14:52Z

setupTests.base.ts

@@ -10,11 +10,13 @@ beforeEach(() => {
  jest.spyOn(console, 'error').mockImplementation((...args) => {
    isConsoleErrorOrWarning = true
    originalError(...args)
+    console.log('ERROR', ...args)


Why do we need it? We already do console.error and console.warn via originalError and originalWarn?

Indeed, this shouldn't be there, good catch!

aigoncharov · 2020-10-30T16:21:41Z

packages/ui/src/Tooltip.module.scss

-.wrapper {
-  max-width: c.$tooltipMaxWidth;
-  word-wrap: break-word;
+.tooltip-sr {


For consistency with other stylesheets

Suggested change

.tooltip-sr {

.tooltipSr {

Updated manually 👍

packages/ui/src/Tooltip.module.scss

aigoncharov · 2020-10-30T16:23:39Z

packages/ui/src/Tooltip.module.scss

+  background: c.$colorNeutralLighter;
+  border: c.$borderWidth solid c.$colorNeutralLight;
+  border-radius: c.$borderRadius;
+  font-size: inherit;


Why do you think we need to inherit the font-size? I figured it would be the same for all tooltips

You're right, I don't know what I was thinking 🤷‍♂️ Removed 👍

packages/ui/src/TruncatedText.tsx

packages/ui/src/HiddenLabel.module.scss

aigoncharov · 2020-10-30T16:27:52Z

packages/ui/src/Help.tsx

+          className={cn(styles.container, className, {
+            [styles.padding]: padding === 'default',
+          })}
+          aria-describedby={tooltipId}


nit: Should we move it to the Icon? I feel like we are describing the icon actually

Hmm, I'm not sure, but probably it makes more sense on icon. Updated 👍

aigoncharov · 2020-10-30T16:29:13Z

packages/ui/src/Button.tsx

 )
+
+Button.displayName = 'Button'


Why do we need this one?

ESLint requires explicit displayName when using forwardRef.

aigoncharov · 2020-10-30T16:34:58Z

packages/ui/src/Button.tsx

@@ -44,10 +46,11 @@ export type ButtonAccessibleIconRightProps =
    }

 export type ButtonDisabledProps =
-  | ({ disabledTooltip: string } & Required<Pick<ButtonHTMLAttributes<HTMLButtonElement>, 'disabled'>>)
+  | ({ disabledTooltip: string; disabledTooltipId: string } & Required<Pick<ButtonHTMLAttributes<HTMLButtonElement>, 'disabled'>>)


Why do we need to pass it? Couldn't we always generate it internally?

I think we can, removed 👍

aigoncharov · 2020-10-30T16:35:19Z

packages/ui/src/Button.tsx

+              [styles.secondary]: kind === 'secondary',
+              [styles.transparent]: kind === 'transparent',
+            })}
+            disabled={disabled}


Should we add aria-describedby here?

Indeed, good catch, added 👍

packages/ui/__tests__/Toast.test.tsx

aigoncharov · 2020-10-30T16:36:51Z

packages/ui/__tests__/TextField.test.tsx

-      helperText: 'A long time ago in a galaxy far, far away....',
-      className: styles.helperText,
+    // This has to be wrapper in act() - you can find an explanation in Tooltip test file.
+    await act(async () => {


Does it still have to be wrapped in act? I thought mountAndCheckA11Y now does that automatically

It does not, it's just a leftover when mountAndCheckA11Y didn't do it automatically. Removed 👍

packages/ui/src/Tooltip.tsx

packages/ui/src/Tooltip.module.scss

aigoncharov · 2020-11-02T13:25:04Z

packages/ui/src/Tooltip.module.scss

+
+      &::after {
+        border-top: c.$tooltipArrowSize solid c.$colorNeutralLighter;
+        transform: translateY(-2px);


Suggested change

transform: translateY(-2px);

transform: translateY(-(c.$tooltipArrowSize / 3));

aigoncharov · 2020-11-02T13:25:28Z

packages/ui/src/Tooltip.module.scss

+
+      &::after {
+        border-right: c.$tooltipArrowSize solid c.$colorNeutralLighter;
+        transform: translateX(-4px);


Suggested change

transform: translateX(-4px);

transform: translateX(-(c.$tooltipArrowSize / 3 * 2));

aigoncharov · 2020-11-02T13:25:45Z

packages/ui/src/Tooltip.module.scss

+
+      &::after {
+        border-bottom: c.$tooltipArrowSize solid c.$colorNeutralLighter;
+        transform: translateY(-4px);


Suggested change

transform: translateY(-4px);

transform: translateY(-(c.$tooltipArrowSize / 3 * 2));

aigoncharov · 2020-11-02T13:25:59Z

packages/ui/src/Tooltip.module.scss

+
+      &::after {
+        border-left: c.$tooltipArrowSize solid c.$colorNeutralLighter;
+        transform: translateX(-2px);


Suggested change

transform: translateX(-2px);

transform: translateX(-(c.$tooltipArrowSize / 3));

aigoncharov

Great job! Love how it looks in the end!

aigoncharov · 2020-11-02T13:29:06Z

packages/ui/__stories__/Tooltip.stories.tsx

+}
+
+export const AutoPlacement = () => (
+  <div className={utilStyles.wrapperCentered}>


Should we add more padding so the screenshots are not cropped?

aigoncharov · 2020-11-02T13:32:44Z

packages/ui/__tests__/Tooltip.test.tsx

-    expect(tooltip.exists()).toBeTruthy()
-    expect(tooltip.props()).toEqual(expectedProps)
-    expect(tooltip.exists(Child)).toBeTruthy()
+    expect(wrapper.findDataTest('tooltip-overlay').hasClass('hidden')).toBeFalsy()


nit

Suggested change

expect(wrapper.findDataTest('tooltip-overlay').hasClass('hidden')).toBeFalsy()

expect(wrapper.findDataTest('tooltip-overlay').hasClass(styles.hidden)).toBeFalsy()

aigoncharov · 2020-11-02T13:34:03Z

packages/ui/__tests__/Tooltip.test.tsx

-    const Child = () => <div />
+    expect(wrapper.findDataTest('tooltip-reference').exists()).toBeTruthy()
+    expect(tooltipOverlay.exists()).toBeTruthy()
+    expect(tooltipOverlay.text()).toEqual('Tooltip content')



Suggested change

expect(tooltipOverlay.hasClass(styles.hidden)).toBeTruthy()

danad02

LGTM. Great job 👏

jozefhruska added the UI label Oct 15, 2020

jozefhruska requested review from aigoncharov, danad02, jankoritak and jvorcak as code owners October 16, 2020 08:37

jozefhruska force-pushed the feat/tooltip branch from 4e96b51 to 963d6ff Compare October 16, 2020 09:47