feat: migrate Tooltip component #801

jinlee93 · 2022-01-21T03:57:08Z

Summary:

[sc-169594]

Copies over Tooltip from traject EDSCandidates.
Converts to Typescript and CSF storybook format.

Test Plan:

Stories match traject stories
Publish alpha release 0.5.1-alpha.2 for Traject testing
- left window local storybook with alpha release, right window main

HorizontalBar.mov

RecommendedStatusTag.mov

shortcut-integration · 2022-01-21T03:57:11Z

This pull request has been linked to Shortcut Story #169594: Tooltip: move to EDS.

jinlee93 · 2022-01-21T03:57:54Z

packages/components/src/Tooltip/Tooltip.module.css

@@ -0,0 +1,88 @@
+/* stylelint-disable selector-pseudo-class-no-unknown */


Better way via stylelintrc? Or would this be the only file with :global use?

I think this is fine. 🤷

Or would this be the only file with :global use?

My guess is that we'll eventually end up doing it again as we start building on top of other components that are not from Headless UI and require us to dig in and target a specific element that's not exposed to us. But I don't have any specific examples; it's just a feeling.

Sounds good, I think we can add to stylelint when we have to get across that bridge

github-actions

size-limit report

Path	Size
components	65.39 KB (+1.82% 🔺)
styles	4.39 KB (+0.34% 🔺)

github-actions

size-limit report

Path	Size
components	65.41 KB (+1.85% 🔺)
styles	4.39 KB (+0.34% 🔺)

codecov · 2022-01-22T00:23:56Z

Codecov Report

Merging #801 (966f690) into main (55181ec) will increase coverage by 0.03%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main     #801      +/-   ##
==========================================
+ Coverage   95.80%   95.83%   +0.03%     
==========================================
  Files         130      132       +2     
  Lines         905      937      +32     
  Branches      121      125       +4     
==========================================
+ Hits          867      898      +31     
- Misses         38       39       +1

Impacted Files	Coverage Δ
src/Tooltip/Tooltip.stories.tsx	`100.00% <0.00%> (ø)`
src/Tooltip/Tooltip.tsx	`92.85% <0.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 55181ec...966f690. Read the comment docs.

This reverts commit 81680a5.

jinlee93 · 2022-01-22T00:32:26Z

packages/components/src/Tooltip/Tooltip.tsx

+   * stored in state or a ref. Most cases will use `children` and not
+   * `reference`.
+   */
+  reference?: React.RefObject<Element> | Element;


This was React.Node in traject, but React.ReactNode not compatible with TippyProp's reference: React.RefObject<Element> | Element | null.

github-actions

size-limit report

Path	Size
components	65.4 KB (+1.84% 🔺)
styles	4.39 KB (+0.34% 🔺)

github-actions

size-limit report

Path	Size
components	65.4 KB (+1.84% 🔺)
styles	4.44 KB (+1.46% 🔺)

github-actions

size-limit report

Path	Size
components	65.4 KB (+1.84% 🔺)
styles	4.44 KB (+1.52% 🔺)

jinlee93 · 2022-01-22T02:26:55Z

All storybook styling changes were made to avoid issues Chromatic snaps cut off the tooltip, e.g.:

This happens because Chromatic snaps element <div id="root"> and Tippy mounts the tooltip in document.body, a sibling element

dierat · 2022-01-24T15:42:46Z

packages/components/src/Tooltip/__snapshots__/Tooltip.spec.ts.snap

+exports[`<Tooltip /> Interactive story renders snapshot 1`] = `
+<body>
+  <div>
+    <button
+      class="mx-32 my-32 button sizeLarge variantFlat colorBrand"
+      type="button"
+    >
+      Tooltip trigger
+    </button>
+  </div>
+</body>
+`;


Oops, I just realized that the Interactive story isn't producing a useful snapshot because it's originally closed. I added it for manual interactive testing, so I feel like we should just tell this story to not make a snapshot. But I can also see the argument that we could use this story to verify that the tooltip does appear on hover or focus by adding a test that makes it appear on hover or focus and getting the snapshot. Thoughts?

Hmmm, I'll mess around with the play function to see if I can get a meaningful snapshot while keeping it interactive, but if too much of a struggle, I can "unsnap" this

Sounds good 👍

dierat · 2022-01-24T21:34:08Z

packages/components/src/Tooltip/Tooltip.stories.tsx

+  argTypes: {
+    variant: {
+      control: {
+        type: "select",
+        options: ["light", "dark"],
+      },
+    },
+    placement: {
+      control: {
+        type: "radio",
+        options: ["left", "right", "top", "bottom"],
+      },
+    },
+  },


I think you don't need these in this repo. Try taking them out and see if you still see the controls on the first story on the docs tab.

Ohh you are right, removed

dierat · 2022-01-24T21:35:34Z

packages/components/src/Tooltip/Tooltip.stories.tsx

+export const LightVariant: StoryObj<Args> = {};
+
+export const DarkVariant: StoryObj<Args> = {
+  ...LightVariant,


What does using the LightVariant here do?

Not much, oops, removed from all the stories

ahuth · 2022-01-24T21:40:10Z

packages/components/src/Tooltip/Tooltip.tsx

+   * Use this instead of `children` if the trigger element is being
+   * stored in state or a ref. Most cases will use `children` and not
+   * `reference`.


Nice prop doc!

Did the idea/reference/wording for storing an element in state come from us or from Tippy?

For my own knowledge, I'm curious as to the intended use case. Ref makes sense, but storing an element in state isn't usually necessary (and according to my own mental pattern matching would still work with children anyway).

Think @dierat deserves all the praise for the work, I'm just migrating to EDS haha. Any thought's on Andrew's question?

I don't remember exactly where "state" came from here; I'm fine with dropping it. This is where we're currently using reference in the platform (and why we added this documentation): https://github.com/FB-PLP/traject/blob/7ab73ba57ce427d8d9b82ec587d89e6bd8121e8d/app/assets/javascripts/v2/teachers/schoolData/components/MetricCard/HorizontalBar.jsx#L171

Makes sense. I say lets drop reference to state.

ahuth

LGTM

dierat · 2022-01-24T22:30:12Z

packages/components/src/Tooltip/Tooltip.stories.tsx

+    const trigger = await screen.findByRole("button");
+    await userEvent.hover(trigger);


dierat · 2022-01-24T22:33:12Z

packages/components/src/Tooltip/Tooltip.stories.tsx

+        Tooltip trigger with hover done by storybook. <br />
+        Click within canvas to make interactive.


Is this specific to this repo? I don't have to click into the canvas in traject to get the hover to work. (You do need to have focus on that browser window though.)

The story loads and immediately hovers the button to spawn to the tooltip, so it doesn't look interactive. Have to click to despawn the tooltip created by the play function.

How should I word this to make such message come across?

Ah, I see. So you have to click to dismiss the tooltip, and then you can hover over it?

Yup. Hovering and after hover of the button also despawns it, but basically have to interact with it a little

I think this is fine as-is, but I'll throw out a suggestion for your consideration.

I'm wondering if non-devs will understand the "canvas" part here, so I was thinking you could move the text out of the button and change it to "Click somewhere in this area to dismiss the tooltip, then hover over the button to make it reappear." and then you could change the button text to something like "Hover here to see tooltip after clicking somewhere outside".

I'm probably overthinking this, so don't spend too much time dwelling on it. Thanks for figuring out how to make this story work! ❤️

Great point 👍

I think devs will probably be confused by this as well, since (I believe) "canvas" here refers to the Storybook "canvas" (basically the div element the story is rendered into, which excludes the Storybook UI), not an actual HTML canvas.

Most folks probably won't be that familiar with that terminology.

Updated story as advised, thanks!

github-actions

size-limit report

Path	Size
components	65.42 KB (+1.86% 🔺)
styles	4.44 KB (+1.52% 🔺)

github-actions

size-limit report

Path	Size
components	65.42 KB (+1.86% 🔺)
styles	4.44 KB (+1.52% 🔺)

github-actions

size-limit report

Path	Size
components	65.42 KB (+1.86% 🔺)
styles	4.44 KB (+1.52% 🔺)

feat: migrate Tooltip component

d4bb6a4

jinlee93 commented Jan 21, 2022

View reviewed changes

github-actions bot reviewed Jan 21, 2022

View reviewed changes

Jin Lee added 5 commits January 21, 2022 09:54

chore(release): v0.5.1-alpha.0

81680a5

feat(tooltip): also allow element for reference

38ec1f3

chore(release): v0.5.1-alpha.1

a221f58

feat(tooltip): add Tooltip to exports

67aafd3

chore(release): v0.5.1-alpha.2

8a432a2

github-actions bot reviewed Jan 22, 2022

View reviewed changes

Revert "chore(release): v0.5.1-alpha.0"

4fd8724

This reverts commit 81680a5.

jinlee93 commented Jan 22, 2022

View reviewed changes

github-actions bot reviewed Jan 22, 2022

View reviewed changes

feat(tooltip): correct misspelling in stories

7a621fb

github-actions bot reviewed Jan 22, 2022

View reviewed changes

feat(tooltip): style stories for better chromatic snaps

686fe82

jinlee93 force-pushed the jlee/moveTooltipToEDS branch from c965493 to 686fe82 Compare January 22, 2022 02:15

github-actions bot reviewed Jan 22, 2022

View reviewed changes

jinlee93 requested a review from a team January 22, 2022 02:28

dierat reviewed Jan 24, 2022

View reviewed changes

jinlee93 marked this pull request as ready for review January 24, 2022 16:58

dierat reviewed Jan 24, 2022

View reviewed changes

ahuth reviewed Jan 24, 2022

View reviewed changes

feat(tooltip): refine stories

7222ca4

ahuth approved these changes Jan 24, 2022

View reviewed changes

feat(tooltip): snap interactive story

12307fb

dierat reviewed Jan 24, 2022

View reviewed changes

dierat approved these changes Jan 24, 2022

View reviewed changes

Jin Lee added 3 commits January 24, 2022 15:41

feat(tooltip): add storybook interactions addon

379c375

feat(tooltip): set displayname

07b0d00

chore(tooltip): make interactive story more intuitive

7aa5bab

jinlee93 force-pushed the jlee/moveTooltipToEDS branch from 9d4e1b6 to 7aa5bab Compare January 25, 2022 22:43

github-actions bot reviewed Jan 25, 2022

View reviewed changes

Merge branch 'main' into jlee/moveTooltipToEDS

966f690

github-actions bot reviewed Jan 25, 2022

View reviewed changes

jinlee93 merged commit 9b68ee3 into main Jan 25, 2022

jinlee93 deleted the jlee/moveTooltipToEDS branch January 25, 2022 23:07

		@@ -0,0 +1,88 @@
		/* stylelint-disable selector-pseudo-class-no-unknown */

		const trigger = await screen.findByRole("button");
		await userEvent.hover(trigger);

		Tooltip trigger with hover done by storybook. <br />
		Click within canvas to make interactive.

feat: migrate Tooltip component #801

feat: migrate Tooltip component #801

Conversation

jinlee93 commented Jan 21, 2022 • edited Loading

Summary:

Test Plan:

shortcut-integration bot commented Jan 21, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

size-limit report

github-actions bot left a comment

Choose a reason for hiding this comment

size-limit report

codecov bot commented Jan 22, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

size-limit report

github-actions bot left a comment

Choose a reason for hiding this comment

size-limit report

github-actions bot left a comment

Choose a reason for hiding this comment

size-limit report

jinlee93 commented Jan 22, 2022

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

dierat Jan 24, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahuth 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

jinlee93 Jan 24, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahuth Jan 25, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

size-limit report

github-actions bot left a comment

Choose a reason for hiding this comment

size-limit report

github-actions bot left a comment

Choose a reason for hiding this comment

size-limit report

jinlee93 commented Jan 21, 2022 •

edited

Loading

codecov bot commented Jan 22, 2022 •

edited

Loading

dierat Jan 24, 2022 •

edited

Loading

jinlee93 Jan 24, 2022 •

edited

Loading

ahuth Jan 25, 2022 •

edited

Loading