feat(PPDSC-2117): tooltip panel and content #204

Xin00163 · 2022-05-18T14:20:31Z

Build tooltip with https://github.com/floating-ui/floating-ui.
Accessibility discussion: Support aria-labelledby attribute floating-ui/floating-ui#1704
Didn't add onDismiss or onOpen props because it seems unnecessary for tooltip.
Add React.forwardRef to icon button

Note 🚨: If the Tooltip is wrapping a functional component, the functional component needs to accept a ref using forwardRef.

I have done:

Written unit tests against changes
Written functional tests against the component and/or NewsKit site
Updated relevant documentation

I have tested manually:

The feature's functionality is working as expected on Chrome, Firefox, Safari and Edge
The screen reader reads and flows through the elements as expected.
There are no new errors in the browser console coming from this PR.
When visual test is not added, it renders correctly on different browsers and mobile viewports (Safari, Firefox, small mobile viewport, tablet)
The Playground feature is working as expected

Before:

After:

Who should review this PR:

How to test:

newskit-engineering · 2022-05-18T14:28:04Z

You can preview these changes on:
Documentation Site: http://ncu-newskit-docs.s3-website-eu-west-1.amazonaws.com/ppdsc-2117-tooltip-panel-and-content
Storybook: http://ncu-newskit-docs.s3-website-eu-west-1.amazonaws.com/ppdsc-2117-tooltip-panel-and-content/storybook

src/tooltip/__tests__/tooltip.test.tsx

Xin00163

Thinking about adding a warning message on prod env:

Note 🚨: If the Tooltip is wrapping a functional component, the functional component needs to accept a ref using forwardRef.

Is it necessary? I am not sure how to test it though?

mutebg

Great job @Xin00163 👏 and all the examples in storybook are 💯

src/icon-button/icon-button.tsx

src/tooltip/types.ts

src/tooltip/styled.tsx

src/tooltip/tooltip.tsx

mstuartf · 2022-05-20T09:52:31Z

src/icon-button/icon-button.tsx

-  return <Button {...props} size={size} overrides={iconButtonSettings} />;
-};
+  return (
+    <Button {...props} size={size} ref={ref} overrides={iconButtonSettings} />


Is it worth adding a test for this?

src/tooltip/types.ts

src/tooltip/tooltip.tsx

Vanals · 2022-05-23T15:30:07Z

src/tooltip/tooltip.tsx

+  overrides,
+  ...props
+}) => {
+  const [open, setOpenState] = useControlled({


should this just be setOpen ? by convention. Is it for changing open 🤔 ?

I guess it's more concise

I would stick to the convention of UseState, if applicable, is more readable: https://reactjs.org/docs/hooks-state.html. Is the one we are using in the repo. [stateName, setStateName]

Vanals · 2022-05-23T15:33:18Z

src/tooltip/tooltip.tsx

+  // If tooltip is used as a label, add aria-labelledby to childrenProps and id to StyledTooltip;
+  // Because of above, 'aria-describedby' has different id for reference and floating, hence manually set below as well;
+  const id = useId();


I think would be good mention to Mike the scenario of a button with an emoji as a text! So we can explain in the documentation when they should do as such 👍

Yeah I will update the docs with more details

Is this gonna be done in an already existing ticket? can we add it in the ticket if so?

No there isn't one. Let's finish building tooltip first, then we can build docs.

Vanals · 2022-05-23T16:13:23Z

Looks good, just shared with you that issue with aria-label. Worth ticking the checkboxes once done, especially the one about Screen Readers! 😬
You can also test on NVDA with Assistive Labs

Xin00163 · 2022-05-24T09:32:16Z

Looks good, just shared with you that issue with aria-label. Worth ticking the checkboxes once done, especially the one about Screen Readers! 😬 You can also test on NVDA with Assistive Labs

So as a result, we will need to make aria-label optional in Icon Button, otherwise we can't match the aria-label with the tooltip title.

Vanals · 2022-05-24T09:58:10Z

src/tooltip/tooltip.tsx

@@ -53,13 +53,17 @@ const ThemelessTooltip: React.FC<TooltipProps> = ({
  // Because of above, 'aria-describedby' has different id for reference and floating, hence manually set below as well;
  const id = useId();

+  const titleIsString = typeof title === 'string';


isTitleString I would personally go for isTitleString. That's the convention for boolean. is interrogative 👍

I don't mind. I did see childrenIsString and testSvgCodeIsString so don't think we are following this convention

I see, unfortunately not. I think we should but ok 👍

Vanals · 2022-05-24T09:58:11Z

src/tooltip/tooltip.tsx

  const labelOrDescProps = {} as {
+    'aria-label'?: string | null;


As far as I know.I think this should be instead:

const labelOrDescProps: { 'aria-label'?: string | null; 'aria-labelledby'?: string | null; 'aria-describedby'?: string | null; } = {}

As it is now, you are "forcing it to be as.. "

src/tooltip/tooltip.tsx

Vanals · 2022-05-24T10:04:02Z

src/tooltip/tooltip.tsx

+    labelOrDescProps['aria-label'] = titleIsString ? title : null;
+    labelOrDescProps['aria-labelledby'] = open && !titleIsString ? id : null;


Which scenarios are these covering? why do we need both? Maybe add some comments would be helpful

There is a comment above but let me know if it's not clear

Vanals · 2022-05-24T10:06:19Z

Can we please use the checkboxes? and use N/a if not applicable? I think we should start getting used to those, if we want to keep them.. they would have helped to find the issue with the aria label.

src/tooltip/tooltip.tsx

Vanals · 2022-05-24T10:23:20Z

src/tooltip/tooltip.tsx

@@ -53,13 +53,17 @@ const ThemelessTooltip: React.FC<TooltipProps> = ({
  // Because of above, 'aria-describedby' has different id for reference and floating, hence manually set below as well;
  const id = useId();

+  const titleIsString = typeof title === 'string';


I see, unfortunately not. I think we should but ok 👍

* fix(PPDSC-2117): wip * feat(PPDSC-2117): add tooltip * fix(PPDSC-2117): add forwardRef to iconButton and update tests * fix(PPDSC-2117): add to index.ts * fix(PPDSC-2117): address comments * fix(PPDSC-2117): address comments * fix(PPDSC-2117): address reviews * fix(PPDSC-2117): a11y test * fix(PPDSC-2117): naming and comment * fix(PPDSC-2117): design feedback

Xin00163 added 2 commits May 11, 2022 09:27

fix(PPDSC-2117): wip

9d52ad0

feat(PPDSC-2117): add tooltip

f1c804b

Xin00163 requested review from jps, pdimova, baburay23, mutebg, Vanals, evgenitsn and Srinivasan-Ramasamy as code owners May 18, 2022 14:20

github-actions bot added the feature This change contains a new feature label May 18, 2022

Xin00163 added the ready for review Please assist in getting this reviewed label May 18, 2022

Xin00163 added the ready for design review Please assist in getting this reviewed label May 18, 2022

baburay23 reviewed May 18, 2022

View reviewed changes

src/tooltip/__tests__/tooltip.test.tsx Outdated Show resolved Hide resolved

fix(PPDSC-2117): add forwardRef to iconButton and update tests

79597b4

Xin00163 commented May 18, 2022

View reviewed changes

fix(PPDSC-2117): add to index.ts

6fddbfd

mutebg reviewed May 20, 2022

View reviewed changes

src/icon-button/icon-button.tsx Show resolved Hide resolved

src/tooltip/types.ts Outdated Show resolved Hide resolved

src/tooltip/types.ts Outdated Show resolved Hide resolved

src/tooltip/styled.tsx Outdated Show resolved Hide resolved

src/tooltip/tooltip.tsx Outdated Show resolved Hide resolved

mutebg reviewed May 20, 2022

View reviewed changes

src/tooltip/tooltip.tsx Outdated Show resolved Hide resolved

fix(PPDSC-2117): address comments

718a2ee

mstuartf reviewed May 20, 2022

View reviewed changes

Xin00163 added 2 commits May 20, 2022 15:43

fix(PPDSC-2117): address comments

2d12d9b

fix(PPDSC-2117): resolve conflicts

1b6a28b

Xin00163 removed the ready for design review Please assist in getting this reviewed label May 20, 2022

mstuartf previously approved these changes May 23, 2022

View reviewed changes

Vanals reviewed May 23, 2022

View reviewed changes

fix(PPDSC-2117): address reviews

ae6b7a3

Xin00163 dismissed mstuartf’s stale review via ae6b7a3 May 23, 2022 21:01

Xin00163 added 2 commits May 24, 2022 10:30

fix(PPDSC-2117): a11y test

c311cfd

Merge branch 'main' into feat/PPDSC-2117-tooltip-panel-and-content

bcb90dd

Xin00163 requested review from Vanals and mstuartf May 24, 2022 09:31

Vanals reviewed May 24, 2022

View reviewed changes

src/tooltip/tooltip.tsx Show resolved Hide resolved

Vanals reviewed May 24, 2022

View reviewed changes

src/tooltip/tooltip.tsx Outdated Show resolved Hide resolved

Vanals reviewed May 24, 2022

View reviewed changes

fix(PPDSC-2117): naming and comment

5136123

Vanals previously approved these changes May 24, 2022

View reviewed changes

Merge branch 'main' into feat/PPDSC-2117-tooltip-panel-and-content

9d0ef74

tbradbury previously approved these changes May 24, 2022

View reviewed changes

fix(PPDSC-2117): design feedback

92c2236

Xin00163 dismissed stale reviews from tbradbury and Vanals via 92c2236 May 24, 2022 11:09

tbradbury approved these changes May 24, 2022

View reviewed changes

Vanals approved these changes May 24, 2022

View reviewed changes

mstuartf approved these changes May 24, 2022

View reviewed changes

Merge branch 'main' into feat/PPDSC-2117-tooltip-panel-and-content

5c9072f

Xin00163 merged commit 573f35a into main May 25, 2022

Xin00163 deleted the feat/PPDSC-2117-tooltip-panel-and-content branch May 25, 2022 09:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(PPDSC-2117): tooltip panel and content #204

feat(PPDSC-2117): tooltip panel and content #204

Xin00163 commented May 18, 2022 •

edited

Loading

newskit-engineering commented May 18, 2022

Xin00163 left a comment •

edited

Loading

mutebg left a comment

mstuartf May 20, 2022

Xin00163 May 20, 2022

Vanals May 23, 2022 •

edited

Loading

Xin00163 May 23, 2022

Vanals May 23, 2022 •

edited

Loading

Vanals May 23, 2022

Xin00163 May 23, 2022

Vanals May 24, 2022 •

edited

Loading

Xin00163 May 24, 2022

Vanals commented May 23, 2022 •

edited

Loading

Xin00163 commented May 24, 2022

Vanals May 24, 2022

Xin00163 May 24, 2022

Vanals May 24, 2022

Vanals May 24, 2022

Vanals May 24, 2022

Xin00163 May 24, 2022

Vanals commented May 24, 2022 •

edited

Loading

Vanals May 24, 2022

		const labelOrDescProps = {} as {
		'aria-label'?: string \| null;

		labelOrDescProps['aria-label'] = titleIsString ? title : null;
		labelOrDescProps['aria-labelledby'] = open && !titleIsString ? id : null;

feat(PPDSC-2117): tooltip panel and content #204

feat(PPDSC-2117): tooltip panel and content #204

Conversation

Xin00163 commented May 18, 2022 • edited Loading

newskit-engineering commented May 18, 2022

Xin00163 left a comment • edited Loading

Choose a reason for hiding this comment

mutebg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Vanals May 23, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Vanals May 23, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Vanals May 24, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Vanals commented May 23, 2022 • edited Loading

Xin00163 commented May 24, 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

Vanals commented May 24, 2022 • edited Loading

Choose a reason for hiding this comment

Xin00163 commented May 18, 2022 •

edited

Loading

Xin00163 left a comment •

edited

Loading

Vanals May 23, 2022 •

edited

Loading

Vanals May 23, 2022 •

edited

Loading

Vanals May 24, 2022 •

edited

Loading

Vanals commented May 23, 2022 •

edited

Loading

Vanals commented May 24, 2022 •

edited

Loading