feat: Create Tooltip component #871

noelledusahel · 2021-02-11T15:53:59Z

Summary

This story creates a react component modeled after the USWDS Tooltip

Related Issues or PRs

How To Test

In storybook see Components > Tooltips

yarn test
yarn storybook

Screenshots (optional)

…e tooltipbody in DOM

…h within useEffent

noelledusahel · 2021-02-23T17:39:50Z

src/components/Tooltip/Tooltip.tsx

+ 'is-visible': isVisible,
+ })
+
+ useEffect(() => {


I placed the position logic within a useEffect hook because certain calculations like tooltipBodyWidth were being generated when the tooltip was offscreen (!isVisible), causing issues with marginLeft within the positionLeft function

…Width

suzubara

This is great work! I have some suggestions/comments:

Please remove the package-lock.json file - this was likely added as a result of using npm install but we should be consistent in using yarn on this project.
I think the button element with the title attribute should be generated by this component, since the USWDS component follows a strict convention (must be a button element and tooltip body is populated with the value of the title attribute) instead of using props.children. This would mean usage would look more like: <Tooltip position="top" title="Top" /> and I think intrinsic props should be for the button element (since the wrapper span in USWDS is not customizable other than adding classes, so we could expose a wrapperClasses prop for that)
I think some additional event handlers are needed, the tooltip should show on focus/mouseenter and hide on mouseleave, blur, keydown according to the source JS

src/components/Tooltip/Tooltip.test.tsx

suzubara · 2021-02-23T17:33:30Z

src/components/Tooltip/Tooltip.tsx

+
+const TRIANGLE_SIZE = 5
+const SPACER = 2
+const tooltipID = `tooltip-${Math.floor(Math.random() * 900000) + 100000}`


I think this should probably be initialized inside the Tooltip component, probably inside a useEffect hook so it just happens once on initial mount.

src/components/Tooltip/Tooltip.tsx

suzubara · 2021-02-23T17:43:59Z

src/components/Tooltip/Tooltip.tsx

+ const adjustToEdgeX = tooltipWidth + TRIANGLE_SIZE + SPACER
+ const adjustToEdgeY = tooltipHeight + TRIANGLE_SIZE + SPACER
+
+ /**


I think it would be good to take advantage of React's declarative nature here, and set these attributes (style, classes) using useState and pass them to the element(s) inside of the markup instead of manipulating the HTML elements directly

very much agree about avoiding element.style since we have JSX className. It is a refactor though and the way the code is written seems fine to me for a first pass (since it just copies uswds). We could backlog an issue about refactoring this. @suzubara thoughts?

yes, I'm okay with that!

brandonlenz · 2021-02-23T18:12:39Z

I think the button element with the title attribute should be generated by this component, since the USWDS component follows a strict convention (must be a button element and tooltip body is populated with the value of the title attribute) instead of using props.children. This would mean usage would look more like: <Tooltip position="top" title="Top" /> and I think intrinsic props should be for the button element (since the wrapper span in USWDS is not customizable other than adding classes, so we could expose a wrapperClasses prop for that)

@suzubara I advised to follow a flexible approach here. While all the uswds examples are buttons, they make a specific mention of using it on more than just buttons in the docs:

I could definitely see people wanting to wrap a custom link in a tooltip, which I thought we might want to allow.

suzubara · 2021-03-09T18:59:05Z

Do we want to add a wrapperClasses prop that mirrors the data-classes attribute used by the USWDS Tooltip?

JavaScript generated most of the tooltip’s markup. Apply utility classes to any tooltip’s wrapping element, by including them inside a data-classes attribute, separated by spaces.

@suzubara on the USWDS side data-classes gets applied on the tooltip trigger. are you referirng to a wrapperClass that applies classes to the wrapper span or to the tooltip trigger?

Actually I read further, and see the classes get applied to the wrapper. I'm curious whether you think it makes sense to use the same terms as USWDS, and use the prop name data-classes rather than wrapperClasses

I don't feel super strongly about the exact name, but my inclination is to use something explicit, like wrapperClasses. The reason is because data- attributes are not specific to React, and in the case of USWDS they are being used to easily interface between vanilla HTML & JS. So I would recommend not naming a prop anything like data- because it will probably not be clear that it's a React prop.

suzubara

I tested this out on MilMove and the Tooltip works great! I left two minor comments that I think should be fixed before merging. We should also make sure to create an issue to refactor the positioning code so it uses React state to store class/position, instead of JS DOM APIs.

suzubara · 2021-03-12T22:25:51Z

src/components/Tooltip/Tooltip.tsx

+ return 'asCustom' in props
+}
+
+const tooltipID = `tooltip-${Math.floor(Math.random() * 900000) + 100000}`


Because this tooltipID is defined outside of the Tooltip component, it's the same value for all instances of a Tooltip used within a single app. This should be defined inside of the Tooltip component in a useRef container:

const tooltipID = useRef(`tooltip-${Math.floor(Math.random() * 900000) + 100000}`) // to access: tooltipID.current

suzubara · 2021-03-12T22:28:12Z

src/components/Tooltip/Tooltip.tsx

+ label: string
+ position?: 'top' | 'bottom' | 'left' | 'right' | undefined
+ className?: string
+ dataClasses?: string


so I tried this out and actually React shows an error in the console because of a potential naming conflict with native data- attributes. I think this prop should be renamed to wrapperClasses.

suzubara · 2021-03-12T22:31:48Z

src/components/Tooltip/Tooltip.tsx

+ data-testid="tooltipWrapper"
+ ref={wrapperRef}
+ className="usa-tooltip"
+ {...customProps}


customProps are passed into the triggerElement below, so they should not be repeated on this wrapper span

suzubara · 2021-03-12T22:32:37Z

src/components/Tooltip/Tooltip.tsx

+ data-testid="tooltipWrapper"
+ ref={wrapperRef}
+ className="usa-tooltip"
+ {...spanProps}


I think the remaining props are meant to be passed onto the button element, rather than the wrapper span (this should mirror the custom implementation above)

…e tooltipElementID to tooltip component level

noelledusahel · 2021-03-16T16:32:05Z

src/components/Tooltip/Tooltip.tsx

+type TooltipProps<T> = {
+ label: string
+ position?: 'top' | 'bottom' | 'left' | 'right' | undefined
+ wrapperclasses?: string


i removed className prop, as i didn't see the purpose of having both className and wrapperclasses. may have lost some perspective for why we'd need a separate prop since starting this ticket so your input would be appreciated @suzubara

The className prop is meant to allow a user to pass a custom CSS class to the trigger element, so that value should be incorporated in the list of classes passed to the button or custom element used. wrapperClasses is the same, but for the wrapper element instead (this mirrors the data-classes attribute exposed by USWDS).

For example if a user of USWDS did this:

<button type="button" class="usa-button usa-tooltip myCustomButtonClass" data-position="top" data-classes="width-full tablet:width-auto" title="Top">Show on top</button>

they should be able to achieve the same result using our Tooltip component with this:

<Tooltip className="myCustomButtonClass" position="top" wrapperClasses="width-full tablet:width-auto" label="Top">Show on top</button>

and that should result in the tooltipWrapper element having the wrapperClasses value (width-full tablet:width-auto), and the triggerElement element having the className value (myCustomButtonClass) in addition to the other usa tooltip classes

awesome, I think my latest change should fix it @suzubara

suzubara

awesome, I think this is a great first pass at the Tooltip! I will create a followup issue for refactoring some of the code to use more React and use less of the JS DOM API

noelledusahel · 2021-03-16T21:40:24Z

awesome, I think this is a great first pass at the Tooltip! I will create a followup issue for refactoring some of the code to use more React and use less of the JS DOM API

gotcha, when you do please add me as a reviewer!

haworku

⭐ @noelledusahel

brandonlenz

Small change. Otherwise good to approve! Great work!

src/components/Tooltip/Tooltop.stories.tsx

* create tooltip using css usilities * additional styles * add useState hooks to tooltip component * add correct styles according to position * additional edits to positioning tooltip body, add element ID to locate tooltipbody in DOM * move all positioning login into a useEffect, register TooltipBodyWidth within useEffent * add tooltip ID and remove comments * remove styles file * add tests to tooltip component * export tooltip component in index.ts * replace reference to tooltipBodyRef.current with variable tooltipBodyWidth * remove package lock file * create a default tooltip trigger button and create tests for tooltip body visibility * begin writing up custom component based off of Link component example * put custom component render in a conditional statement * utilize forward ref in customComponent * abstract useEffect into a fn, useTooltip * move html element role, move event listeners to custom component props * move isElementInViewport to a utils file * correct tests and add test id to tooltip trigger element * add display name * add story displayname * corect var name CustomLinkProps * update tests * add data classes prop for css utility classes * add keyboard events to tests * change dataclasses to wrapperclasses and move to trigger element, move tooltipElementID to tooltip component level * remove className for wrapperclasses from tooltipComponent * apply classname to tooltipTrigger element * reformat for storybook addon Co-authored-by: Andrew Hobson <21983+ahobson@users.noreply.github.com>

create tooltip using css usilities

9e83e45

noelledusahel requested review from ahobson, brandonlenz, gidjin, haworku, sarboc, SirenaBorracha, suzubara and tinyels as code owners February 11, 2021 15:54

additional styles

bddd6df

noelledusahel added the status: wip Work in progress - not ready for code review label Feb 11, 2021

noelledusahel changed the title ~~Create Tooltip component~~ WIP - Create Tooltip component Feb 11, 2021

add useState hooks to tooltip component

e7614a7

noelledusahel marked this pull request as draft February 15, 2021 01:28

noelledusahel added 9 commits February 17, 2021 10:53

add correct styles according to position

69f46d8

Merge remote-tracking branch 'origin/main' into nb-react-uswds-tooltip

378d9ce

additional edits to positioning tooltip body, add element ID to locat…

f2e4cb4

…e tooltipbody in DOM

move all positioning login into a useEffect, register TooltipBodyWidt…

41c6529

…h within useEffent

add tooltip ID and remove comments

ff163b5

remove styles file

7da70f6

add tests to tooltip component

86ed906

export tooltip component in index.ts

3d04526

Merge remote-tracking branch 'origin/main' into nb-react-uswds-tooltip

05455d8

noelledusahel marked this pull request as ready for review February 23, 2021 16:45

noelledusahel changed the title ~~WIP - Create Tooltip component~~ Feat: Create Tooltip component Feb 23, 2021

noelledusahel changed the title ~~Feat: Create Tooltip component~~ feat: Create Tooltip component Feb 23, 2021

noelledusahel commented Feb 23, 2021

View reviewed changes

replace reference to tooltipBodyRef.current with variable tooltipBody…

54d7527

…Width

suzubara suggested changes Feb 23, 2021

View reviewed changes

trussworks-infra-zz temporarily deployed to storybook March 9, 2021 17:01 Inactive

suzubara reviewed Mar 12, 2021

View reviewed changes

noelledusahel added 2 commits March 16, 2021 11:52

change dataclasses to wrapperclasses and move to trigger element, mov…

ac29ad6

…e tooltipElementID to tooltip component level

Merge remote-tracking branch 'origin/main' into nb-react-uswds-tooltip

f261e00

noelledusahel requested a review from suzubara March 16, 2021 15:53

remove className for wrapperclasses from tooltipComponent

a0fbe6e

noelledusahel commented Mar 16, 2021

View reviewed changes

trussworks-infra-zz temporarily deployed to storybook March 16, 2021 16:32 Inactive

apply classname to tooltipTrigger element

92763d1

trussworks-infra-zz temporarily deployed to storybook March 16, 2021 18:38 Inactive

suzubara previously approved these changes Mar 16, 2021

View reviewed changes

suzubara mentioned this pull request Mar 16, 2021

[refactor] Minimize use of JS DOM API in the Tooltip component #1027

Closed

haworku previously approved these changes Mar 17, 2021

View reviewed changes

brandonlenz requested changes Mar 17, 2021

View reviewed changes

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

noelledusahel added 2 commits March 18, 2021 11:58

reformat for storybook addon

a50949c

Merge remote-tracking branch 'origin/main' into nb-react-uswds-tooltip

e11b26e

noelledusahel dismissed stale reviews from haworku and suzubara via e11b26e March 18, 2021 15:59

noelledusahel requested a review from brandonlenz March 18, 2021 15:59

trussworks-infra-zz temporarily deployed to storybook March 18, 2021 16:05 Inactive

suzubara approved these changes Mar 18, 2021

View reviewed changes

brandonlenz approved these changes Mar 18, 2021

View reviewed changes

noelledusahel merged commit 92ea5f0 into main Mar 18, 2021

noelledusahel deleted the nb-react-uswds-tooltip branch March 18, 2021 17:20

brandonlenz mentioned this pull request Mar 22, 2021

chore(release): 1.14.0 #1043

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Create Tooltip component #871

feat: Create Tooltip component #871

noelledusahel commented Feb 11, 2021 •

edited

Loading

noelledusahel Feb 23, 2021 •

edited

Loading

suzubara left a comment

suzubara Feb 23, 2021

suzubara Feb 23, 2021

haworku Mar 2, 2021

suzubara Mar 3, 2021

brandonlenz commented Feb 23, 2021

suzubara commented Mar 9, 2021

suzubara left a comment

suzubara Mar 12, 2021

suzubara Mar 12, 2021

suzubara Mar 12, 2021

suzubara Mar 12, 2021

noelledusahel Mar 16, 2021

suzubara Mar 16, 2021

noelledusahel Mar 16, 2021

suzubara left a comment

noelledusahel commented Mar 16, 2021

haworku left a comment

brandonlenz left a comment

feat: Create Tooltip component #871

feat: Create Tooltip component #871

Conversation

noelledusahel commented Feb 11, 2021 • edited Loading

Summary

Related Issues or PRs

How To Test

Screenshots (optional)

noelledusahel Feb 23, 2021 • edited Loading

Choose a reason for hiding this comment

suzubara 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

brandonlenz commented Feb 23, 2021

suzubara commented Mar 9, 2021

suzubara 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

suzubara left a comment

Choose a reason for hiding this comment

noelledusahel commented Mar 16, 2021

haworku left a comment

Choose a reason for hiding this comment

brandonlenz left a comment

Choose a reason for hiding this comment

noelledusahel commented Feb 11, 2021 •

edited

Loading

noelledusahel Feb 23, 2021 •

edited

Loading