Move tooltip to next #887

jinlee93 · 2022-03-08T00:50:13Z

[sc-187512]

Summary:

moves Tooltip from v0.8.1 to next
remove TW use
use CSS custom properties
convert styles to BEM
add storybook testing libraries

Test Plan:

tests pass
manual testing

Screen.Recording.2022-03-07.at.4.27.39.PM.mov

codecov · 2022-03-08T00:52:21Z

Codecov Report

Merging #887 (6db6e9f) into next (cf1fb18) will increase coverage by 18.08%.
The diff coverage is 90.76%.

@@             Coverage Diff             @@
##             next     #887       +/-   ##
===========================================
+ Coverage   61.70%   79.78%   +18.08%     
===========================================
  Files           2        4        +2     
  Lines          47       94       +47     
  Branches       21       27        +6     
===========================================
+ Hits           29       75       +46     
- Misses         17       18        +1     
  Partials        1        1

Impacted Files	Coverage Δ
src/components/Button/Button.tsx	`82.75% <80.00%> (+1.27%)`	⬆️
src/components/Tooltip/Tooltip.tsx	`95.83% <95.23%> (ø)`
src/components/Tooltip/Tooltip.stories.tsx	`100.00% <100.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 cf1fb18...6db6e9f. Read the comment docs.

jinlee93 · 2022-03-08T00:52:39Z

src/components/Tooltip/Tooltip.tsx

  );
 };
+export default Tooltip;


Noticed the original components were not exporting default, is that something we should also do?

Oh yeah, that's funny. They're always importing components like import { Button } from '../Button/Button';. I personally prefer exporting default, but I don't feel strongly about this since in the app they're going to import components like that anyway (import {Button} from "@chanzuckerberg/eds-components";).

I don't think it hurts for us to have the extra Button/index.ts file, and like the extra flexibility it gives 🙂 if that's not reflected in the plop-templates could you update them as well?

@anniehu4 @dierat @jinlee93 This is all good stuff, and happy to adopt whatever ergonomic things like this help the cause. FWIW, I wrote a post that broke down the pros and cons of all these approaches: https://bradfrost.com/blog/post/this-or-that-component-names-index-js-or-component-js/

jinlee93 · 2022-03-08T00:56:17Z

src/components/Tooltip/Tooltip.tsx

-  buttonText,
-  ...other
-}) => {
+  variant?: 'light' | 'dark';


Should this be inverted?

I'm not sure 🤔 inverted is when the color scheme is inverted so it will have enough contrast on a dark background, but both of these variants work on light and dark backgrounds. Actually, this might be a good time to bring up whether we really need both of them, and, if we do, what are the guidelines around when to use which?

yeah that's a great point @dierat

Flagged this conundrum with the team over here and we're hopefully helping make some calls here: https://bigmedium.slack.com/archives/C0310HJ9WEN/p1646949365214339

jinlee93 · 2022-03-08T00:57:20Z

src/components/Tooltip/Tooltip.module.css

+  border-style: solid;
+  border-width: var(--eds-border-width-sm);
+  border-radius: var(--eds-border-radius-lg);
+  box-shadow: var(--eds-theme-box-shadow);


I think this shadow is thinner than the original shadow, but using what is available for consistency
original on the left, this pr on the right:

I wonder if we need a ticket for updating the tokens 🤔

I think all tokens are subject to change while design is doing discovery right now; so we can feel free to replace the values with our existing ones

Sorry, to clarify, are you saying to add a token to shadows.json, or not using a token for now and assigning a value to box-shadow?

either add a token to shadows.json or update one of the base tokens to match ours 🙂 we should always be using a tier 2 token, but the values are owned by us, and will probably evolve over the next few weeks. so not sure it's worth to update all of them up front, but if we notice discrepancies we can default to the values we were already using for the component

Ah thanks for the explanation, will update shadow token md,

@anniehu4 @jinlee93 @dierat yep the token work is firming up, and I'm going to take the lead on an initial import of what they've put in place in Figma. Hoping to get that in there by the end of the day.

jinlee93 · 2022-03-08T00:58:29Z

src/components/Tooltip/Tooltip.stories.module.css

@@ -0,0 +1,20 @@
+.trigger--spacing {
+  margin: 9rem;


Weird number but it's appropriate required for the tooltip to place where alignment is indicated, otherwise Tippy places somewhere else due to lack of space

Maybe leave that in a comment?

Sounds good, added comment

let's also do the footnote commenting thing brad had, i.e.:

Suggested change

margin: 9rem;

margin: 9rem; /* 1 */

to note that this specific line is connected to the 1) comment above

jinlee93 · 2022-03-08T01:01:19Z

src/components/Tooltip/Tooltip.stories.tsx

+/* TODO: replace ForwardRefButton with EDS button since it's already using forwardRef */
+const ForwardRefButton = React.forwardRef((props: Props, ref) => (
+  <Button {...props} buttonRef={ref} />
+));
+ForwardRefButton.displayName = 'ForwardRefButton';


This is required for for Tippy. Button that will be imported from EDS already uses forwardRef

dierat · 2022-03-08T16:25:33Z

docs/CODE_GUIDELINES.md

@@ -88,7 +88,7 @@ Here's another example:

 ### CSS Nesting

-EDS uses [PostCSS Nested](https://github.com/postcss/postcss-nested) to provide some developer ergononics. As a general principle, nesting should be used sparingly and is only used in the following situations:
+EDS uses [PostCSS Nested](https://github.com/postcss/postcss-nested) to provide some developer ergonomics. As a general principle, nesting should be used sparingly and is only used in the following situations:


Wow thanks for all the spelling updates! ❤️

I somehow feel like this was some sort of test and you passed.

dierat · 2022-03-08T16:26:51Z

src/components/Counter/Counter.stories.tsx

@@ -20,6 +21,19 @@ Default.args = {
  plusButtonText: 'Add by 1',
 };

+/* TODO: replace <ForwardRefButton> with EDS button since it's already using forwardRef */


could we quickly add forwardRef to the existing button now? not sure how long EDS button conversion will take & it's nicer not having to hunt down TODOs

Yep we can, and will do

dierat · 2022-03-08T16:32:03Z

src/components/Tooltip/Tooltip.tsx

   */
-  children?: ReactNode;
+  content?: React.ReactNode;


I wonder if this should be text based on the prop guidelines Big Medium uses.

Changed to text, thanks

dierat · 2022-03-08T17:22:47Z

src/components/Tooltip/Tooltip.tsx

-   * The visually-hidden button text for the tooltip trigger button
+   * The trigger element the tooltip appears next to.
+   *
+   * Use this instead of `children` if the trigger element is being
+   * stored in a ref. Most cases will use `children` and not
+   * `reference`.
   */
-  buttonText?: string;


I can't figure out what this prop was 🤔 Do you understand the comment?

Uhh not sure what the comment means but the b/m tooltip also exported the trigger button, and buttonText is for the text inside the trigger button

dierat · 2022-03-08T17:24:39Z

src/components/Tooltip/Tooltip.tsx


-  useEffect(() => {
-    if (isActive) {


Interesting that their version of the component is focusable. I wonder why.

Hmmm in my understanding, tooltip does not receive focus

maybe they were trying to handle tooltips w/ interactive content 🤔 we don't want that tho

src/components/Tooltip/Tooltip.stories.tsx

shortcut-integration · 2022-03-08T18:05:49Z

This pull request has been linked to Shortcut Story #187512: Move Tooltip to new architecture.

dierat · 2022-03-08T21:27:09Z

src/components/Tooltip/Tooltip.tsx

+        variant === 'light' && styles['tooltip--light'],
+        variant === 'dark' && styles['tooltip--dark'],


Do you know if we've cleared with BM if we're using ampersands or object literals for these subjective classes? The code guidelines still has it using the object literals, but I think we want to keep using the ampersands. https://github.com/chanzuckerberg/edu-design-system/blob/next/docs/CODE_GUIDELINES.md#define-componentclassname

Oooh good q, I do like ampersands b/c I feel like it's easier to read

I think they wanted to see an example in code first. So here it is! @bradfrost @ifrost1 @csnizik any concerns with this syntax?

Via offline eng sync, committing to this style of syntax

dierat · 2022-03-08T21:38:57Z

Very nice!

So the open questions I still have about our process based on seeing this change are:

Do we need a ticket to update the tokens (ex: shadows)?
Should inverted be used in a case like this where there are two light/dark colors schemes but it's not related to dark backgrounds or dark mode?
Do we still need 2 colors schemes for tooltips?
Should we export a default?
Are we using objects literals or ampersands for subjective classes (ex: size === 'lg' && styles['my-component--lg' vs {[styles['my-component--lg']]: size === 'lg'}? (If using ampersands, we should update the code guidelines.)

Does that sound right?

jinlee93 · 2022-03-08T21:50:49Z

Does that sound right?

Yup that sounds right, thanks for surfacing these questions

dierat · 2022-03-08T23:05:25Z

src/components/Tooltip/Tooltip.tsx

+    <Tippy
+      {...rest}
+      className={clsx(
+        styles.tooltip,


This is a really small thing but I wonder if we want to use the brackets and quotation syntax for class names all the time for consistency or not (I see they used styles['tooltip'] in their version of the component).

let's be consistent with the brackets + quotes

dierat · 2022-03-08T23:16:05Z

src/components/Tooltip/Tooltip.tsx


-export interface Props {
+// Full list of Tippy props: https://atomiks.github.io/tippyjs/v6/all-props/
+type TooltipProps = {


Just noticed that they use Prop and we use component name + Props. We often have several sub components in a file, so it may make sense to just keep using component name + Props for consistency.

Sounds good, will change to Prop and also export it

anniehu4 · 2022-03-08T23:14:24Z

src/components/Counter/Counter.stories.tsx

@@ -20,6 +21,19 @@ Default.args = {
  plusButtonText: 'Add by 1',
 };

+/* TODO: replace <ForwardRefButton> with EDS button since it's already using forwardRef */


could we quickly add forwardRef to the existing button now? not sure how long EDS button conversion will take & it's nicer not having to hunt down TODOs

anniehu4 · 2022-03-08T23:15:34Z

src/components/Tooltip/Tooltip.module.css

@@ -1,72 +1,126 @@
-@import '../../design-tokens/mixins.css';
+/* stylelint-disable selector-pseudo-class-no-unknown */


since we're here, probs worth configuring this rule to ignore global: https://stylelint.io/user-guide/rules/list/selector-pseudo-class-no-unknown/#ignorepseudoclasses-regex-string

Will add to stylelintrc

anniehu4 · 2022-03-08T23:16:03Z

src/components/Tooltip/Tooltip.module.css

+  border-style: solid;
+  border-width: var(--eds-border-width-sm);
+  border-radius: var(--eds-border-radius-lg);
+  box-shadow: var(--eds-theme-box-shadow);


I think all tokens are subject to change while design is doing discovery right now; so we can feel free to replace the values with our existing ones

anniehu4 · 2022-03-08T23:18:32Z

src/components/Tooltip/Tooltip.stories.module.css

@@ -0,0 +1,20 @@
+.trigger--spacing {
+  margin: 9rem;


let's also do the footnote commenting thing brad had, i.e.:

Suggested change

margin: 9rem;

margin: 9rem; /* 1 */

to note that this specific line is connected to the 1) comment above

anniehu4 · 2022-03-08T23:20:18Z

src/components/Tooltip/Tooltip.tsx

   */
-  align?: 'right' | 'below' | 'left';
+  children?: React.ReactElement;


nit: can we keep the props alphabetized

Yes! Alphabetizing

anniehu4 · 2022-03-08T23:22:00Z

src/components/Tooltip/Tooltip.tsx

-  buttonText,
-  ...other
-}) => {
+  variant?: 'light' | 'dark';


yeah that's a great point @dierat

anniehu4 · 2022-03-08T23:23:02Z

src/components/Tooltip/Tooltip.tsx


-  useEffect(() => {
-    if (isActive) {


maybe they were trying to handle tooltips w/ interactive content 🤔 we don't want that tho

anniehu4 · 2022-03-08T23:23:57Z

src/components/Tooltip/Tooltip.tsx

+    <Tippy
+      {...rest}
+      className={clsx(
+        styles.tooltip,


let's be consistent with the brackets + quotes

anniehu4 · 2022-03-08T23:26:03Z

src/components/Tooltip/Tooltip.tsx

  );
 };
+export default Tooltip;


I don't think it hurts for us to have the extra Button/index.ts file, and like the extra flexibility it gives 🙂 if that's not reflected in the plop-templates could you update them as well?

anniehu4 · 2022-03-08T23:27:57Z

src/components/Tooltip/Tooltip.tsx

+        variant === 'light' && styles['tooltip--light'],
+        variant === 'dark' && styles['tooltip--dark'],


I think they wanted to see an example in code first. So here it is! @bradfrost @ifrost1 @csnizik any concerns with this syntax?

dierat · 2022-03-11T16:51:37Z

src/components/Counter/Counter.stories.tsx

+        aria-label="Hover this button to trigger the tooltip"
+        text="Hover this button to trigger the tooltip"


Nit - I don't think you need both the text and aria-label here since they're the same.

fixed, thanks

dierat

I saw go ahead without deprecating the dark variant; we can do that separately.

jinlee93 · 2022-03-14T22:31:30Z

I saw go ahead without deprecating the dark variant; we can do that separately.

Sounds good to me

jinlee93 · 2022-03-15T20:27:06Z

Will go ahead and merge as comments have been addressed. Deprecating the dark variant can be done in a separate PR after clearer deprecation guidelines have been established, and deprecation badge for storybook has been merged

* chore: move Tooltip to next * chore: update counter story since it uses tooltip * chore: add tooltip styling comments * chore: update prop and comments * chore: respond to comments * chore: nit: remove redundant prop in story

Jin Lee added 2 commits March 7, 2022 15:57

chore: move Tooltip to next

fe6309f

chore: update counter story since it uses tooltip

5e5c452

jinlee93 requested review from bradfrost and a team March 8, 2022 00:50

chore: add tooltip styling comments

3a11bbe

jinlee93 force-pushed the jlee/moveTooltip branch from 5b0f0b1 to 3a11bbe Compare March 8, 2022 00:51

jinlee93 commented Mar 8, 2022

View reviewed changes

dierat reviewed Mar 8, 2022

View reviewed changes

src/components/Tooltip/Tooltip.stories.tsx Show resolved Hide resolved

chore: update prop and comments

ec4c39d

dierat reviewed Mar 8, 2022

View reviewed changes

anniehu4 reviewed Mar 8, 2022

View reviewed changes

jinlee93 requested a review from csnizik March 9, 2022 00:05

chore: respond to comments

1ae1447

dierat reviewed Mar 11, 2022

View reviewed changes

dierat approved these changes Mar 14, 2022

View reviewed changes

chore: nit: remove redundant prop in story

6db6e9f

jinlee93 merged commit 1a3968f into next Mar 15, 2022

jinlee93 deleted the jlee/moveTooltip branch March 15, 2022 20:27

anniehu4 pushed a commit that referenced this pull request May 12, 2022

feat: move tooltip to next (#887)

96799a0

anniehu4 pushed a commit that referenced this pull request May 13, 2022

feat: move tooltip to next (#887)

83e68b4

		variant === 'light' && styles['tooltip--light'],
		variant === 'dark' && styles['tooltip--dark'],

		@@ -1,72 +1,126 @@
		@import '../../design-tokens/mixins.css';
		/* stylelint-disable selector-pseudo-class-no-unknown */

		aria-label="Hover this button to trigger the tooltip"
		text="Hover this button to trigger the tooltip"

Move tooltip to next #887

Move tooltip to next #887

Conversation

jinlee93 commented Mar 8, 2022 • edited Loading

Summary:

Test Plan:

codecov bot commented Mar 8, 2022 • edited Loading

Codecov Report

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

Choose a reason for hiding this comment

anniehu4 Mar 8, 2022 • 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

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

Choose a reason for hiding this comment

jinlee93 Mar 8, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shortcut-integration bot commented Mar 8, 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

dierat commented Mar 8, 2022

jinlee93 commented Mar 8, 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

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

Choose a reason for hiding this comment

jinlee93 commented Mar 14, 2022

jinlee93 commented Mar 15, 2022 • edited Loading

jinlee93 commented Mar 8, 2022 •

edited

Loading

codecov bot commented Mar 8, 2022 •

edited

Loading

anniehu4 Mar 8, 2022 •

edited

Loading

jinlee93 Mar 8, 2022 •

edited

Loading

jinlee93 commented Mar 15, 2022 •

edited

Loading