Feature: Add tooltip component #1274

kris-tremblay · 2023-02-02T17:53:01Z

Add Tooltip Component: https://github.com/Shopify/internal-cli-foundations/issues/526
Add Tooltip to Status Column: https://github.com/Shopify/internal-cli-foundations/issues/527

Updated with keyboard navigation and status column tooltips (with dotted underline). Now with viewport bound checks

tooltip-with-bound-checks.mp4

API/Usage (WIP)

The Tooltip component currently takes only one prop text and accepts a single child of either type JSX.Element or string.

import {Tooltip} from '@components/Tooltip'

// Add a tooltip to a component

<Tooltip text="This is a tooltip!">
  <IconButton icon={SomeIcon} />
</Tooltip>

// Add a tooltip to an a string

<Tooltip text="This is a tooltip!">
  This string will have a dotted underline.
</Tooltip>

// Since Tooltip is wrapped an inline-block div, it may be used
// in a block of text

<p>
  Only <Tooltip text="This right here!">this section</Tooltip> will be underlined and trigger a tooltip.
</p>

Notes:

I have used fixed positioning and z-index in lieu of portals for simplicity. This can be swapped out later, but at least for this application it seems to work fine.
~~There are currently no client boundary checks, so it's possible the tooltip can be off the screen under certain circumstances.~~
The component has little accessibility support, save for it appears on focus (elements that are not "tabable" will not trigger with the tab key, ie. images) and the role of the container is set to "tooltip". This can be addressed later.

Measuring impact

How do we know this change was effective? Please choose one:

n/a - this doesn't need measurement, e.g. a linting rule or a bug-fix

github-actions · 2023-02-02T17:53:36Z

Thanks for your contribution!

Depending on what you are working on, you may want to request a review from a Shopify team:

Themes: @shopify/theme-developer-tools
UI extensions: @shopify/ui-extensions-cli
- Checkout UI extensions: @shopify/checkout-ui-extensions-api-stewardship
Hydrogen: @shopify/hydrogen
Other: @shopify/cli-foundations

github-actions · 2023-02-02T17:59:39Z

Benchmark report

The following table contains a summary of the startup time for all commands.

Status	Command	Baseline (avg)	Current (avg)	Diff
🟢	`app build`	1042 ms	1032.33 ms	-0.93 %
🟢	`app deploy`	1270.67 ms	1262.33 ms	-0.66 %
🟢	`app dev`	1269.33 ms	1257 ms	-0.97 %
🟢	`app env pull`	1183 ms	1178.67 ms	-0.37 %
🟢	`app env show`	1169 ms	1158.67 ms	-0.88 %
🟢	`app generate extension`	1248.67 ms	1234.33 ms	-1.15 %
🟢	`app generate schema`	1209.33 ms	1193.67 ms	-1.3 %
🟢	`app info`	1183.67 ms	1187 ms	0.28 %
🟢	`app scaffold extension`	1231.33 ms	1232.33 ms	0.08 %
🟢	`app update-url`	1128.33 ms	1113 ms	-1.36 %
🟢	`theme check`	1008.33 ms	989.33 ms	-1.88 %
🟢	`theme delete`	1110.33 ms	1097.33 ms	-1.17 %
🟢	`theme dev`	1130.33 ms	1094.33 ms	-3.18 %
🟢	`theme help-old`	985.67 ms	1004 ms	1.86 %
🟢	`theme info`	1032.33 ms	1037 ms	0.45 %
🟢	`theme init`	1002 ms	1005.67 ms	0.37 %
🟢	`theme language-server`	992.67 ms	968 ms	-2.48 %
🟢	`theme list`	1108.33 ms	1084.67 ms	-2.14 %
🟢	`theme open`	1112.67 ms	1108.33 ms	-0.39 %
🟢	`theme package`	1016.33 ms	1025.67 ms	0.92 %
🟢	`theme publish`	1121 ms	1118.67 ms	-0.21 %
🟢	`theme pull`	1112.33 ms	1086 ms	-2.37 %
🟢	`theme push`	1103 ms	1094 ms	-0.82 %
🟢	`theme serve`	1105 ms	1108 ms	0.27 %
🟢	`theme share`	1097.33 ms	1099.67 ms	0.21 %
🟢	`webhook trigger`	1099 ms	1097.33 ms	-0.15 %

github-actions · 2023-02-02T18:00:00Z

Coverage report

St.^❔	Category	Percentage	Covered / Total
🟡	Statements	71.85% (+0.01% 🔼)	3823/5321
🟡	Branches	69.59% (+0.33% 🔼)	1732/2489
🟡	Functions	70.13% (-0.03% 🔻)	998/1423
🟡	Lines	73.07% (-0% 🔻)	3644/4987

Show files with reduced coverage 🔻

St.^❔	File	Statements	Branches	Functions	Lines
🟡	`...` / app.ts	62% (-0.75% 🔻)	28.57%	50%	65.22% (-2.17% 🔻)
🟢	`...` / extension.ts	96.43% (-3.57% 🔻)	91.67% (-8.33% 🔻)	100%	96% (-4% 🔻)
🟢	`...` / generate.ts	95.35% (-0.11% 🔻)	89.29%	88.89% (-1.11% 🔻)	97.3% (-0.14% 🔻)
🟢	`...` / AutocompletePrompt.tsx	94.74%	93.33% (-0.42% 🔻)	100%	94.64%
🟢	`...` / SelectPrompt.tsx	95.65%	90.48% (-0.43% 🔻)	100%	95.45%

Test suite run success

980 tests passing in 498 suites.

Report generated by 🧪jest coverage report action from 8af58c2

github-actions · 2023-02-02T20:03:55Z

We detected some changes at packages/*/src and there are no updates in the .changeset.
If the changes are user-facing, run "yarn changeset add" to track your changes and include them in the next release CHANGELOG.

byrichardpowell

GJ, approving so you can merge without another review, but I'd love it if we addressed some of the feedback. Totally don't have to address it all though.

packages/ui-extensions-dev-console/src/components/Tooltip/Tooltip.tsx

packages/ui-extensions-dev-console/src/components/Tooltip/TooltipPopover.module.css

byrichardpowell · 2023-02-03T14:57:18Z

packages/ui-extensions-dev-console/src/components/Tooltip/types.ts

+  position: Position
+}
+
+export type TooltipAction = {type: 'show' | 'hide'} | {type: 'position'; payload: Position}


Pros & Cons of a seperate types file? It's something we don't do anywhere else, and I think the types are only used in one file, so I'm wondering what we are gaining.

There are a couple of types that overlap between the Tooltip and TooltipPopover components (specifically, Position appears in multiple places).

We could remove the file and shove the types into their respective files, but we would need to either duplicate Position, or simply write it out in each type that is using it (ie prop: {x: number, y: number})

byrichardpowell · 2023-02-03T15:10:48Z

packages/ui-extensions-dev-console/src/components/Tooltip/Tooltip.tsx

+    default:
+      throw new Error()
+  }
+}


It looks like there are just two actions here:

Show, which sets isVisible and the position

Hide, which sets is visible

Is a reducer overkill?

Perhaps a case of YAGNI but my assumption was that by using a reducer we would be able to easily extend it in the future. Is there a performance impact of using a reducer over setState x2?

byrichardpowell · 2023-02-03T15:11:53Z

packages/ui-extensions-dev-console/src/components/Tooltip/Tooltip.tsx

+        ref={ref}
+      >
+        {children}
+        {state.isVisible && <TooltipPopover position={state.position} text={text} />}


I think this means keyboard users won't get any value from the tooltip? How easy would it be to architect this so that keyboard users do get some value?

Looking into this now. We can add a tabIndex={0} prop to the component's div element, but the downside for anything clickable we need an additional tab to get into that in its current state.

I think that I could remedy this by using cloneElement and applying the props from the parent div. I'll look into this today and let you know.

For the second time now, it looks like cloneElement isn't needed. tabIndex={ typeof children === 'string' ? 0 : undefined } gives us the behaviour we want

I added some functionality with comments that appears to work. There was some weird behaviour when elements (such as buttons and links, anything focusable) were the child. The best solution I was able to come up with was to detect this, set the tabIndex of the first child to -1, and forward ENTER keypresses to the child. I believe this solves the keyboard navigation issues, but does present a complication in the event that we wish to add a tooltip to an input field. This could probably be fixed with a boolean prop to skip the added behaviour, or we could always detect the child node type.

byrichardpowell · 2023-02-03T15:13:23Z

packages/ui-extensions-dev-console/src/sections/Extensions/Extensions.tsx

+              <span className={styles.WithIcon}>
+                {i18n.translate('extensionList.view')}
+                <Tooltip text={i18n.translate('tooltips.viewColumnHeader')}>
+                  <Icon source={QuestionMarkMajor} />


I think the color of the Icon doesn't match the deigns, please could we update it?

Certainly. Need to modify the icon component and remove the svg fill from the extensions list.

The new version will have an optional muted prop on the Icon component. In the future we may want to add the ability to colour them.

packages/ui-extensions-dev-console/tsconfig.base.json

packages/ui-extensions-dev-console/src/components/Tooltip/TooltipPopover.module.css

kris-tremblay

Will merge pending final review on approach to better support keyboard nav

packages/ui-extensions-dev-console/src/components/Tooltip/Tooltip.tsx

kris-tremblay · 2023-02-03T16:04:08Z

packages/ui-extensions-dev-console/src/components/Tooltip/Tooltip.tsx

+        ref={ref}
+      >
+        {children}
+        {state.isVisible && <TooltipPopover position={state.position} text={text} />}


Looking into this now. We can add a tabIndex={0} prop to the component's div element, but the downside for anything clickable we need an additional tab to get into that in its current state.

I think that I could remedy this by using cloneElement and applying the props from the parent div. I'll look into this today and let you know.

kris-tremblay · 2023-02-03T21:05:04Z

packages/ui-extensions-dev-console/src/components/Tooltip/Tooltip.tsx

+        ref={ref}
+      >
+        {children}
+        {state.isVisible && <TooltipPopover position={state.position} text={text} />}


For the second time now, it looks like cloneElement isn't needed. tabIndex={ typeof children === 'string' ? 0 : undefined } gives us the behaviour we want

kris-tremblay · 2023-02-03T21:07:48Z

packages/ui-extensions-dev-console/src/sections/Extensions/Extensions.tsx

+              <span className={styles.WithIcon}>
+                {i18n.translate('extensionList.view')}
+                <Tooltip text={i18n.translate('tooltips.viewColumnHeader')}>
+                  <Icon source={QuestionMarkMajor} />


Certainly. Need to modify the icon component and remove the svg fill from the extensions list.

The new version will have an optional muted prop on the Icon component. In the future we may want to add the ability to colour them.

packages/ui-extensions-dev-console/src/components/Tooltip/TooltipPopover.module.css

kris-tremblay · 2023-02-03T21:11:21Z

packages/ui-extensions-dev-console/src/components/Tooltip/Tooltip.tsx

+    default:
+      throw new Error()
+  }
+}


Perhaps a case of YAGNI but my assumption was that by using a reducer we would be able to easily extend it in the future. Is there a performance impact of using a reducer over setState x2?

kris-tremblay · 2023-02-03T21:14:38Z

packages/ui-extensions-dev-console/src/components/Tooltip/types.ts

+  position: Position
+}
+
+export type TooltipAction = {type: 'show' | 'hide'} | {type: 'position'; payload: Position}


There are a couple of types that overlap between the Tooltip and TooltipPopover components (specifically, Position appears in multiple places).

We could remove the file and shove the types into their respective files, but we would need to either duplicate Position, or simply write it out in each type that is using it (ie prop: {x: number, y: number})

kris-tremblay · 2023-02-03T23:07:31Z

packages/ui-extensions-dev-console/src/components/Tooltip/Tooltip.tsx

+        ref={ref}
+      >
+        {children}
+        {state.isVisible && <TooltipPopover position={state.position} text={text} />}


I added some functionality with comments that appears to work. There was some weird behaviour when elements (such as buttons and links, anything focusable) were the child. The best solution I was able to come up with was to detect this, set the tabIndex of the first child to -1, and forward ENTER keypresses to the child. I believe this solves the keyboard navigation issues, but does present a complication in the event that we wish to add a tooltip to an input field. This could probably be fixed with a boolean prop to skip the added behaviour, or we could always detect the child node type.

kris-tremblay added 4 commits February 2, 2023 11:40

update lib array to include DOM

115c92c

fixed incorrect type

9746d15

first draft tooltip component and styles

b7dbc9b

add modifications for demo purposes

1d0286f

kris-tremblay added the Type: Enhancement New feature or request label Feb 2, 2023

kris-tremblay added 3 commits February 2, 2023 12:14

add prop and state interfaces

7a75807

types and replace inline css w useLayoutEffect

b8a7da7

remove background rule

d35ba1c

kris-tremblay marked this pull request as ready for review February 2, 2023 20:03

kris-tremblay requested a review from a team February 2, 2023 20:07

kris-tremblay self-assigned this Feb 2, 2023

kris-tremblay added 3 commits February 2, 2023 16:11

remove demo code

da9855d

split tooltip, popover, types, and styles

da9ee65

localize View column header tooltip

35ca86f

byrichardpowell approved these changes Feb 3, 2023

View reviewed changes

kris-tremblay added 3 commits February 3, 2023 16:09

updates/fixes per pr

9423167

add tooltip to status

ea2d5d3

proposed changes for keyboard nav

2d6736b

kris-tremblay commented Feb 3, 2023

View reviewed changes

kris-tremblay added 7 commits February 5, 2023 12:48

remove test tooltip from View col buttons

c45d6e6

fix missing classNames and remove types

3d74fe5

refactor tooltip and popover, add bound chks

df924ef

remove redundant if

6ce7b8b

remove non-null assertion

0602009

remove more non-null assertions

8409a6b

fix weird overflow + set max w based on len

8af58c2

kris-tremblay merged commit 5d79e19 into main Feb 10, 2023

kris-tremblay deleted the add-tooltip-component branch February 10, 2023 16:21

shopify-shipit bot temporarily deployed to production February 10, 2023 16:55 Inactive

shopify-shipit bot temporarily deployed to nightly February 11, 2023 02:26 Inactive

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature: Add tooltip component #1274

Feature: Add tooltip component #1274

kris-tremblay commented Feb 2, 2023 •

edited

Loading

github-actions bot commented Feb 2, 2023

github-actions bot commented Feb 2, 2023 •

edited

Loading

github-actions bot commented Feb 2, 2023 •

edited

Loading

github-actions bot commented Feb 2, 2023

byrichardpowell left a comment

byrichardpowell Feb 3, 2023

kris-tremblay Feb 3, 2023

byrichardpowell Feb 3, 2023

kris-tremblay Feb 3, 2023

byrichardpowell Feb 3, 2023

kris-tremblay Feb 3, 2023

kris-tremblay Feb 3, 2023

kris-tremblay Feb 3, 2023

byrichardpowell Feb 3, 2023

kris-tremblay Feb 3, 2023

kris-tremblay left a comment

kris-tremblay Feb 3, 2023

kris-tremblay Feb 3, 2023

kris-tremblay Feb 3, 2023

kris-tremblay Feb 3, 2023

kris-tremblay Feb 3, 2023

kris-tremblay Feb 3, 2023

Feature: Add tooltip component #1274

Feature: Add tooltip component #1274

Conversation

kris-tremblay commented Feb 2, 2023 • edited Loading

API/Usage (WIP)

Notes:

Measuring impact

github-actions bot commented Feb 2, 2023

github-actions bot commented Feb 2, 2023 • edited Loading

Benchmark report

github-actions bot commented Feb 2, 2023 • edited Loading

Coverage report

Test suite run success

github-actions bot commented Feb 2, 2023

byrichardpowell left a 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

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

kris-tremblay left a 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

Choose a reason for hiding this comment

kris-tremblay commented Feb 2, 2023 •

edited

Loading

github-actions bot commented Feb 2, 2023 •

edited

Loading

github-actions bot commented Feb 2, 2023 •

edited

Loading