feat(Tooltip): base implementation #1455

mnajdova · 2019-06-05T12:14:40Z

Fix #1453 - add Tooltip component.
A tooltip displays information related to an element when the element receives keyboard focus, or the mouse hovers over it. The Tooltip component has the following API:

Common Stardust props

accessibility
styles
variables
className
content - the content that will appear inside the tooltip
children

Custom component props

target - the element to which the tooltip is associated with
mouseLeaveDelay - delay in ms for the mouse leave event, before the tooltip will be closed.
pointing - whether the tooltip should show pointer towards the target
align - alignment for the tooltip. Can have one of the following values: 'top' | 'bottom' | 'start' | 'end' | 'center'
mountNode - Existing element the tooltip should be bound to.
position - has one of the following value: 'above' | 'below' | 'before' | 'after'. It has higher priority than align. If position is vertical ('above' | 'below') and align is also vertical ('top' | 'bottom') or if both position and align are horizontal ('before' | 'after' and 'start' | 'end' respectively), then provided value for 'align' will be ignored and 'center' will be used instead.
With this API the component will be similar to the Popup component, and it will be easy for the users to learn the new component's API.

TODO:

add Tooltip tests

codecov · 2019-06-05T12:25:52Z

Codecov Report

Merging #1455 into master will decrease coverage by 0.19%.
The diff coverage is 55.2%.

@@            Coverage Diff            @@
##           master    #1455     +/-   ##
=========================================
- Coverage   73.63%   73.44%   -0.2%     
=========================================
  Files         808      818     +10     
  Lines        6092     6179     +87     
  Branches     1774     1774             
=========================================
+ Hits         4486     4538     +52     
- Misses       1601     1636     +35     
  Partials        5        5

Impacted Files	Coverage Δ
packages/react/src/lib/positioner/Popper.tsx	`89.13% <ø> (ø)`	⬆️
packages/react/src/index.ts	`100% <ø> (ø)`	⬆️
...hemes/teams/components/Tooltip/tooltipVariables.ts	`0% <0%> (ø)`
...rast/components/Tooltip/tooltipContentVariables.ts	`0% <0%> (ø)`
...gh-contrast/components/Tooltip/tooltipVariables.ts	`0% <0%> (ø)`
...dark/components/Tooltip/tooltipContentVariables.ts	`0% <0%> (ø)`
...eams/components/Tooltip/tooltipContentVariables.ts	`0% <0%> (ø)`
...hemes/teams/components/Popup/popupContentStyles.ts	`20% <0%> (-3.08%)`	⬇️
...es/react/src/components/Tooltip/TooltipContent.tsx	`100% <100%> (ø)`
...s/teams/components/Tooltip/tooltipContentStyles.ts	`20% <20%> (ø)`
... and 14 more

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 14a91bc...38c0e6f. Read the comment docs.

-added tests

codepretty · 2019-06-06T19:35:39Z

@mnajdova thanks for starting on this!! There are just a couple things I noticed..

the pointing tooltip needs to have an svg beak for the pointer - @notandrew has the asset and can give to you
should pointing be the default? it seems to be the most common way i see tooltips
i don't think tooltips should ever render off page even if an offset is set?
i might just not have noticed this, but is there a way to specify one of the common color schemes? It might be nice to have those as example usages if possible :)
are nested tooltips a common paradigm?

jurokapsiar · 2019-06-08T06:32:35Z

Similarly to reakit tooltip or Fabric, we will need to have a reference between the tooltip between the tooltip and trigger, most probably using aria-describedby. When rendered it should look something like this:

<button aria-describedby="tooltip1">I am a button</button>
<div id="tooltip1">I am a tooltip</div>

Let's discuss how the API should look like, I would propose to make the id prop of Tooltip required so that the user would need to provide the ID. In the jsdoc comment for the id prop, we can say something like "put the tooltip id in the aria-describedby prop"

We can later (out of this PR) have some helper hook:

let tooltip = useTooltip();
<Tooltip
    {...tooltip.tooltipProps}
    trigger={<Button content="I am a button" {...tooltip.triggerProps} />}
    content={{
      content: "I am a tooltip"
    }}
  />

And for the actionable components:

<Button content="I am a button" tooltip="I am a tooltip" />

which would automatically render the Tooltip and Button with appropriate pros

mnajdova · 2019-06-10T11:13:24Z

@jurokapsiar if the id is necessary, we can make it in required and hoist it in the Tooltip component. Then in the behavior we can specify this prop as id in the content and aria-describedby in the trigger. Wouldn't this be sufficient for all cases? We can make the adding of the required props in the behavior, I don't see why we need from the client to make this correlation. Am I missing something?

I see the tooltipBehavior as something like this:

const tooltipBehavior: Accessibility = (props: any) => {
  const { id } = props
  return {
    attributes: {
      trigger: {
        'aria-describedby': id,
      },
      content: {
        id,
      },
    },
  }
}

mnajdova · 2019-06-10T11:30:26Z

@codepretty answers to your points

@mnajdova thanks for starting on this!! There are just a couple things I noticed..

the pointing tooltip needs to have an svg beak for the pointer - @notandrew has the asset and can give to you

Currently the beak is implemented exactlly as it is in the Popup. Are these two different? Let's align them both in a separate PR for red-lining the component, as this is base implementation

should pointing be the default? it seems to be the most common way i see tooltips

I was thinking the same, but as in the Popup this prop was by default false, I added that as a default. I will change this to be true by default in the Tooltip. If people are agains this, we can export the Tooltip on the client's side to have this to be true by default

i don't think tooltips should ever render off page even if an offset is set?

Let me try to remove this option in the alignment of the Tooltip, I agree is not something typical for the Tooltip.

i might just not have noticed this, but is there a way to specify one of the common color schemes? It might be nice to have those as example usages if possible :)

Would rather wait for designs for something like that before adding examples in the docs, but yes if there are use cases like this, would be happy to add more examples

are nested tooltips a common paradigm?

NO! Very valid point, will remove that example :)

jurokapsiar · 2019-06-10T12:03:39Z

we did some testing on our prototypes and it looks like we will need to change the approach: the div containing tooltip text will always need to be present in the DOM and the trigger elements will need to point to it using aria-describedby. focus/hover should then only change the visibility. let's discuss offline how this can be achieved.

Regarding your response - yes, it can work, if we are ok with modifying the trigger.
There is one more case I need to think about - if the user provides element/component with aria-describedby - then the tooltip id should probably be added to the beginning of trigger's aria-describedby.

packages/react/test/specs/components/Tooltip/Tooltip-test.tsx

-addressed other PR comments

# Conflicts: # CHANGELOG.md # packages/react/src/lib/positioner/Popper.tsx # packages/react/src/lib/positioner/types.ts

packages/react/src/themes/teams-high-contrast/components/Tooltip/tooltipContentVariables.ts

packages/react/src/components/Tooltip/Tooltip.tsx

docs/src/examples/components/Tooltip/Types/TooltipExamplePointing.tsx

docs/src/examples/components/Tooltip/Types/TooltipExamplePointing.shorthand.tsx

docs/src/examples/components/Tooltip/Types/TooltipExample.shorthand.steps.ts

bmdalex

👍 great job!

kuzhelov · 2019-06-12T12:22:20Z

packages/react/src/lib/positioner/Popper.tsx

@@ -127,7 +127,15 @@ const Popper: React.FunctionComponent<PopperProps> = props => {
      popperRef.current = createPopper(targetRef.current, contentRef.current, options)
    },
    // TODO review dependencies for popperHasScrollableParent


is this comment still necessary - I mean, if these are reviewed, we may remove TODO comment :) or there are some checks that you still expect to be made?

Not sure, this was here before. I found this issue while working on the tooltip, so I added the triggerRef there as well. I think is @Bugaa92 's call about whether it is safe to remove it

layershifter · 2019-06-12T14:36:16Z

docs/src/examples/components/Tooltip/States/TooltipOpenControlledExample.shorthand.tsx

+import { useBooleanKnob } from '@stardust-ui/docs-components'
+
+const TooltipOpenExample = () => {
+  const [open, setOpen] = useBooleanKnob({ name: 'open-c', initialValue: true })


Suggested change

const [open, setOpen] = useBooleanKnob({ name: 'open-c', initialValue: true })

const [open, setOpen] = useBooleanKnob({ name: 'open', initialValue: true })

After #1489 will be merged

layershifter · 2019-06-12T14:36:32Z

docs/src/examples/components/Tooltip/States/TooltipOpenControlledExample.tsx

+import { useBooleanKnob } from '@stardust-ui/docs-components'
+
+const TooltipOpenExample = () => {
+  const [open, setOpen] = useBooleanKnob({ name: 'open-c', initialValue: true })


Suggested change

const [open, setOpen] = useBooleanKnob({ name: 'open-c', initialValue: true })

const [open, setOpen] = useBooleanKnob({ name: 'open', initialValue: true })

After #1489 will be merged

layershifter · 2019-06-12T14:36:50Z

docs/src/examples/components/Tooltip/Variations/TooltipExamplePosition.tsx

+
+const TooltipExamplePosition = () => {
+  const [positionAndAlign] = useSelectKnob({
+    name: 'position-align-c',


Suggested change

name: 'position-align-c',

name: 'position-align',

After #1489 will be merged

layershifter · 2019-06-12T14:37:12Z

docs/src/examples/components/Tooltip/Variations/TooltipExamplePosition.shorthand.tsx

+
+const TooltipExamplePosition = () => {
+  const [positionAndAlign] = useSelectKnob({
+    name: 'position-align-s',


Suggested change

name: 'position-align-s',

name: 'position-align',

After #1489 will be merged

-wip

0f6499c

mnajdova requested review from dvdzkwsk, kuzhelov, layershifter, levithomason and miroslavstastny as code owners June 5, 2019 12:14

-simplified the Tooltip component

26721b9

-extracted pointer styles

a8a5cfb

kuzhelov added the 🚧 WIP label Jun 5, 2019

manajdov added 8 commits June 6, 2019 11:17

-fixed examples

da353bb

-fixed steps

4a2478e

-fixed steps

2905fd0

-simplified Tooltip

28c148d

-updated changelog

81859ee

-added tests

-removed Tooltip test, we need a custom one for this component

2b2a00c

-added test

42464e0

-fixed screenshot test

2196c15

mnajdova changed the title ~~[WIP] - Tooltip base component~~ feat(Tooltip): base implementation Jun 6, 2019

codepretty requested a review from notandrew June 6, 2019 19:31

mnajdova added 🚀 ready for review and removed 🚧 WIP labels Jun 10, 2019

-addressing PR comments

2ab417c

-simplified HC theme variables

fb8547a

layershifter reviewed Jun 11, 2019

View reviewed changes

packages/react/test/specs/components/Tooltip/Tooltip-test.tsx Outdated Show resolved Hide resolved

layershifter reviewed Jun 11, 2019

View reviewed changes

packages/react/test/specs/components/Tooltip/Tooltip-test.tsx Outdated Show resolved Hide resolved

manajdov and others added 7 commits June 11, 2019 17:46

-fixed target example, added steps for it

5079968

-addressed other PR comments

-fixed open examples

d0200a2

Merge branch 'master' into feat/tooltip-base-component

421dbf9

# Conflicts: # CHANGELOG.md # packages/react/src/lib/positioner/Popper.tsx # packages/react/src/lib/positioner/types.ts

-fixed pointing examples

3097b28

-reverted changes in the test

f15652e

-fixed tooltip test

7220aa3

Merge branch 'master' into feat/tooltip-base-component

c7b0428

layershifter reviewed Jun 12, 2019

View reviewed changes

packages/react/src/themes/teams-high-contrast/components/Tooltip/tooltipContentVariables.ts Outdated Show resolved Hide resolved

layershifter reviewed Jun 12, 2019

View reviewed changes

packages/react/src/components/Tooltip/Tooltip.tsx Outdated Show resolved Hide resolved

layershifter reviewed Jun 12, 2019

View reviewed changes

packages/react/src/components/Tooltip/Tooltip.tsx Outdated Show resolved Hide resolved

layershifter reviewed Jun 12, 2019

View reviewed changes

docs/src/examples/components/Tooltip/Types/TooltipExamplePointing.tsx Outdated Show resolved Hide resolved

layershifter reviewed Jun 12, 2019

View reviewed changes

docs/src/examples/components/Tooltip/Types/TooltipExamplePointing.shorthand.tsx Outdated Show resolved Hide resolved

layershifter reviewed Jun 12, 2019

View reviewed changes

docs/src/examples/components/Tooltip/Types/TooltipExample.shorthand.steps.ts Outdated Show resolved Hide resolved

manajdov and others added 4 commits June 12, 2019 13:05

-addressing comments

3047a30

-added box shadow variables

2d0079d

-changed screener step

57a6879

Merge branch 'master' into feat/tooltip-base-component

05bb166

layershifter approved these changes Jun 12, 2019

View reviewed changes

bmdalex approved these changes Jun 12, 2019

View reviewed changes

kuzhelov reviewed Jun 12, 2019

View reviewed changes

kuzhelov approved these changes Jun 12, 2019

View reviewed changes

layershifter reviewed Jun 12, 2019

View reviewed changes

Merge branch 'master' into feat/tooltip-base-component

38c0e6f

mnajdova merged commit 840a3ea into master Jun 12, 2019

delete-merged-branch bot deleted the feat/tooltip-base-component branch June 12, 2019 15:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(Tooltip): base implementation #1455

feat(Tooltip): base implementation #1455

mnajdova commented Jun 5, 2019 •

edited by kuzhelov

Loading

codecov bot commented Jun 5, 2019 •

edited

Loading

codepretty commented Jun 6, 2019

jurokapsiar commented Jun 8, 2019

mnajdova commented Jun 10, 2019 •

edited

Loading

mnajdova commented Jun 10, 2019 •

edited

Loading

jurokapsiar commented Jun 10, 2019

bmdalex left a comment

kuzhelov Jun 12, 2019

mnajdova Jun 12, 2019

layershifter Jun 12, 2019

layershifter Jun 12, 2019

layershifter Jun 12, 2019

layershifter Jun 12, 2019

	const [open, setOpen] = useBooleanKnob({ name: 'open-c', initialValue: true })
	const [open, setOpen] = useBooleanKnob({ name: 'open', initialValue: true })

feat(Tooltip): base implementation #1455

feat(Tooltip): base implementation #1455

Conversation

mnajdova commented Jun 5, 2019 • edited by kuzhelov Loading

Common Stardust props

Custom component props

codecov bot commented Jun 5, 2019 • edited Loading

Codecov Report

codepretty commented Jun 6, 2019

jurokapsiar commented Jun 8, 2019

mnajdova commented Jun 10, 2019 • edited Loading

mnajdova commented Jun 10, 2019 • edited Loading

jurokapsiar commented Jun 10, 2019

bmdalex left a comment

Choose a reason for hiding this comment

kuzhelov Jun 12, 2019

Choose a reason for hiding this comment

mnajdova Jun 12, 2019

Choose a reason for hiding this comment

layershifter Jun 12, 2019

Choose a reason for hiding this comment

layershifter Jun 12, 2019

Choose a reason for hiding this comment

layershifter Jun 12, 2019

Choose a reason for hiding this comment

layershifter Jun 12, 2019

Choose a reason for hiding this comment

mnajdova commented Jun 5, 2019 •

edited by kuzhelov

Loading

codecov bot commented Jun 5, 2019 •

edited

Loading

mnajdova commented Jun 10, 2019 •

edited

Loading

mnajdova commented Jun 10, 2019 •

edited

Loading