feat(PPDSC-2120): popover v1 #228

mstuartf · 2022-06-10T15:25:03Z

PPDSC-2120

What

Background: add Popover component
Created BaseFloatingElement component to be used by Tooltip and Popover
What does the reviewers should expect

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

Differences to ticket:

Removed support for defaultOpen prop for Tooltip and Popover due to issue with 3rd party lib
Added aria-labelledby to Popover floating element to prevent aria-dialog-name issue (also requires giving the reference element an id if it does not already have one).
overrides.distance must be a px string
Added a pointer.padding default / override so that the pointer does not reach the edge of the element (this looks strange with border radius applied).
Changed fallbackPlacement to fallbackBehaviour for clarity.

- Add base component to support Tooltip and Popover - Move offset utils and tests to base component folder - Use base component for Tooltip - Create Popover component using base component - Add basic Popover tests and stories

newskit-engineering · 2022-06-10T15:33:02Z

You can preview these changes on:
Documentation Site: http://ncu-newskit-docs.s3-website-eu-west-1.amazonaws.com/ppdsc-2120-popover-v1
Storybook: http://ncu-newskit-docs.s3-website-eu-west-1.amazonaws.com/ppdsc-2120-popover-v1/storybook

- Applying offset manually gives us more flexibility for overrides - But this is not compatible with the flip() middleware - Simplify distance override type (string) and use lib middleware - Update tests

- Use flip() and shift() middlewares to keep floating element within parent boundary - Add stories showcasing behaviour and for visual tests. Hard to test flip and shift in unit tests.

- floating-ui/floating-ui#1741 - There is a bug in the lib that prevents setting to open by default in the uncontrolled case - Remove support for this prop and update tests / stories

- Icon buttons in popover stories must have aria-label attr - Only add aria-controls to the ref element if the popover is open

src/popover/__tests__/popover.stories.tsx

src/tooltip/defaults.ts

src/tooltip/types.ts

src/tooltip/tooltip.tsx

src/base-floating-element/utils.ts

src/base-floating-element/base-floating-element.tsx

mutebg · 2022-06-17T10:47:37Z

few more things from the ticket:

its mention that Esc key shout close the popover
In the next ticket, we will need to add the Header, and Close button props, wondering if we should implement the Content component now since is part of that ticket, or leave it for later when we have the header and close btn

mstuartf · 2022-06-17T13:10:55Z

its mention that Esc key shout close the popover

I clarified this with Mike M and he said we should not do this. See comments on the ticket: https://nidigitalsolutions.jira.com/browse/PPDSC-2120

In the next ticket, we will need to add the Header, and Close button props, wondering if we should implement the Content component now since is part of that ticket, or leave it for later when we have the header and close btn

I'd rather not add any more to this PR if that's ok.

@mutebg

mutebg · 2022-06-20T05:12:34Z

its mention that Esc key shout close the popover

I clarified this with Mike M and he said we should not do this. See comments on the ticket: https://nidigitalsolutions.jira.com/browse/PPDSC-2120

In the next ticket, we will need to add the Header, and Close button props, wondering if we should implement the Content component now since is part of that ticket, or leave it for later when we have the header and close btn

I'd rather not add any more to this PR if that's ok.

@mutebg

Thanks, I should have read the comments as well. Seems good

- Focus shifts to panel (not entire Popover) on open. This is to prevent focus outline bugs on all browsers caused by the pointer rotation.

src/theme/__tests__/__snapshots__/theme.test.ts.snap

mutebg

Everything seems good to me, ready to approve it. just let me know when you fix your snapshots!

tbradbury

LGTM

* feat(PPDSC-2120): mock useId to make tooltip tests less brittle * feat(PPDSC-2120): popover v1 - Add base component to support Tooltip and Popover - Move offset utils and tests to base component folder - Use base component for Tooltip - Create Popover component using base component - Add basic Popover tests and stories * feat(PPDSC-2120): use floating-ui offset middleware - Applying offset manually gives us more flexibility for overrides - But this is not compatible with the flip() middleware - Simplify distance override type (string) and use lib middleware - Update tests * feat(PPDSC-2120): add support for arrow padding * feat(PPDSC-2120): add flip and shift support - Use flip() and shift() middlewares to keep floating element within parent boundary - Add stories showcasing behaviour and for visual tests. Hard to test flip and shift in unit tests. * feat(PPDSC-2120): remove support for default open - floating-ui/floating-ui#1741 - There is a bug in the lib that prevents setting to open by default in the uncontrolled case - Remove support for this prop and update tests / stories * feat(PPDSC-2120): fix a11y errors - Icon buttons in popover stories must have aria-label attr - Only add aria-controls to the ref element if the popover is open * feat(PPDSC-2120): fix aria-dialog-name issue * feat(PPDSC-2120): fix firefox style overrides issue * feat(PPDSC-2120): hide non-test stories from applitools * feat(PPDSC-2120): remove duplicate content check from popover * feat(PPDSC-2120): remove unneeded props from tooltip * feat(PPDSC-2120): update pointer padding prop name * feat(PPDSC-2120): simplify px override logic * feat(PPDSC-2120): don't show warnings on prod * feat(PPDSC-2120): rename positions to placements in stories * feat(PPDSC-2120): update focus behaviour - Focus shifts to panel (not entire Popover) on open. This is to prevent focus outline bugs on all browsers caused by the pointer rotation. * feat(PPDSC-2120): allow pointer events inside popover * feat(PPDSC-2120): implement design review feedback * feat(PPDSC-2120): update theme snapshots * feat(PPDSC-2120): fix a11y story contrast issue

mstuartf added 2 commits June 8, 2022 16:32

feat(PPDSC-2120): mock useId to make tooltip tests less brittle

3b21b5e

feat(PPDSC-2120): popover v1

ae7618c

- Add base component to support Tooltip and Popover - Move offset utils and tests to base component folder - Use base component for Tooltip - Create Popover component using base component - Add basic Popover tests and stories

mstuartf added the WIP Work in progress label Jun 10, 2022

mstuartf requested review from jps, Xin00163, pdimova, baburay23, mutebg, Vanals, evgenitsn and Srinivasan-Ramasamy as code owners June 10, 2022 15:25

github-actions bot added the feature This change contains a new feature label Jun 10, 2022

mstuartf added 3 commits June 14, 2022 11:59

feat(PPDSC-2120): use floating-ui offset middleware

aab303f

- Applying offset manually gives us more flexibility for overrides - But this is not compatible with the flip() middleware - Simplify distance override type (string) and use lib middleware - Update tests

feat(PPDSC-2120): add support for arrow padding

ee38341

feat(PPDSC-2120): add flip and shift support

74f8c06

- Use flip() and shift() middlewares to keep floating element within parent boundary - Add stories showcasing behaviour and for visual tests. Hard to test flip and shift in unit tests.

mstuartf force-pushed the feat/PPDSC-2120-popover-v1 branch from 7acfd7d to 74f8c06 Compare June 15, 2022 15:21

mstuartf added 6 commits June 16, 2022 08:49

Merge branch 'main' into feat/PPDSC-2120-popover-v1

6c9fb8f

feat(PPDSC-2120): remove support for default open

5f1e527

- floating-ui/floating-ui#1741 - There is a bug in the lib that prevents setting to open by default in the uncontrolled case - Remove support for this prop and update tests / stories

feat(PPDSC-2120): fix a11y errors

071af10

- Icon buttons in popover stories must have aria-label attr - Only add aria-controls to the ref element if the popover is open

feat(PPDSC-2120): fix aria-dialog-name issue

4531882

Merge branch 'main' into feat/PPDSC-2120-popover-v1

82d4749

feat(PPDSC-2120): fix firefox style overrides issue

f67264a

mstuartf added ready for review Please assist in getting this reviewed and removed WIP Work in progress labels Jun 16, 2022

mutebg reviewed Jun 17, 2022

View reviewed changes

feat(PPDSC-2120): hide non-test stories from applitools

899ded4

mstuartf added 10 commits June 20, 2022 10:16

feat(PPDSC-2120): remove duplicate content check from popover

1c3f037

feat(PPDSC-2120): remove unneeded props from tooltip

f835f18

feat(PPDSC-2120): update pointer padding prop name

d120640

feat(PPDSC-2120): simplify px override logic

59171ab

feat(PPDSC-2120): don't show warnings on prod

dae69a8

feat(PPDSC-2120): rename positions to placements in stories

4552f9c

feat(PPDSC-2120): update focus behaviour

1452f60

- Focus shifts to panel (not entire Popover) on open. This is to prevent focus outline bugs on all browsers caused by the pointer rotation.

feat(PPDSC-2120): allow pointer events inside popover

cf49f94

feat(PPDSC-2120): implement design review feedback

a4eef30

Merge branch 'main' into feat/PPDSC-2120-popover-v1

8bd458e

mutebg reviewed Jun 21, 2022

View reviewed changes

src/theme/__tests__/__snapshots__/theme.test.ts.snap Outdated Show resolved Hide resolved

mutebg reviewed Jun 21, 2022

View reviewed changes

feat(PPDSC-2120): update theme snapshots

b0bcfb5

mutebg previously approved these changes Jun 21, 2022

View reviewed changes

mstuartf added 2 commits June 21, 2022 11:03

Merge branch 'main' into feat/PPDSC-2120-popover-v1

267092f

feat(PPDSC-2120): fix a11y story contrast issue

b3387fe

mstuartf dismissed mutebg’s stale review via b3387fe June 21, 2022 11:15

mutebg approved these changes Jun 21, 2022

View reviewed changes

Merge branch 'main' into feat/PPDSC-2120-popover-v1

33bed6f

tbradbury approved these changes Jun 22, 2022

View reviewed changes

mstuartf merged commit c9d7b7e into main Jun 22, 2022

mstuartf deleted the feat/PPDSC-2120-popover-v1 branch June 22, 2022 11:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(PPDSC-2120): popover v1 #228

feat(PPDSC-2120): popover v1 #228

mstuartf commented Jun 10, 2022 •

edited

Loading

newskit-engineering commented Jun 10, 2022

mutebg commented Jun 17, 2022

mstuartf commented Jun 17, 2022 •

edited

Loading

mutebg commented Jun 20, 2022

mutebg left a comment

tbradbury left a comment

feat(PPDSC-2120): popover v1 #228

feat(PPDSC-2120): popover v1 #228

Conversation

mstuartf commented Jun 10, 2022 • edited Loading

newskit-engineering commented Jun 10, 2022

mutebg commented Jun 17, 2022

mstuartf commented Jun 17, 2022 • edited Loading

mutebg commented Jun 20, 2022

mutebg left a comment

Choose a reason for hiding this comment

tbradbury left a comment

Choose a reason for hiding this comment

mstuartf commented Jun 10, 2022 •

edited

Loading

mstuartf commented Jun 17, 2022 •

edited

Loading