🪟 🧹 Cleanup Tooltip component usages #15789

edmundito · 2022-08-19T14:00:26Z

What

Followup to #14581

This cleans up the tooltip naming and gets rid of redundant tooltips

Moves ToolTip to components/base/Tooltip
Replaces all ToolTip with tooltip as it's one word
Replaces duplicate tooltips with base/Tooltip
Updates variable input fields component to use the TooltipTable and show tooltip at the top, as designed
Fixes tooltip functionality in the catalog tree and makes the dropdowns look more like dropdowns:

Screen.Recording.2022-08-19.at.08.54.25.mov

Makes InfoTooltip part of base and fixes vertical alignment:

How

Lots of renaming and moving things around replaced styled-components with SCSS.

One thing that was changed in the tooltip was to render it a portal in the body. This allows the tooltip to render correctly even in situations where a parent overflow may be set to hidden, which is what was causing the catalog tree tooltips not to appear. For this, the floating-ui needs autoUpdate.

Tests

Storybook
Scrolling while tooltip visible
Showing different tooltips quickly
Variable Input field component in Salesforce source
Catalog tree cursor and primary key in connection with Postgres source

lmossman

Nice refactor! Just had a couple small questions but overall this looks good!

I tried it out locally and the tooltips/dropdowns look like they are working as expected

lmossman · 2022-08-19T20:14:02Z

airbyte-webapp/src/components/base/Tooltip/InfoTooltip.tsx

+    <Tooltip
+      control={
+        <div className={styles.container}>
+          <div className={styles.icon}>


Why have this intermediate div with its own styling that contains <InfoIcon>, instead of just putting className={styles.icon} on the <InfoIcon> component directly? Can those positioning styles not be applied directly to SVGs or something like that?

lmossman · 2022-08-19T20:17:50Z

airbyte-webapp/src/components/base/Tooltip/Tooltip.tsx

+    flip(),
+    shift(),
+  ],
+  whileElementsMounted: autoUpdate,


Is this needed because you are now using a portal to place the tooltip on the document.body?

Yes. Now it needs to move the tooltip when the user scrolls on the page.

lmossman · 2022-08-19T20:24:51Z

airbyte-webapp/src/packages/cloud/views/users/UsersSettingsView/components/RoleToolTip.tsx

-        <LineBlock>
-          <FormattedMessage id="settings.accessManagement.roleEditors" />
-        </LineBlock>
+    <InfoTooltip>


Nice abstraction 👍

lmossman · 2022-08-19T20:25:34Z

airbyte-webapp/src/views/Connection/CatalogTree/components/PathPopout.tsx

@@ -81,11 +72,9 @@ export const PathPopout: React.FC<PathPopoutProps> = (props) => {
      placeholder={props.placeholder}
      components={props.isMulti ? { MultiValue: () => null } : undefined}
      targetComponent={({ onOpen }) => (
-        <div onClick={onOpen}>
+        <PathPopoutButton items={props.isMulti ? props.path?.map(pathDisplayName) : props.path} onClick={onOpen}>


Nice refactor, this looks so much better in the UI now! I had thought those dropdowns looked bad for a long while now :)

There's a new version of this table in the works, but I figured I give it a little ❤️ until we get there.

dizel852

Overall - looks good.
Tested locally(not all tooltips)

Also noticed we started getting warning during start webpack dev-server(not significant but would be great to get rid of it)

krishnaglick

LGTM!

krishnaglick · 2022-08-22T14:39:00Z

airbyte-webapp/src/components/ArrayOfObjectsEditor/components/EditorRow.tsx

@@ -2,9 +2,9 @@ import React from "react";
 import { useIntl } from "react-intl";

 import { Button } from "components";
+import { Tooltip } from "components/base/Tooltip";


Just a thought: Should we rename base to ui?

I like the idea: base is really generic, to the point that it doesn't convey much at all except "some of this might get overridden, maybe". ui would make it clearer to me what other components I could expect to find in that directory, which already seems like a win.

edmundito · 2022-08-22T18:53:17Z

@dizel852 good catch on that. I wasn't sure what sass supported but missed the warning.

teallarson · 2022-08-22T19:53:46Z

I can't review for some reason. But onFocus should be added in addition to onMouseOver for accessibility (it'll be required once we merge the a11y linting rules). Same with onBlur and onMouseOut.

…e duplicate tooltip

Update ArrayOfObjectsEditor to render description with TooltipTable

edmundito · 2022-08-23T19:00:35Z

@tealjulia I'm going to separate the a11y focus/blur changes because after giving it a try, it may take some iterations.

* Temp rename tooltip folder to lowercase * Move Tooltip to components/base and rename to one word * Move InfoTooltip to base/Tooltip and eliminate redundancies * Fix cursor field and primary key dropdowns in the catalog tree, remove duplicate tooltip * Update ArraySection to show tooltip at the top Update ArrayOfObjectsEditor to render description with TooltipTable * Update InfoTooltip to SCSS, ensure that it's centered aligned * Ensure that Tooltips auto update on scroll * Add TooltipLearnMoreLink to Tooltip index and update imports * Fix InfoTooltip icon math in SCSS

edmundito added area/frontend area/frontend Related to the Airbyte webapp labels Aug 19, 2022

edmundito requested a review from a team as a code owner August 19, 2022 14:00

github-actions bot added the area/platform issues related to the platform label Aug 19, 2022

lmossman approved these changes Aug 19, 2022

View reviewed changes

dizel852 approved these changes Aug 22, 2022

View reviewed changes

krishnaglick approved these changes Aug 22, 2022

View reviewed changes

edmundito force-pushed the edmundito/tooltip-cleanup branch from c760b3e to cb93ab2 Compare August 22, 2022 19:10

edmundito added 9 commits August 23, 2022 08:46

Temp rename tooltip folder to lowercase

a90ff82

Move Tooltip to components/base and rename to one word

56c0c0f

Move InfoTooltip to base/Tooltip and eliminate redundancies

45e3d70

Fix cursor field and primary key dropdowns in the catalog tree, remov…

c1c97a9

…e duplicate tooltip

Update ArraySection to show tooltip at the top

c511848

Update ArrayOfObjectsEditor to render description with TooltipTable

Update InfoTooltip to SCSS, ensure that it's centered aligned

0151b1d

Ensure that Tooltips auto update on scroll

69b5f91

Add TooltipLearnMoreLink to Tooltip index and update imports

4f966c8

Fix InfoTooltip icon math in SCSS

e7da1e6

edmundito force-pushed the edmundito/tooltip-cleanup branch from cb93ab2 to e7da1e6 Compare August 23, 2022 12:46

edmundito merged commit 0910451 into master Aug 23, 2022

edmundito deleted the edmundito/tooltip-cleanup branch August 23, 2022 19:00

edmundito mentioned this pull request Aug 23, 2022

🪟 [WIP] a11y for tooltip #15892

Closed

1 task

This was referenced Aug 23, 2022

Bump Airbyte version from 0.40.1 to 0.40.2 #15909

Closed

Bump Airbyte version from 0.40.1 to 0.40.2 #15931

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🪟 🧹 Cleanup Tooltip component usages #15789

🪟 🧹 Cleanup Tooltip component usages #15789

edmundito commented Aug 19, 2022 •

edited

Loading

lmossman left a comment •

edited

Loading

lmossman Aug 19, 2022

lmossman Aug 19, 2022

edmundito Aug 23, 2022

lmossman Aug 19, 2022

lmossman Aug 19, 2022

edmundito Aug 22, 2022

dizel852 left a comment

krishnaglick left a comment

krishnaglick Aug 22, 2022

ambirdsall Aug 22, 2022

edmundito commented Aug 22, 2022

teallarson commented Aug 22, 2022 •

edited

Loading

edmundito commented Aug 23, 2022

🪟 🧹 Cleanup Tooltip component usages #15789

🪟 🧹 Cleanup Tooltip component usages #15789

Conversation

edmundito commented Aug 19, 2022 • edited Loading

What

How

Recommended reading order

Tests

lmossman left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dizel852 left a comment

Choose a reason for hiding this comment

krishnaglick left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edmundito commented Aug 22, 2022

teallarson commented Aug 22, 2022 • edited Loading

edmundito commented Aug 23, 2022

edmundito commented Aug 19, 2022 •

edited

Loading

lmossman left a comment •

edited

Loading

teallarson commented Aug 22, 2022 •

edited

Loading