From cd8ab7e35b918e2b7fdc44c5c6848fcbc1c07367 Mon Sep 17 00:00:00 2001 From: Ollie Curtis <8831547+olliecurtis@users.noreply.github.com> Date: Mon, 3 Jun 2024 15:47:16 +0100 Subject: [PATCH 01/11] [ARGG-1157][BpkTooltip]: Migrate tooltip to floating-ui --- .../{examples.js => examples.tsx} | 27 +- .../{stories.js => stories.ts} | 6 +- packages/bpk-component-tooltip/index.ts | 8 +- .../src/BpkTooltip-test.tsx | 43 ++-- .../src/BpkTooltip.module.scss | 100 +------- .../bpk-component-tooltip/src/BpkTooltip.tsx | 142 +++++++++-- .../src/BpkTooltipPortal-test.tsx | 94 ------- .../src/BpkTooltipPortal.tsx | 230 ------------------ .../__snapshots__/BpkTooltip-test.tsx.snap | 93 ------- .../BpkTooltipPortal-test.tsx.snap | 138 ----------- .../src/accessibility-test.tsx | 11 +- 11 files changed, 158 insertions(+), 734 deletions(-) rename examples/bpk-component-tooltip/{examples.js => examples.tsx} (88%) rename examples/bpk-component-tooltip/{stories.js => stories.ts} (91%) delete mode 100644 packages/bpk-component-tooltip/src/BpkTooltipPortal-test.tsx delete mode 100644 packages/bpk-component-tooltip/src/BpkTooltipPortal.tsx delete mode 100644 packages/bpk-component-tooltip/src/__snapshots__/BpkTooltip-test.tsx.snap delete mode 100644 packages/bpk-component-tooltip/src/__snapshots__/BpkTooltipPortal-test.tsx.snap diff --git a/examples/bpk-component-tooltip/examples.js b/examples/bpk-component-tooltip/examples.tsx similarity index 88% rename from examples/bpk-component-tooltip/examples.js rename to examples/bpk-component-tooltip/examples.tsx index 9bdd003567..9887f9f7ce 100644 --- a/examples/bpk-component-tooltip/examples.js +++ b/examples/bpk-component-tooltip/examples.tsx @@ -16,8 +16,6 @@ * limitations under the License. */ -/* @flow strict */ - import { useRef, forwardRef } from 'react'; import { @@ -41,7 +39,7 @@ const Heading = withDefaultProps(BpkText, { tagName: 'h4', }); -const HeadingComponent = forwardRef((props: { children: Node }, ref) => ( +const HeadingComponent = forwardRef((props, ref) => (
{props.children}
@@ -148,28 +146,6 @@ const LinkExample = () => { ); }; -const PopperModifiersExample = () => { - const target = useRef(null); - - return ( -
- BER} - popperModifiers={[ - { - name: 'flip', - options: { enabled: false }, - }, - ]} - > - Berlin Brandenburg Airport - -
- ); -}; - const FocusExample = () => { const targetRef1 = useRef(null); const targetRef2 = useRef(null); @@ -219,6 +195,5 @@ export { SideExample, NoPaddingExample, LinkExample, - PopperModifiersExample, FocusExample, }; diff --git a/examples/bpk-component-tooltip/stories.js b/examples/bpk-component-tooltip/stories.ts similarity index 91% rename from examples/bpk-component-tooltip/stories.js rename to examples/bpk-component-tooltip/stories.ts index 3e682fabfa..ee4f0ba495 100644 --- a/examples/bpk-component-tooltip/stories.js +++ b/examples/bpk-component-tooltip/stories.ts @@ -16,8 +16,7 @@ * limitations under the License. */ - -import BpkTooltip from '../../packages/bpk-component-tooltip/src/BpkTooltipPortal'; +import BpkTooltip from '../../packages/bpk-component-tooltip/src/BpkTooltip'; import { DefaultExample, @@ -25,7 +24,6 @@ import { SideExample, NoPaddingExample, LinkExample, - PopperModifiersExample, FocusExample, } from './examples'; @@ -42,6 +40,4 @@ export const WithoutPadding = NoPaddingExample; export const OnALink = LinkExample; -export const PopperModifiers = PopperModifiersExample; - export const Focus = FocusExample; diff --git a/packages/bpk-component-tooltip/index.ts b/packages/bpk-component-tooltip/index.ts index 831c008acd..385c5617d4 100644 --- a/packages/bpk-component-tooltip/index.ts +++ b/packages/bpk-component-tooltip/index.ts @@ -16,12 +16,12 @@ * limitations under the License. */ -import BpkTooltipPortal from './src/BpkTooltipPortal'; +import BpkTooltip from './src/BpkTooltip'; import { TOOLTIP_TYPES } from './src/constants'; -import type { Props } from './src/BpkTooltipPortal'; +import type { Props } from './src/BpkTooltip'; -export type BpkTooltipPortalProps = Props; +export type BpkTooltipProps = Props; -export default BpkTooltipPortal; +export default BpkTooltip; export { TOOLTIP_TYPES }; diff --git a/packages/bpk-component-tooltip/src/BpkTooltip-test.tsx b/packages/bpk-component-tooltip/src/BpkTooltip-test.tsx index 7db4020494..ed5ce54319 100644 --- a/packages/bpk-component-tooltip/src/BpkTooltip-test.tsx +++ b/packages/bpk-component-tooltip/src/BpkTooltip-test.tsx @@ -16,47 +16,48 @@ * limitations under the License. */ -import { render } from '@testing-library/react'; +import { render, screen } from '@testing-library/react'; +import '@testing-library/jest-dom'; import BpkTooltip from './BpkTooltip'; import { TOOLTIP_TYPES } from './constants'; describe('BpkTooltip', () => { it('should render correctly', () => { - const { asFragment } = render( - My tooltip content, + const target = (My tooltip target); + + render( + My tooltip content, ); - expect(asFragment()).toMatchSnapshot(); + expect(screen.getByText('My tooltip content')).toBeVisible(); }); it('should render correctly with type=dark', () => { - const { asFragment } = render( - + const target = (My tooltip target); + + const { container } = render( + My tooltip content , ); - - expect(asFragment()).toMatchSnapshot(); + + expect(screen.getByText('My tooltip content')).toBeVisible(); + expect(container.querySelector('.bpk-tooltip__inner--dark')).not.toBeNull(); + expect(container.querySelector('.bpk-tooltip__arrow--dark')).not.toBeNull(); }); it('should render correctly with "padded" attribute equal to false', () => { - const { asFragment } = render( - - My tooltip content - , - ); - - expect(asFragment()).toMatchSnapshot(); - }); + const target = (My tooltip target); - it('should render correctly with "className" attribute', () => { - const { asFragment } = render( - + const { container } = render( + My tooltip content , ); - - expect(asFragment()).toMatchSnapshot(); + + expect(screen.getByText('My tooltip content')).toBeVisible(); + expect(container.querySelector('.bpk-tooltip__inner--padded')).toBeNull(); }); + }); diff --git a/packages/bpk-component-tooltip/src/BpkTooltip.module.scss b/packages/bpk-component-tooltip/src/BpkTooltip.module.scss index d2a10fde81..d0d2109127 100644 --- a/packages/bpk-component-tooltip/src/BpkTooltip.module.scss +++ b/packages/bpk-component-tooltip/src/BpkTooltip.module.scss @@ -18,126 +18,42 @@ @use '../../unstable__bpk-mixins/tokens'; @use '../../unstable__bpk-mixins/layers'; +@use '../../unstable__bpk-mixins/radii'; +@use '../../unstable__bpk-mixins/shadows'; $arrow-size: tokens.bpk-spacing-md(); -$arrow-inset-margin: -1 * (tokens.$bpk-one-pixel-rem * 3); -$light-arrow-border-color: tokens.$bpk-line-day; $dark-background-color: tokens.$bpk-surface-contrast-day; -.bpk-tooltip-portal { +.bpk-tooltip--container { z-index: tokens.$bpk-zindex-tooltip; - - &--target { - display: inline-block; - } } .bpk-tooltip { - transition: - opacity tokens.$bpk-duration-sm ease-in-out, - transform tokens.$bpk-duration-sm ease-in-out; + transition: opacity tokens.$bpk-duration-sm ease-in-out; outline: 0; opacity: 1; + @include layers.bpk-layer; + &--appear { opacity: 0; - - .bpk-tooltip-portal[data-popper-placement='top'] - &:not(.bpk-tooltip--appear-active) { - transform: translate(0, -1rem); - transition: none; - } - - .bpk-tooltip-portal[data-popper-placement='right'] - &:not(.bpk-tooltip--appear-active) { - transform: translate(1rem, 0); - transition: none; - } - - .bpk-tooltip-portal[data-popper-placement='bottom'] - &:not(.bpk-tooltip--appear-active) { - transform: translate(0, 1rem); - transition: none; - } - - .bpk-tooltip-portal[data-popper-placement='left'] - &:not(.bpk-tooltip--appear-active) { - transform: translate(-1rem, 0); - transition: none; - } } &--appear-active { - transform: translate(0, 0); - transition: - opacity tokens.$bpk-duration-sm ease-in-out, - transform tokens.$bpk-duration-sm ease-in-out; opacity: 1; } &__arrow { - position: absolute; width: $arrow-size; height: $arrow-size; - transform: rotate(45deg); - border: tokens.$bpk-one-pixel-rem solid transparent; - background: inherit; - background-color: tokens.$bpk-surface-default-day; - - .bpk-tooltip-portal[data-popper-placement='top'] & { - bottom: $arrow-inset-margin; - border-right-color: $light-arrow-border-color; - border-bottom-color: $light-arrow-border-color; - } - - .bpk-tooltip-portal[data-popper-placement='right'] & { - left: $arrow-inset-margin; - border-bottom-color: $light-arrow-border-color; - border-left-color: $light-arrow-border-color; - } - - .bpk-tooltip-portal[data-popper-placement='bottom'] & { - top: $arrow-inset-margin; - border-top-color: $light-arrow-border-color; - border-left-color: $light-arrow-border-color; - } - - .bpk-tooltip-portal[data-popper-placement='left'] & { - right: $arrow-inset-margin; - border-top-color: $light-arrow-border-color; - border-right-color: $light-arrow-border-color; - } + fill: tokens.$bpk-surface-default-day; &--dark { - background-color: $dark-background-color; - - .bpk-tooltip-portal[data-popper-placement='top'] & { - border-right-color: $dark-background-color; - border-bottom-color: $dark-background-color; - } - - .bpk-tooltip-portal[data-popper-placement='right'] & { - border-bottom-color: $dark-background-color; - border-left-color: $dark-background-color; - } - - .bpk-tooltip-portal[data-popper-placement='bottom'] & { - border-top-color: $dark-background-color; - border-left-color: $dark-background-color; - } - - .bpk-tooltip-portal[data-popper-placement='left'] & { - border-top-color: $dark-background-color; - border-right-color: $dark-background-color; - } + fill: $dark-background-color; } } &__inner { - overflow: hidden; - - @include layers.bpk-layer; - &--padded { padding: tokens.bpk-spacing-md() tokens.bpk-spacing-base(); } diff --git a/packages/bpk-component-tooltip/src/BpkTooltip.tsx b/packages/bpk-component-tooltip/src/BpkTooltip.tsx index 63fceed3d1..7773138bd8 100644 --- a/packages/bpk-component-tooltip/src/BpkTooltip.tsx +++ b/packages/bpk-component-tooltip/src/BpkTooltip.tsx @@ -22,33 +22,104 @@ This is the component for the tooltip that is displayed to users. The actual component that developers create (i.e. the default export from this package) is BpkTooltipPortal. */ -import type { ReactNode } from 'react'; +import type { ReactNode, ReactElement } from 'react'; +import { cloneElement, useRef , useState } from 'react'; + +import { + arrow, + flip, + FloatingArrow, + offset, + shift, + useFloating, + useHover, + useInteractions, +} from '@floating-ui/react'; + +import { surfaceHighlightDay } from '@skyscanner/bpk-foundations-web/tokens/base.es6'; import { TransitionInitialMount, cssModules } from '../../bpk-react-utils'; import { ARROW_ID, TOOLTIP_TYPES } from './constants'; +import type { Placement } from '@floating-ui/react'; + + + import STYLES from './BpkTooltip.module.scss'; + const getClassName = cssModules(STYLES); -export type TooltipProps = { +// The stroke width is used to set the border width of the arrow. +const strokeWidth = 1; + +export type Props = { id: string; children: ReactNode | string; type?: (typeof TOOLTIP_TYPES)[keyof typeof TOOLTIP_TYPES]; padded?: boolean; - className?: string | null; + target: ReactElement; + ariaLabel: string; + hideOnTouchDevices?: boolean; + placement?: Placement; + isOpen?: boolean; +}; + +// This function is to ensure the arrow alignment when used on the top and bottom +// doesn't look clipped away from the tooltip. This is due to our use of box-shadows that makes it look floating away, +// so we need to compensate slightly to make it look as one. +const getArrowAlignment = (placement: Placement) => { + if (placement.includes('bottom')) { + return { bottom: '98%' }; + } if (placement.includes('top')) { + return { top: '98%' }; + } + return undefined; + }; const BpkTooltip = ({ + ariaLabel, children, - className = null, + hideOnTouchDevices = true, id, + isOpen = false, padded = true, + placement = 'bottom', + target, type = TOOLTIP_TYPES.light, ...rest -}: TooltipProps) => { - const classNames = getClassName('bpk-tooltip', className); +}: Props) => { + const [isOpenState, setIsOpenState] = useState(isOpen); + const arrowRef = useRef(null); + + const { context, floatingStyles, refs } = useFloating({ + open: isOpenState, + onOpenChange: setIsOpenState, + placement, + middleware: [ + offset(8), + flip({ crossAxis: true }), + shift(), + arrow({ element: arrowRef }), + ], + }); + + const hover = useHover(context, { + mouseOnly: true, + }); + + const { getFloatingProps, getReferenceProps } = useInteractions([hover]); + + const targetWithAccessibilityProps = cloneElement(target, { + ...getReferenceProps(), + ref: refs.setReference, + tabIndex: '0', + 'aria-label': ariaLabel, + }); + + const classNames = getClassName('bpk-tooltip'); const innerClassNames = getClassName( 'bpk-tooltip__inner', @@ -62,27 +133,44 @@ const BpkTooltip = ({ ); return ( - - - + <> + {targetWithAccessibilityProps} + {isOpenState && ( +
+ + + +
+ )} + ); }; diff --git a/packages/bpk-component-tooltip/src/BpkTooltipPortal-test.tsx b/packages/bpk-component-tooltip/src/BpkTooltipPortal-test.tsx deleted file mode 100644 index cf9f4bd056..0000000000 --- a/packages/bpk-component-tooltip/src/BpkTooltipPortal-test.tsx +++ /dev/null @@ -1,94 +0,0 @@ -/* - * Backpack - Skyscanner's Design System - * - * Copyright 2016 Skyscanner Ltd - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -import { render } from '@testing-library/react'; - -import { colorPanjin } from '@skyscanner/bpk-foundations-web/tokens/base.es6'; - -jest.mock('../../bpk-react-utils', () => { - const original = jest.requireActual('../../bpk-react-utils'); - - return { - ...original, - Portal: 'Portal', - }; -}); - -// eslint-disable-next-line import/first -import BpkTooltipPortal from './BpkTooltipPortal'; - -describe('BpkTooltipPortal', () => { - it('should render correctly', () => { - const { asFragment } = render( - target} - > - My tooltip content - , - ); - - expect(asFragment()).toMatchSnapshot(); - }); - - it('should render correctly with a custom portal className', () => { - const { asFragment } = render( - target} - portalClassName="my-custom-class" - > - My tooltip content - , - ); - - expect(asFragment()).toMatchSnapshot(); - }); - - it('should render correctly with a custom tooltip className', () => { - const { asFragment } = render( - target} - className="my-custom-class" - > - My tooltip content - , - ); - - expect(asFragment()).toMatchSnapshot(); - }); - - it('should render correctly with custom portal style', () => { - const { asFragment } = render( - target} - portalStyle={{ color: colorPanjin }} - > - My tooltip content - , - ); - - expect(asFragment()).toMatchSnapshot(); - }); -}); diff --git a/packages/bpk-component-tooltip/src/BpkTooltipPortal.tsx b/packages/bpk-component-tooltip/src/BpkTooltipPortal.tsx deleted file mode 100644 index 9580e59daa..0000000000 --- a/packages/bpk-component-tooltip/src/BpkTooltipPortal.tsx +++ /dev/null @@ -1,230 +0,0 @@ -/* - * Backpack - Skyscanner's Design System - * - * Copyright 2016 Skyscanner Ltd - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -import { cloneElement, Component } from 'react'; -import type { ReactNode, ReactElement } from 'react'; - -import { createPopper } from '@popperjs/core'; - -import { Portal, cssModules } from '../../bpk-react-utils'; - -import BpkTooltip from './BpkTooltip'; -import { TOOLTIP_TYPES } from './constants'; - -import type { TooltipProps } from './BpkTooltip'; - -import STYLES from './BpkTooltip.module.scss'; - -const getClassName = cssModules(STYLES); - -const hasTouchSupport = () => - !!( - typeof window !== 'undefined' && - ('ontouchstart' in window || navigator.maxTouchPoints > 0) - ); - -export type Props = TooltipProps & { - /** - * Tooltips are invisible to assistive technologies such as screen readers. - * To improve accessibility, `ariaLabel` is required to describe the content of the tooltip to assistive technologies. - * The label will be used on the `target` element, so any existing `aria-label` attached to `target` will be overridden. - */ - ariaLabel: string; - /** - * "target" should be a DOM element with a "ref" attached to it. - */ - target: ReactElement; - children: ReactNode | string; - placement?: 'top' | 'right' | 'bottom' | 'left' | 'auto'; - hideOnTouchDevices?: boolean; - portalStyle?: object; - portalClassName?: string; - renderTarget: null | (() => null | HTMLElement); - /** - * Please refer to the [documentation](https://popper.js.org/docs/v2/modifiers/) for the underlying positioning library "Popper.js". - * You can achieve various behaviours such as allowing the tooltip to overflow the viewport etc. - */ - popperModifiers?: object[]; -}; - -type State = { - isOpen: boolean; -}; - -class BpkTooltipPortal extends Component { - popper?: ReturnType | null; - - targetRef?: Element | null; - - static defaultProps = { - // Disabling as the rule doesn't work when types are defined in a different file - /* eslint-disable react/default-props-match-prop-types */ - className: null, - padded: true, - type: TOOLTIP_TYPES.light, - /* eslint-enable */ - placement: 'bottom', - hideOnTouchDevices: true, - portalStyle: null, - portalClassName: null, - renderTarget: null, - popperModifiers: null, - }; - - constructor(props: Props) { - super(props); - - this.state = { - isOpen: false, - }; - - this.popper = null; - this.targetRef = null; - } - - componentDidMount() { - if (this.targetRef) { - const ref = this.targetRef; - - ref.addEventListener('focusin', this.openTooltip); - ref.addEventListener('focusout', this.closeTooltip); - ref.addEventListener('mouseenter', this.openTooltip); - ref.addEventListener('mouseleave', this.closeTooltip); - } - } - - componentWillUnmount() { - if (this.targetRef) { - const ref = this.targetRef; - - ref.addEventListener('focusin', this.openTooltip); - ref.addEventListener('focusout', this.closeTooltip); - ref.removeEventListener('mouseenter', this.openTooltip); - ref.removeEventListener('mouseleave', this.closeTooltip); - } - } - - onOpen = ( - tooltipElement: HTMLElement, - targetElement?: HTMLElement | null | undefined, - ) => { - // The default modifiers for the popper - // Note that GPU acceleration should be disabled otherwise Popper will use `translate3d` - // which can cause blurriness in Safari and Chrome. - const stdModifiers = [ - { - name: 'computeStyles', - options: { - gpuAcceleration: false, - }, - }, - { - name: 'offset', - options: { - offset: [0, 8], - }, - }, - ]; - - this.popper = createPopper(targetElement as HTMLElement, tooltipElement, { - placement: this.props.placement, - modifiers: this.props.popperModifiers - ? [...this.props.popperModifiers, ...stdModifiers] - : stdModifiers, - }); - - this.popper.update(); - }; - - beforeClose = (done: () => void | null) => { - if (this.popper) { - this.popper.destroy(); - this.popper = null; - } - - done(); - }; - - openTooltip = () => { - this.setState({ - isOpen: true, - }); - }; - - closeTooltip = () => { - this.setState({ - isOpen: false, - }); - }; - - render() { - const { - ariaLabel, - children, - hideOnTouchDevices, - padded, - placement, - popperModifiers, - portalClassName, - portalStyle, - renderTarget, - target, - ...rest - } = this.props; - - const classNames = [getClassName('bpk-tooltip-portal')]; - const renderPortal = !hasTouchSupport() || !hideOnTouchDevices; - - const targetWithAccessibilityProps = cloneElement(target, { - tabIndex: '0', - 'aria-label': ariaLabel, - }); - - if (portalClassName) { - classNames.push(portalClassName); - } - - return ( - <> - {targetWithAccessibilityProps} - {renderPortal && ( - { - this.targetRef = targetRef; - }} - isOpen={this.state.isOpen} - onOpen={this.onOpen} - onClose={this.closeTooltip} - style={portalStyle} - renderTarget={renderTarget} - // TODO: className to be removed - // eslint-disable-next-line @skyscanner/rules/forbid-component-props - className={classNames.join(' ')} - > - - {children} - - - )} - - ); - } -} - -export default BpkTooltipPortal; diff --git a/packages/bpk-component-tooltip/src/__snapshots__/BpkTooltip-test.tsx.snap b/packages/bpk-component-tooltip/src/__snapshots__/BpkTooltip-test.tsx.snap deleted file mode 100644 index 570c61fc85..0000000000 --- a/packages/bpk-component-tooltip/src/__snapshots__/BpkTooltip-test.tsx.snap +++ /dev/null @@ -1,93 +0,0 @@ -// Jest Snapshot v1, https://goo.gl/fbAQLP - -exports[`BpkTooltip should render correctly 1`] = ` - - - -`; - -exports[`BpkTooltip should render correctly with "className" attribute 1`] = ` - - - -`; - -exports[`BpkTooltip should render correctly with "padded" attribute equal to false 1`] = ` - - - -`; - -exports[`BpkTooltip should render correctly with type=dark 1`] = ` - - - -`; diff --git a/packages/bpk-component-tooltip/src/__snapshots__/BpkTooltipPortal-test.tsx.snap b/packages/bpk-component-tooltip/src/__snapshots__/BpkTooltipPortal-test.tsx.snap deleted file mode 100644 index ced89df834..0000000000 --- a/packages/bpk-component-tooltip/src/__snapshots__/BpkTooltipPortal-test.tsx.snap +++ /dev/null @@ -1,138 +0,0 @@ -// Jest Snapshot v1, https://goo.gl/fbAQLP - -exports[`BpkTooltipPortal should render correctly 1`] = ` - -
- target -
- - - - -`; - -exports[`BpkTooltipPortal should render correctly with a custom portal className 1`] = ` - -
- target -
- - - - -`; - -exports[`BpkTooltipPortal should render correctly with a custom tooltip className 1`] = ` - -
- target -
- - - - -`; - -exports[`BpkTooltipPortal should render correctly with custom portal style 1`] = ` - -
- target -
- - - - -`; diff --git a/packages/bpk-component-tooltip/src/accessibility-test.tsx b/packages/bpk-component-tooltip/src/accessibility-test.tsx index c540cdcc33..4c6e7b66c0 100644 --- a/packages/bpk-component-tooltip/src/accessibility-test.tsx +++ b/packages/bpk-component-tooltip/src/accessibility-test.tsx @@ -16,7 +16,7 @@ * limitations under the License. */ -import { render } from '@testing-library/react'; +import { render, waitFor } from '@testing-library/react'; import { axe } from 'jest-axe'; import BpkTooltip from './BpkTooltip'; @@ -24,11 +24,14 @@ import BpkTooltip from './BpkTooltip'; describe('BpkTooltip accessibility tests', () => { it('should not have programmatically-detectable accessibility issues', async () => { const { container } = render( - + My tooltip target

} isOpen> My tooltip content , ); - const results = await axe(container); - expect(results).toHaveNoViolations(); + await waitFor(async () => { + const results = await axe(container); + expect(results).toHaveNoViolations(); + }) + }); }); From 9586a927da90550bd7c2584a0c9e1a28942c5748 Mon Sep 17 00:00:00 2001 From: Ollie Curtis <8831547+olliecurtis@users.noreply.github.com> Date: Mon, 3 Jun 2024 15:58:36 +0100 Subject: [PATCH 02/11] Formatting and adding visual testing example --- examples/bpk-component-tooltip/examples.tsx | 19 ++++++++++ examples/bpk-component-tooltip/stories.ts | 9 +++-- .../src/BpkTooltip-test.tsx | 38 ++++++++++++++----- .../bpk-component-tooltip/src/BpkTooltip.tsx | 13 +++---- .../src/accessibility-test.tsx | 10 +++-- 5 files changed, 65 insertions(+), 24 deletions(-) diff --git a/examples/bpk-component-tooltip/examples.tsx b/examples/bpk-component-tooltip/examples.tsx index 9887f9f7ce..acee166852 100644 --- a/examples/bpk-component-tooltip/examples.tsx +++ b/examples/bpk-component-tooltip/examples.tsx @@ -189,6 +189,24 @@ const FocusExample = () => { ); }; +const VisualTestExample = () => { + const target = useRef(null); + + return ( +
+ YUL} + isOpen + > + Montréal-Trudeau International Airport + +
+ ); +}; + + export { DefaultExample, DarkExample, @@ -196,4 +214,5 @@ export { NoPaddingExample, LinkExample, FocusExample, + VisualTestExample }; diff --git a/examples/bpk-component-tooltip/stories.ts b/examples/bpk-component-tooltip/stories.ts index ee4f0ba495..c62e3d78b1 100644 --- a/examples/bpk-component-tooltip/stories.ts +++ b/examples/bpk-component-tooltip/stories.ts @@ -25,6 +25,7 @@ import { NoPaddingExample, LinkExample, FocusExample, + VisualTestExample } from './examples'; export default { @@ -35,9 +36,11 @@ export default { export const Default = DefaultExample; export const Dark = DarkExample; export const OnTheSide = SideExample; - export const WithoutPadding = NoPaddingExample; - export const OnALink = LinkExample; - export const Focus = FocusExample; +export const VisualTest = VisualTestExample; +export const VisualTestWithZoom = VisualTest.bind({}); +VisualTestWithZoom.args = { + zoomEnabled: true +}; diff --git a/packages/bpk-component-tooltip/src/BpkTooltip-test.tsx b/packages/bpk-component-tooltip/src/BpkTooltip-test.tsx index ed5ce54319..34226024cc 100644 --- a/packages/bpk-component-tooltip/src/BpkTooltip-test.tsx +++ b/packages/bpk-component-tooltip/src/BpkTooltip-test.tsx @@ -24,40 +24,58 @@ import { TOOLTIP_TYPES } from './constants'; describe('BpkTooltip', () => { it('should render correctly', () => { - const target = (My tooltip target); + const target = My tooltip target; render( - My tooltip content, + + My tooltip content + , ); expect(screen.getByText('My tooltip content')).toBeVisible(); }); it('should render correctly with type=dark', () => { - const target = (My tooltip target); + const target = My tooltip target; const { container } = render( - + My tooltip content , ); - + expect(screen.getByText('My tooltip content')).toBeVisible(); expect(container.querySelector('.bpk-tooltip__inner--dark')).not.toBeNull(); expect(container.querySelector('.bpk-tooltip__arrow--dark')).not.toBeNull(); }); it('should render correctly with "padded" attribute equal to false', () => { - const target = (My tooltip target); + const target = My tooltip target; - const { container } = render( - + const { container } = render( + My tooltip content , ); - + expect(screen.getByText('My tooltip content')).toBeVisible(); expect(container.querySelector('.bpk-tooltip__inner--padded')).toBeNull(); }); - }); diff --git a/packages/bpk-component-tooltip/src/BpkTooltip.tsx b/packages/bpk-component-tooltip/src/BpkTooltip.tsx index 7773138bd8..f9ea5a9fe4 100644 --- a/packages/bpk-component-tooltip/src/BpkTooltip.tsx +++ b/packages/bpk-component-tooltip/src/BpkTooltip.tsx @@ -23,7 +23,7 @@ The actual component that developers create (i.e. the default export from this p */ import type { ReactNode, ReactElement } from 'react'; -import { cloneElement, useRef , useState } from 'react'; +import { cloneElement, useRef, useState } from 'react'; import { arrow, @@ -44,11 +44,8 @@ import { ARROW_ID, TOOLTIP_TYPES } from './constants'; import type { Placement } from '@floating-ui/react'; - - import STYLES from './BpkTooltip.module.scss'; - const getClassName = cssModules(STYLES); // The stroke width is used to set the border width of the arrow. @@ -72,11 +69,11 @@ export type Props = { const getArrowAlignment = (placement: Placement) => { if (placement.includes('bottom')) { return { bottom: '98%' }; - } if (placement.includes('top')) { + } + if (placement.includes('top')) { return { top: '98%' }; - } - return undefined; - + } + return undefined; }; const BpkTooltip = ({ diff --git a/packages/bpk-component-tooltip/src/accessibility-test.tsx b/packages/bpk-component-tooltip/src/accessibility-test.tsx index 4c6e7b66c0..31f8ce7b64 100644 --- a/packages/bpk-component-tooltip/src/accessibility-test.tsx +++ b/packages/bpk-component-tooltip/src/accessibility-test.tsx @@ -24,14 +24,18 @@ import BpkTooltip from './BpkTooltip'; describe('BpkTooltip accessibility tests', () => { it('should not have programmatically-detectable accessibility issues', async () => { const { container } = render( - My tooltip target

} isOpen> + My tooltip target

} + isOpen + > My tooltip content , ); await waitFor(async () => { const results = await axe(container); expect(results).toHaveNoViolations(); - }) - + }); }); }); From 4f82791acc27c0a2125b6a711c0b1e18cf10962d Mon Sep 17 00:00:00 2001 From: Ollie Curtis <8831547+olliecurtis@users.noreply.github.com> Date: Mon, 3 Jun 2024 16:08:27 +0100 Subject: [PATCH 03/11] Returned comment to props for documentation --- packages/bpk-component-tooltip/src/BpkTooltip.tsx | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/packages/bpk-component-tooltip/src/BpkTooltip.tsx b/packages/bpk-component-tooltip/src/BpkTooltip.tsx index f9ea5a9fe4..d995f3722f 100644 --- a/packages/bpk-component-tooltip/src/BpkTooltip.tsx +++ b/packages/bpk-component-tooltip/src/BpkTooltip.tsx @@ -52,12 +52,20 @@ const getClassName = cssModules(STYLES); const strokeWidth = 1; export type Props = { + /** + * Tooltips are invisible to assistive technologies such as screen readers. + * To improve accessibility, `ariaLabel` is required to describe the content of the tooltip to assistive technologies. + * The label will be used on the `target` element, so any existing `aria-label` attached to `target` will be overridden. + */ + ariaLabel: string; + /** + * "target" should be a DOM element with a "ref" attached to it. + */ + target: ReactElement; id: string; children: ReactNode | string; type?: (typeof TOOLTIP_TYPES)[keyof typeof TOOLTIP_TYPES]; padded?: boolean; - target: ReactElement; - ariaLabel: string; hideOnTouchDevices?: boolean; placement?: Placement; isOpen?: boolean; From 734a87fbb2a473246f0456c7e05e508b45cc500f Mon Sep 17 00:00:00 2001 From: Ollie Curtis <8831547+olliecurtis@users.noreply.github.com> Date: Mon, 3 Jun 2024 17:11:09 +0100 Subject: [PATCH 04/11] Fix type failures and fix unused prop --- .../bpk-component-tooltip/{stories.ts => stories.tsx} | 10 ++++++---- packages/bpk-component-tooltip/src/BpkTooltip.tsx | 10 ++++++++-- 2 files changed, 14 insertions(+), 6 deletions(-) rename examples/bpk-component-tooltip/{stories.ts => stories.tsx} (90%) diff --git a/examples/bpk-component-tooltip/stories.ts b/examples/bpk-component-tooltip/stories.tsx similarity index 90% rename from examples/bpk-component-tooltip/stories.ts rename to examples/bpk-component-tooltip/stories.tsx index c62e3d78b1..38919bf145 100644 --- a/examples/bpk-component-tooltip/stories.ts +++ b/examples/bpk-component-tooltip/stories.tsx @@ -25,7 +25,7 @@ import { NoPaddingExample, LinkExample, FocusExample, - VisualTestExample + VisualTestExample, } from './examples'; export default { @@ -40,7 +40,9 @@ export const WithoutPadding = NoPaddingExample; export const OnALink = LinkExample; export const Focus = FocusExample; export const VisualTest = VisualTestExample; -export const VisualTestWithZoom = VisualTest.bind({}); -VisualTestWithZoom.args = { - zoomEnabled: true +export const VisualTestWithZoom = { + render: VisualTest, + args: { + zoomEnabled: true, + }, }; diff --git a/packages/bpk-component-tooltip/src/BpkTooltip.tsx b/packages/bpk-component-tooltip/src/BpkTooltip.tsx index d995f3722f..d0b906970e 100644 --- a/packages/bpk-component-tooltip/src/BpkTooltip.tsx +++ b/packages/bpk-component-tooltip/src/BpkTooltip.tsx @@ -52,7 +52,7 @@ const getClassName = cssModules(STYLES); const strokeWidth = 1; export type Props = { - /** + /** * Tooltips are invisible to assistive technologies such as screen readers. * To improve accessibility, `ariaLabel` is required to describe the content of the tooltip to assistive technologies. * The label will be used on the `target` element, so any existing `aria-label` attached to `target` will be overridden. @@ -84,6 +84,12 @@ const getArrowAlignment = (placement: Placement) => { return undefined; }; +const hasTouchSupport = () => + !!( + typeof window !== 'undefined' && + ('ontouchstart' in window || navigator.maxTouchPoints > 0) + ); + const BpkTooltip = ({ ariaLabel, children, @@ -112,7 +118,7 @@ const BpkTooltip = ({ }); const hover = useHover(context, { - mouseOnly: true, + mouseOnly: !hasTouchSupport() || !hideOnTouchDevices, }); const { getFloatingProps, getReferenceProps } = useInteractions([hover]); From 1a61a8f04cdd1b9e3ba4cc2938c3ce370b5f5f29 Mon Sep 17 00:00:00 2001 From: Ollie Curtis <8831547+olliecurtis@users.noreply.github.com> Date: Mon, 3 Jun 2024 17:36:15 +0100 Subject: [PATCH 05/11] Fixing a11y voiceover issues --- examples/bpk-component-tooltip/examples.tsx | 33 +++++++------------ .../bpk-component-tooltip/src/BpkTooltip.tsx | 1 - .../src/accessibility-test.tsx | 1 - 3 files changed, 11 insertions(+), 24 deletions(-) diff --git a/examples/bpk-component-tooltip/examples.tsx b/examples/bpk-component-tooltip/examples.tsx index acee166852..0850839b9a 100644 --- a/examples/bpk-component-tooltip/examples.tsx +++ b/examples/bpk-component-tooltip/examples.tsx @@ -16,11 +16,7 @@ * limitations under the License. */ -import { useRef, forwardRef } from 'react'; - -import { - colorMonteverde, -} from '@skyscanner/bpk-foundations-web/tokens/base.es6'; +import { useRef } from 'react'; import BpkText, { TEXT_STYLES } from '../../packages/bpk-component-text'; import BpkTooltip, { @@ -39,11 +35,6 @@ const Heading = withDefaultProps(BpkText, { tagName: 'h4', }); -const HeadingComponent = forwardRef((props, ref) => ( -
- {props.children} -
-)); const DefaultExample = () => { const target = useRef(null); @@ -53,7 +44,7 @@ const DefaultExample = () => { YUL} + target={YUL} > Montréal-Trudeau International Airport @@ -70,7 +61,7 @@ const DarkExample = () => { ariaLabel="Edinburgh Airport" type={TOOLTIP_TYPES.dark} id="my-tooltip" - target={EDI} + target={EDI} > Edinburgh Airport @@ -86,7 +77,7 @@ const SideExample = () => { DAR} + target={DAR} placement="right" > Julius Nyerere International Airport, Dar es Salaam @@ -103,13 +94,12 @@ const NoPaddingExample = () => { SIN} + target={SIN} padded={false} >
{ One} + target={One} > Should be focused on first Two} + target={Two} > Should be focused on second Three} + target={Three} > Should be focused on third @@ -181,7 +171,7 @@ const FocusExample = () => { Five} + target={Five} > Should be focused on fifth @@ -197,7 +187,7 @@ const VisualTestExample = () => { YUL} + target={YUL} isOpen > Montréal-Trudeau International Airport @@ -206,7 +196,6 @@ const VisualTestExample = () => { ); }; - export { DefaultExample, DarkExample, @@ -214,5 +203,5 @@ export { NoPaddingExample, LinkExample, FocusExample, - VisualTestExample + VisualTestExample, }; diff --git a/packages/bpk-component-tooltip/src/BpkTooltip.tsx b/packages/bpk-component-tooltip/src/BpkTooltip.tsx index d0b906970e..ff9b002e88 100644 --- a/packages/bpk-component-tooltip/src/BpkTooltip.tsx +++ b/packages/bpk-component-tooltip/src/BpkTooltip.tsx @@ -163,7 +163,6 @@ const BpkTooltip = ({ tabIndex={-1} role="dialog" className={classNames} - aria-label={ariaLabel} {...rest} > { id="my-popover" ariaLabel="Tooltip" target={

My tooltip target

} - isOpen > My tooltip content , From dd7b2ecb3850e59f9bc756f4a898023f758a882d Mon Sep 17 00:00:00 2001 From: Ollie Curtis <8831547+olliecurtis@users.noreply.github.com> Date: Tue, 4 Jun 2024 10:02:03 +0100 Subject: [PATCH 06/11] Testing adding timeout for stories to render --- examples/bpk-component-tooltip/stories.tsx | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/examples/bpk-component-tooltip/stories.tsx b/examples/bpk-component-tooltip/stories.tsx index 38919bf145..2733e71974 100644 --- a/examples/bpk-component-tooltip/stories.tsx +++ b/examples/bpk-component-tooltip/stories.tsx @@ -39,10 +39,24 @@ export const OnTheSide = SideExample; export const WithoutPadding = NoPaddingExample; export const OnALink = LinkExample; export const Focus = FocusExample; -export const VisualTest = VisualTestExample; + +const VisualExample = VisualTestExample; +export const VisualTest = { + render: VisualExample, + parameters: { + percy: { + waitForTimeout: 10000 + } + } +}; export const VisualTestWithZoom = { - render: VisualTest, + render: VisualExample, args: { zoomEnabled: true, }, + parameters: { + percy: { + waitForTimeout: 10000 + } + } }; From 1ac716efd552a7cce48ef0d17c2f77ebef769a84 Mon Sep 17 00:00:00 2001 From: Ollie Curtis <8831547+olliecurtis@users.noreply.github.com> Date: Tue, 4 Jun 2024 18:15:34 +0100 Subject: [PATCH 07/11] Fixing a11y voiceover issues and attaching to references in stories --- examples/bpk-component-tooltip/examples.tsx | 53 ++----------------- examples/bpk-component-tooltip/stories.tsx | 2 - .../bpk-component-tooltip/src/BpkTooltip.tsx | 12 +++-- 3 files changed, 14 insertions(+), 53 deletions(-) diff --git a/examples/bpk-component-tooltip/examples.tsx b/examples/bpk-component-tooltip/examples.tsx index 0850839b9a..86dc18281f 100644 --- a/examples/bpk-component-tooltip/examples.tsx +++ b/examples/bpk-component-tooltip/examples.tsx @@ -44,7 +44,7 @@ const DefaultExample = () => { YUL} + target={
YUL
} > Montréal-Trudeau International Airport @@ -61,7 +61,7 @@ const DarkExample = () => { ariaLabel="Edinburgh Airport" type={TOOLTIP_TYPES.dark} id="my-tooltip" - target={EDI} + target={
EDI
} > Edinburgh Airport @@ -77,7 +77,7 @@ const SideExample = () => { DAR} + target={
DAR
} placement="right" > Julius Nyerere International Airport, Dar es Salaam @@ -94,7 +94,7 @@ const NoPaddingExample = () => { SIN} + target={
SIN
} padded={false} >
{ ); }; -const FocusExample = () => { - const targetRef1 = useRef(null); - const targetRef2 = useRef(null); - const targetRef3 = useRef(null); - const targetRef5 = useRef(null); - - return ( -
- One} - > - Should be focused on first - - Two} - > - Should be focused on second - - Three} - > - Should be focused on third - - - Five} - > - Should be focused on fifth - -
- ); -}; const VisualTestExample = () => { const target = useRef(null); @@ -187,7 +145,7 @@ const VisualTestExample = () => { YUL} + target={
YUL
} isOpen > Montréal-Trudeau International Airport @@ -202,6 +160,5 @@ export { SideExample, NoPaddingExample, LinkExample, - FocusExample, VisualTestExample, }; diff --git a/examples/bpk-component-tooltip/stories.tsx b/examples/bpk-component-tooltip/stories.tsx index 2733e71974..b2e5805b2e 100644 --- a/examples/bpk-component-tooltip/stories.tsx +++ b/examples/bpk-component-tooltip/stories.tsx @@ -24,7 +24,6 @@ import { SideExample, NoPaddingExample, LinkExample, - FocusExample, VisualTestExample, } from './examples'; @@ -38,7 +37,6 @@ export const Dark = DarkExample; export const OnTheSide = SideExample; export const WithoutPadding = NoPaddingExample; export const OnALink = LinkExample; -export const Focus = FocusExample; const VisualExample = VisualTestExample; export const VisualTest = { diff --git a/packages/bpk-component-tooltip/src/BpkTooltip.tsx b/packages/bpk-component-tooltip/src/BpkTooltip.tsx index ff9b002e88..3955e66797 100644 --- a/packages/bpk-component-tooltip/src/BpkTooltip.tsx +++ b/packages/bpk-component-tooltip/src/BpkTooltip.tsx @@ -34,6 +34,7 @@ import { useFloating, useHover, useInteractions, + useRole, } from '@floating-ui/react'; import { surfaceHighlightDay } from '@skyscanner/bpk-foundations-web/tokens/base.es6'; @@ -120,14 +121,19 @@ const BpkTooltip = ({ const hover = useHover(context, { mouseOnly: !hasTouchSupport() || !hideOnTouchDevices, }); + const role = useRole(context, { role: 'tooltip' }); - const { getFloatingProps, getReferenceProps } = useInteractions([hover]); + const { getFloatingProps, getReferenceProps } = useInteractions([ + hover, + role, + ]); const targetWithAccessibilityProps = cloneElement(target, { - ...getReferenceProps(), - ref: refs.setReference, tabIndex: '0', 'aria-label': ariaLabel, + role: 'tooltip', + ref: refs.setReference, + ...getReferenceProps(), }); const classNames = getClassName('bpk-tooltip'); From 7b04057ce41c82314e8c99b3e50d58562a354c89 Mon Sep 17 00:00:00 2001 From: Ollie Curtis <8831547+olliecurtis@users.noreply.github.com> Date: Mon, 17 Jun 2024 11:29:55 +0100 Subject: [PATCH 08/11] [ARGG-1157]: Update tooltip to attach to body outside of target elements --- .../src/BpkTooltip-test.tsx | 24 ++++--- .../bpk-component-tooltip/src/BpkTooltip.tsx | 63 ++++++++++--------- 2 files changed, 49 insertions(+), 38 deletions(-) diff --git a/packages/bpk-component-tooltip/src/BpkTooltip-test.tsx b/packages/bpk-component-tooltip/src/BpkTooltip-test.tsx index 34226024cc..30f17e09f1 100644 --- a/packages/bpk-component-tooltip/src/BpkTooltip-test.tsx +++ b/packages/bpk-component-tooltip/src/BpkTooltip-test.tsx @@ -16,13 +16,15 @@ * limitations under the License. */ -import { render, screen } from '@testing-library/react'; +import { render, screen, act } from '@testing-library/react'; import '@testing-library/jest-dom'; +import userEvent from '@testing-library/user-event'; import BpkTooltip from './BpkTooltip'; import { TOOLTIP_TYPES } from './constants'; describe('BpkTooltip', () => { + it('should render correctly', () => { const target = My tooltip target; @@ -40,10 +42,10 @@ describe('BpkTooltip', () => { expect(screen.getByText('My tooltip content')).toBeVisible(); }); - it('should render correctly with type=dark', () => { + it('should render correctly with type=dark', async () => { const target = My tooltip target; - const { container } = render( + render( { , ); + await act(async () => { + await userEvent.hover(screen.getByText('My tooltip target')); + }); expect(screen.getByText('My tooltip content')).toBeVisible(); - expect(container.querySelector('.bpk-tooltip__inner--dark')).not.toBeNull(); - expect(container.querySelector('.bpk-tooltip__arrow--dark')).not.toBeNull(); + expect(screen.getByText('My tooltip content').className).toContain('bpk-tooltip__inner--dark'); }); - it('should render correctly with "padded" attribute equal to false', () => { + it('should render correctly with "padded" attribute equal to false', async () => { const target = My tooltip target; - const { container } = render( + render( { , ); + await act(async () => { + await userEvent.hover(screen.getByText('My tooltip target')); + }); + expect(screen.getByText('My tooltip content')).toBeVisible(); - expect(container.querySelector('.bpk-tooltip__inner--padded')).toBeNull(); + expect(screen.getByText('My tooltip content').className).not.toContain('bpk-tooltip__inner--padded'); }); }); diff --git a/packages/bpk-component-tooltip/src/BpkTooltip.tsx b/packages/bpk-component-tooltip/src/BpkTooltip.tsx index 3955e66797..d18a805c13 100644 --- a/packages/bpk-component-tooltip/src/BpkTooltip.tsx +++ b/packages/bpk-component-tooltip/src/BpkTooltip.tsx @@ -29,6 +29,7 @@ import { arrow, flip, FloatingArrow, + FloatingPortal, offset, shift, useFloating, @@ -153,38 +154,40 @@ const BpkTooltip = ({ <> {targetWithAccessibilityProps} {isOpenState && ( -
- +
- - -
+ + +
+ )} ); From a486065e835da3cf870bcc88a5231ada1e3e91d2 Mon Sep 17 00:00:00 2001 From: Ollie Curtis <8831547+olliecurtis@users.noreply.github.com> Date: Thu, 27 Jun 2024 16:46:09 +0100 Subject: [PATCH 09/11] Remove the autoflip to avoid problems and learnings from the popover --- .../bpk-component-tooltip/src/BpkTooltip.tsx | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/packages/bpk-component-tooltip/src/BpkTooltip.tsx b/packages/bpk-component-tooltip/src/BpkTooltip.tsx index d18a805c13..08e025a2ce 100644 --- a/packages/bpk-component-tooltip/src/BpkTooltip.tsx +++ b/packages/bpk-component-tooltip/src/BpkTooltip.tsx @@ -23,11 +23,10 @@ The actual component that developers create (i.e. the default export from this p */ import type { ReactNode, ReactElement } from 'react'; -import { cloneElement, useRef, useState } from 'react'; +import { cloneElement, useRef, useState, useEffect } from 'react'; import { arrow, - flip, FloatingArrow, FloatingPortal, offset, @@ -107,16 +106,17 @@ const BpkTooltip = ({ const [isOpenState, setIsOpenState] = useState(isOpen); const arrowRef = useRef(null); + useEffect(() => { + if (!isOpen) { + setIsOpenState(false); + } + }, [isOpen]); + const { context, floatingStyles, refs } = useFloating({ open: isOpenState, onOpenChange: setIsOpenState, placement, - middleware: [ - offset(8), - flip({ crossAxis: true }), - shift(), - arrow({ element: arrowRef }), - ], + middleware: [offset(8), shift(), arrow({ element: arrowRef })], }); const hover = useHover(context, { From 10bb8e556374aeb377e9a484bba2aedbf2725af5 Mon Sep 17 00:00:00 2001 From: Ollie Curtis <8831547+olliecurtis@users.noreply.github.com> Date: Tue, 8 Apr 2025 10:16:16 +0100 Subject: [PATCH 10/11] PR feedback addressing: Remove comment about ref and fix act warnings in tests --- examples/bpk-component-tooltip/examples.tsx | 223 +++++++++--------- .../src/BpkTooltip-test.tsx | 26 +- .../bpk-component-tooltip/src/BpkTooltip.tsx | 2 +- 3 files changed, 124 insertions(+), 127 deletions(-) diff --git a/examples/bpk-component-tooltip/examples.tsx b/examples/bpk-component-tooltip/examples.tsx index 86dc18281f..7aae8cdae6 100644 --- a/examples/bpk-component-tooltip/examples.tsx +++ b/examples/bpk-component-tooltip/examples.tsx @@ -35,124 +35,117 @@ const Heading = withDefaultProps(BpkText, { tagName: 'h4', }); - -const DefaultExample = () => { - const target = useRef(null); - - return ( -
- YUL
} - > - Montréal-Trudeau International Airport - -
- ); -}; - -const DarkExample = () => { - const target = useRef(null); - - return ( -
- EDI
} - > - Edinburgh Airport - -
- ); -}; - -const SideExample = () => { - const target = useRef(null); - - return ( -
- DAR
} - placement="right" - > - Julius Nyerere International Airport, Dar es Salaam - - - ); -}; - -const NoPaddingExample = () => { - const target = useRef(null); - - return ( -
- SIN
} - padded={false} +const DefaultExample = () => ( +
+ + YUL +
+ } + > + Montréal-Trudeau International Airport + + +); + +const DarkExample = () => ( +
+ + EDI +
+ } + > + Edinburgh Airport + + +); + +const SideExample = () => ( +
+ + DAR +
+ } + placement="right" + > + Julius Nyerere International Airport, Dar es Salaam + + +); + +const NoPaddingExample = () => ( +
+ + SIN +
+ } + padded={false} + > +
-
+ +
+); + +const LinkExample = () => ( +
+ - Singapore Changi Airport + Hotels + + } + > + We do hotels too! + +
+); + +const VisualTestExample = () => ( +
+ + YUL
- -
- ); -}; - -const LinkExample = () => { - const target = useRef(null); - - return ( -
- - Hotels - - } - > - We do hotels too! - -
- ); -}; - - -const VisualTestExample = () => { - const target = useRef(null); - - return ( -
- YUL
} - isOpen - > - Montréal-Trudeau International Airport - - - ); -}; + } + isOpen + > + Montréal-Trudeau International Airport + + +); export { DefaultExample, diff --git a/packages/bpk-component-tooltip/src/BpkTooltip-test.tsx b/packages/bpk-component-tooltip/src/BpkTooltip-test.tsx index 30f17e09f1..644440d91e 100644 --- a/packages/bpk-component-tooltip/src/BpkTooltip-test.tsx +++ b/packages/bpk-component-tooltip/src/BpkTooltip-test.tsx @@ -16,7 +16,7 @@ * limitations under the License. */ -import { render, screen, act } from '@testing-library/react'; +import { render, screen, waitFor } from '@testing-library/react'; import '@testing-library/jest-dom'; import userEvent from '@testing-library/user-event'; @@ -24,7 +24,6 @@ import BpkTooltip from './BpkTooltip'; import { TOOLTIP_TYPES } from './constants'; describe('BpkTooltip', () => { - it('should render correctly', () => { const target = My tooltip target; @@ -57,11 +56,14 @@ describe('BpkTooltip', () => { , ); - await act(async () => { - await userEvent.hover(screen.getByText('My tooltip target')); + await userEvent.hover(screen.getByText('My tooltip target')); + + await waitFor(() => { + expect(screen.getByText('My tooltip content')).toBeVisible(); + expect(screen.getByText('My tooltip content').className).toContain( + 'bpk-tooltip__inner--dark', + ); }); - expect(screen.getByText('My tooltip content')).toBeVisible(); - expect(screen.getByText('My tooltip content').className).toContain('bpk-tooltip__inner--dark'); }); it('should render correctly with "padded" attribute equal to false', async () => { @@ -79,11 +81,13 @@ describe('BpkTooltip', () => { , ); - await act(async () => { - await userEvent.hover(screen.getByText('My tooltip target')); - }); + await userEvent.hover(screen.getByText('My tooltip target')); - expect(screen.getByText('My tooltip content')).toBeVisible(); - expect(screen.getByText('My tooltip content').className).not.toContain('bpk-tooltip__inner--padded'); + await waitFor(() => { + expect(screen.getByText('My tooltip content')).toBeVisible(); + expect(screen.getByText('My tooltip content').className).not.toContain( + 'bpk-tooltip__inner--padded', + ); + }); }); }); diff --git a/packages/bpk-component-tooltip/src/BpkTooltip.tsx b/packages/bpk-component-tooltip/src/BpkTooltip.tsx index 08e025a2ce..bf28ceb076 100644 --- a/packages/bpk-component-tooltip/src/BpkTooltip.tsx +++ b/packages/bpk-component-tooltip/src/BpkTooltip.tsx @@ -60,7 +60,7 @@ export type Props = { */ ariaLabel: string; /** - * "target" should be a DOM element with a "ref" attached to it. + * "target" should be a DOM element. */ target: ReactElement; id: string; From 38ac538154c37ca48babd2f2f37aa84d35991a51 Mon Sep 17 00:00:00 2001 From: Ollie Curtis <8831547+olliecurtis@users.noreply.github.com> Date: Tue, 8 Apr 2025 22:57:01 +0100 Subject: [PATCH 11/11] Remove unused useRef import --- examples/bpk-component-tooltip/examples.tsx | 2 -- 1 file changed, 2 deletions(-) diff --git a/examples/bpk-component-tooltip/examples.tsx b/examples/bpk-component-tooltip/examples.tsx index 7aae8cdae6..2137f05b2c 100644 --- a/examples/bpk-component-tooltip/examples.tsx +++ b/examples/bpk-component-tooltip/examples.tsx @@ -16,8 +16,6 @@ * limitations under the License. */ -import { useRef } from 'react'; - import BpkText, { TEXT_STYLES } from '../../packages/bpk-component-text'; import BpkTooltip, { TOOLTIP_TYPES,