From 541e136a1478cd315893ed1d48725e660af9d11d Mon Sep 17 00:00:00 2001
From: metalix2 <matthewr@skyscanner.net>
Date: Thu, 17 Oct 2024 19:28:54 +0100
Subject: [PATCH 01/11] Fix native event handling for the Slider
---
.../bpk-component-slider/src/BpkSlider.tsx | 64 +++++++++++++++----
packages/package-lock.json | 15 -----
packages/package.json | 1 -
3 files changed, 53 insertions(+), 27 deletions(-)
diff --git a/packages/bpk-component-slider/src/BpkSlider.tsx b/packages/bpk-component-slider/src/BpkSlider.tsx
index b56ba9dff6..12a9d98ac2 100644
--- a/packages/bpk-component-slider/src/BpkSlider.tsx
+++ b/packages/bpk-component-slider/src/BpkSlider.tsx
@@ -20,11 +20,11 @@ import {
useRef,
useEffect,
type ComponentPropsWithRef,
+ type RefObject,
} from 'react';
import { useComposedRefs } from '@radix-ui/react-compose-refs';
import * as Slider from '@radix-ui/react-slider';
-import { usePrevious } from '@radix-ui/react-use-previous';
import { cssModules, isRTL, setNativeValue } from '../../bpk-react-utils';
@@ -72,6 +72,8 @@ const BpkSlider = ({
}
};
+ const thumbRefs = [useRef(null), useRef(null)];
+
const handleOnChange = (sliderValues: number[]) => {
processSliderValues(sliderValues, onChange);
};
@@ -103,11 +105,16 @@ const BpkSlider = ({
aria-valuetext={ariaValuetext ? ariaValuetext[index] : val.toString()}
className={getClassName('bpk-slider__thumb')}
aria-valuenow={currentValue[index]}
+ ref={thumbRefs[index]}
asChild
>
{/* custom thumb with child input */}
<span>
- <BubbleInput value={currentValue[index]} {...(inputProps && inputProps[index])} />
+ <BubbleInput
+ value={currentValue[index]}
+ thumbRef={thumbRefs[index]}
+ {...(inputProps && inputProps[index])}
+ />
</span>
</Slider.Thumb>
))}
@@ -117,20 +124,55 @@ const BpkSlider = ({
// Work around until radix-ui/react-slider is updated to accept an inputRef prop https://github.com/radix-ui/primitives/pull/3033
const BubbleInput = forwardRef(
- (props: ComponentPropsWithRef<'input'>, forwardedRef) => {
- const { value, ...inputProps } = props;
+ (
+ props: ComponentPropsWithRef<'input'> & {
+ thumbRef: RefObject<HTMLElement>;
+ },
+ forwardedRef,
+ ) => {
+ const { thumbRef, value, ...inputProps } = props;
const ref = useRef<HTMLInputElement>();
const composedRefs = useComposedRefs(forwardedRef, ref);
- const prevValue = usePrevious(value);
-
- // Bubble value change to parents (e.g form change event)
+ // Provide native behaviour that the input range type provides.
useEffect(() => {
- const input = ref.current!;
- if (prevValue !== value) {
- setNativeValue(input, `${value}`);
+ const thumb = thumbRef.current;
+ const input = ref.current;
+ if (thumb) {
+ const interactionEndHandler = (prevValue: number) => {
+ if (input) {
+ const newValue = parseFloat(input.getAttribute('value') as string);
+ if (prevValue !== newValue) {
+ setNativeValue(input, newValue);
+ }
+ }
+ };
+
+ const focusInHandler = () => {
+ // Controller needed as we may click more than once when in focus. On focusout we can then abort the event listeners
+ const controller = new AbortController();
+ thumb.addEventListener('focusout', () => controller.abort(), {
+ once: true,
+ });
+ // needed on document as users can drag the thumb while out of the thumb elements mouse area
+ document.addEventListener('click', () => interactionEndHandler(value as number), {
+ signal: controller.signal,
+ });
+ thumb.addEventListener('keyup', () => interactionEndHandler(value as number), {
+ signal: controller.signal,
+ });
+ };
+
+ thumb.addEventListener('focusin', focusInHandler);
+
+ return () => {
+ if (thumb) {
+ thumb.removeEventListener('focusin', focusInHandler);
+ }
+ };
}
- }, [prevValue, value]);
+ return () => {};
+ }, [thumbRef, ref, value]);
/**
* We purposefully do not use `type="hidden"` here otherwise forms that
diff --git a/packages/package-lock.json b/packages/package-lock.json
index b072af78cc..d1fcd5e13f 100644
--- a/packages/package-lock.json
+++ b/packages/package-lock.json
@@ -13,7 +13,6 @@
"@popperjs/core": "^2.11.8",
"@radix-ui/react-compose-refs": "^1.1.0",
"@radix-ui/react-slider": "^1.1.2",
- "@radix-ui/react-use-previous": "^1.1.0",
"@react-google-maps/api": "^2.19.3",
"@skyscanner/bpk-foundations-web": "^18.1.0",
"@skyscanner/bpk-svgs": "^19.3.0",
@@ -424,20 +423,6 @@
}
}
},
- "node_modules/@radix-ui/react-use-previous": {
- "version": "1.1.0",
- "resolved": "https://registry.npmjs.org/@radix-ui/react-use-previous/-/react-use-previous-1.1.0.tgz",
- "integrity": "sha512-Z/e78qg2YFnnXcW88A4JmTtm4ADckLno6F7OXotmkQfeuCVaKuYzqAATPhVzl3delXE7CxIV8shofPn3jPc5Og==",
- "peerDependencies": {
- "@types/react": "*",
- "react": "^16.8 || ^17.0 || ^18.0 || ^19.0 || ^19.0.0-rc"
- },
- "peerDependenciesMeta": {
- "@types/react": {
- "optional": true
- }
- }
- },
"node_modules/@radix-ui/react-use-size": {
"version": "1.0.1",
"resolved": "https://registry.npmjs.org/@radix-ui/react-use-size/-/react-use-size-1.0.1.tgz",
diff --git a/packages/package.json b/packages/package.json
index 63d60d8cb7..eeb33f7baf 100644
--- a/packages/package.json
+++ b/packages/package.json
@@ -26,7 +26,6 @@
"@popperjs/core": "^2.11.8",
"@radix-ui/react-compose-refs": "^1.1.0",
"@radix-ui/react-slider": "^1.1.2",
- "@radix-ui/react-use-previous": "^1.1.0",
"@react-google-maps/api": "^2.19.3",
"@skyscanner/bpk-foundations-web": "^18.1.0",
"@skyscanner/bpk-svgs": "^19.3.0",
From 46a75942abed7931b260fbaf5ad34c423f2af26a Mon Sep 17 00:00:00 2001
From: metalix2 <matthewr@skyscanner.net>
Date: Fri, 18 Oct 2024 10:02:10 +0100
Subject: [PATCH 02/11] Updated tests
---
.../bpk-component-slider/src/BpkSlider.tsx | 4 ++-
.../bpk-component-slider/src/form-test.tsx | 29 ++++++++++++-------
2 files changed, 21 insertions(+), 12 deletions(-)
diff --git a/packages/bpk-component-slider/src/BpkSlider.tsx b/packages/bpk-component-slider/src/BpkSlider.tsx
index 12a9d98ac2..2beccfffb8 100644
--- a/packages/bpk-component-slider/src/BpkSlider.tsx
+++ b/packages/bpk-component-slider/src/BpkSlider.tsx
@@ -42,7 +42,9 @@ export type Props = {
value: number[] | number;
ariaLabel: string[];
ariaValuetext?: string[];
- inputProps?: [{ [key: string]: any }];
+ inputProps?:
+ | [{ [key: string]: any }]
+ | [{ [key: string]: any }, { [key: string]: any }];
[rest: string]: any;
};
diff --git a/packages/bpk-component-slider/src/form-test.tsx b/packages/bpk-component-slider/src/form-test.tsx
index 43079da8e0..238cd3f7cb 100644
--- a/packages/bpk-component-slider/src/form-test.tsx
+++ b/packages/bpk-component-slider/src/form-test.tsx
@@ -16,7 +16,7 @@
* limitations under the License.
*/
-import { useState } from 'react';
+import { useEffect, useState } from 'react';
import { render, screen } from '@testing-library/react';
import userEvent from '@testing-library/user-event';
@@ -34,7 +34,7 @@ window.ResizeObserver =
describe('BpkSlider form-test for single-thumb slider', () => {
it('should work as a form component in a form', async () => {
const Wrap = () => {
- const [sliderValue, setSliderValue] = useState<number>(50); // single-thumb slider with only max value
+ const [sliderValue, setSliderValue] = useState<number>(50); // single-thumb slider with only max value
return (
<form data-testid="form">
<BpkSlider
@@ -67,9 +67,12 @@ describe('BpkSlider form-test for single-thumb slider', () => {
});
it('should emit change event when both sides of slider value are changed for two-thumb slider', async () => {
- const handleChange = jest.fn();
+ const formValidation = jest.fn();
const Wrap = () => {
- const [sliderValue, setSliderValue] = useState<number[]>([0, 100]); // two-thumb slider with min and max value
+ const [sliderValue, setSliderValue] = useState<number[]>([0, 100]); // two-thumb slider with min and max value
+ useEffect(() => {
+ document.addEventListener('change', formValidation);
+ }, []);
return (
<form data-testid="form">
<BpkSlider
@@ -80,8 +83,8 @@ describe('BpkSlider form-test for single-thumb slider', () => {
ariaLabel={['min', 'max']}
ariaValuetext={['0', '100']}
value={sliderValue}
+ inputProps={[{ 'data-testid': 'sliderInputMin' }, { 'data-testid': 'sliderInputMax' }]}
onChange={(value) => {
- handleChange(value as number[]);
setSliderValue(value as number[]);
}}
/>
@@ -94,16 +97,20 @@ describe('BpkSlider form-test for single-thumb slider', () => {
expect(sliderThumbMin).toBeInTheDocument();
await sliderThumbMin.focus();
- await userEvent.keyboard('{ArrowRight}');
- expect(handleChange).toHaveBeenCalledTimes(1);
- expect(handleChange).toHaveBeenCalledWith([1, 100]);
+ // Ideally we could use the mouse and drag but not currently possible in jsdom
+ // Moving the slider over multiple steps should result in one change event
+ await userEvent.keyboard('{ArrowRight>5}{/ArrowRight}');
+
+ expect(screen.getByTestId('sliderInputMin').getAttribute('value')).toBe('5');
+ expect(formValidation).toHaveBeenCalledTimes(1);
const sliderThumbMax = screen.getByLabelText('max');
expect(sliderThumbMax).toBeInTheDocument();
await sliderThumbMax.focus();
- await userEvent.keyboard('{ArrowLeft}');
- expect(handleChange).toHaveBeenCalledTimes(2);
- expect(handleChange).toHaveBeenCalledWith([1, 99]);
+ await userEvent.keyboard('{ArrowLeft>25}{/ArrowLeft}');
+
+ expect(screen.getByTestId('sliderInputMax').getAttribute('value')).toBe('75');
+ expect(formValidation).toHaveBeenCalledTimes(2);
});
});
From eba0ab8cdce62f267942f01d60abe9b81bf91273 Mon Sep 17 00:00:00 2001
From: metalix2 <matthewr@skyscanner.net>
Date: Fri, 18 Oct 2024 10:20:45 +0100
Subject: [PATCH 03/11] restrict keyup to arrow keys
---
.../bpk-component-slider/src/BpkSlider.tsx | 28 +++++++++++++------
1 file changed, 20 insertions(+), 8 deletions(-)
diff --git a/packages/bpk-component-slider/src/BpkSlider.tsx b/packages/bpk-component-slider/src/BpkSlider.tsx
index 2beccfffb8..dfcc8acf10 100644
--- a/packages/bpk-component-slider/src/BpkSlider.tsx
+++ b/packages/bpk-component-slider/src/BpkSlider.tsx
@@ -43,8 +43,8 @@ export type Props = {
ariaLabel: string[];
ariaValuetext?: string[];
inputProps?:
- | [{ [key: string]: any }]
- | [{ [key: string]: any }, { [key: string]: any }];
+ | [{ [key: string]: any }]
+ | [{ [key: string]: any }, { [key: string]: any }];
[rest: string]: any;
};
@@ -157,12 +157,24 @@ const BubbleInput = forwardRef(
once: true,
});
// needed on document as users can drag the thumb while out of the thumb elements mouse area
- document.addEventListener('click', () => interactionEndHandler(value as number), {
- signal: controller.signal,
- });
- thumb.addEventListener('keyup', () => interactionEndHandler(value as number), {
- signal: controller.signal,
- });
+ document.addEventListener(
+ 'click',
+ () => interactionEndHandler(value as number),
+ {
+ signal: controller.signal,
+ },
+ );
+ thumb.addEventListener(
+ 'keyup',
+ (e: KeyboardEvent) => {
+ ['ArrowLeft', 'ArrowRight', 'ArrowUp', 'ArrowDown'].includes(
+ e.key,
+ ) && interactionEndHandler(value as number);
+ },
+ {
+ signal: controller.signal,
+ },
+ );
};
thumb.addEventListener('focusin', focusInHandler);
From ee1167e3861dfd0742e48604946a94c4c69b349d Mon Sep 17 00:00:00 2001
From: metalix2 <matthewr@skyscanner.net>
Date: Fri, 18 Oct 2024 10:24:42 +0100
Subject: [PATCH 04/11] update readme
---
packages/bpk-component-slider/README.md | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/packages/bpk-component-slider/README.md b/packages/bpk-component-slider/README.md
index 4457fff4fc..7ee7576d67 100644
--- a/packages/bpk-component-slider/README.md
+++ b/packages/bpk-component-slider/README.md
@@ -28,3 +28,9 @@ const Slider = () => (
## Props
Check out the full list of props on Skyscanner's [design system documentation website](https://www.skyscanner.design/latest/components/slider/web-aNXvlY7y#section-props-1e).
+
+
+## Native Events
+
+Just like a `input` `type="range"` the BpkSlider will fire a change event from the hidden `input` `type="number"` for each thumb. These behave similarly where user can drag the thumb and will fire a change event on `mouseup`/`click`.
+As for the keyboard events the change event will fire on `keyup` rather than on every keystroke registered like the `input` `type=range` does.
\ No newline at end of file
From 78189ed9d5284dc11f0c1e10ee7a316e7c33e81c Mon Sep 17 00:00:00 2001
From: metalix2 <matthewr@skyscanner.net>
Date: Fri, 18 Oct 2024 15:25:59 +0100
Subject: [PATCH 05/11] refactor and comments
---
.../bpk-component-slider/src/BpkSlider.tsx | 63 ++++++++++---------
1 file changed, 35 insertions(+), 28 deletions(-)
diff --git a/packages/bpk-component-slider/src/BpkSlider.tsx b/packages/bpk-component-slider/src/BpkSlider.tsx
index dfcc8acf10..f53ff21147 100644
--- a/packages/bpk-component-slider/src/BpkSlider.tsx
+++ b/packages/bpk-component-slider/src/BpkSlider.tsx
@@ -135,58 +135,65 @@ const BubbleInput = forwardRef(
const { thumbRef, value, ...inputProps } = props;
const ref = useRef<HTMLInputElement>();
const composedRefs = useComposedRefs(forwardedRef, ref);
+
+ const thumb = thumbRef.current;
+ const input = ref.current;
- // Provide native behaviour that the input range type provides.
+ // This Hook Provides the native behaviour that the input range type would have around the "change" event.
+ // When a user changes the value of the slider. The change event is emitted.
useEffect(() => {
- const thumb = thumbRef.current;
- const input = ref.current;
+ // thumb should be rendered before adding any eventListeners
if (thumb) {
- const interactionEndHandler = (prevValue: number) => {
- if (input) {
+ // The interactionEndHandler is used to ensure that the input value is updated
+ // and change event fired when the user stops interacting with the thumb
+ const interactionEndHandler = (event: MouseEvent | KeyboardEvent) => {
+ if (
+ input &&
+ // if it's a mouse event or arrow key event
+ ((event as MouseEvent).button > -1 ||
+ ['ArrowLeft', 'ArrowRight', 'ArrowUp', 'ArrowDown'].includes(
+ (event as KeyboardEvent).key,
+ ))
+ ) {
+ // parseFloat works for both integers and floats and the Slider supports decimal stepping. e.g. 0 - 1
const newValue = parseFloat(input.getAttribute('value') as string);
- if (prevValue !== newValue) {
+ if (value !== newValue) { // newValue is changed if the slider has moved, clicking away from the thumb will not fire the change event
setNativeValue(input, newValue);
}
}
};
+ // the focusInHandler is used to add event listeners to the document when the thumb is in focus
+ // Allows us to track at which moment the user
const focusInHandler = () => {
- // Controller needed as we may click more than once when in focus. On focusout we can then abort the event listeners
+ // The Controller is needed as we may click more than once when in focus (clicking along the line of slider to move the thumb to that position).
const controller = new AbortController();
+ // On focusout we can then abort the event listeners attached to the thumb and document
thumb.addEventListener('focusout', () => controller.abort(), {
- once: true,
+ once: true, // not necessary to fire more than once and will restart on the next focusin
+ });
+
+ // These two EventListeners signal the end of the interaction with the thumb
+ document.addEventListener('click', interactionEndHandler, { // needed on document as users can drag the thumb while out of the thumb elements mouse area
+ signal: controller.signal,
+ });
+ thumb.addEventListener('keyup', interactionEndHandler, {
+ signal: controller.signal,
});
- // needed on document as users can drag the thumb while out of the thumb elements mouse area
- document.addEventListener(
- 'click',
- () => interactionEndHandler(value as number),
- {
- signal: controller.signal,
- },
- );
- thumb.addEventListener(
- 'keyup',
- (e: KeyboardEvent) => {
- ['ArrowLeft', 'ArrowRight', 'ArrowUp', 'ArrowDown'].includes(
- e.key,
- ) && interactionEndHandler(value as number);
- },
- {
- signal: controller.signal,
- },
- );
};
+ // Add event listeners to the thumb for when the user starts interacting with the thumb
thumb.addEventListener('focusin', focusInHandler);
return () => {
+ // clean up event listeners
if (thumb) {
thumb.removeEventListener('focusin', focusInHandler);
}
};
}
return () => {};
- }, [thumbRef, ref, value]);
+ }, [thumb, input, value]);
/**
* We purposefully do not use `type="hidden"` here otherwise forms that
From de69c930e1c6f6352aacb79afcc864a468e12422 Mon Sep 17 00:00:00 2001
From: metalix2 <matthewr@skyscanner.net>
Date: Fri, 18 Oct 2024 15:58:10 +0100
Subject: [PATCH 06/11] fix tests
---
packages/bpk-component-slider/src/BpkSlider.tsx | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/packages/bpk-component-slider/src/BpkSlider.tsx b/packages/bpk-component-slider/src/BpkSlider.tsx
index f53ff21147..4eb5cffa43 100644
--- a/packages/bpk-component-slider/src/BpkSlider.tsx
+++ b/packages/bpk-component-slider/src/BpkSlider.tsx
@@ -135,13 +135,13 @@ const BubbleInput = forwardRef(
const { thumbRef, value, ...inputProps } = props;
const ref = useRef<HTMLInputElement>();
const composedRefs = useComposedRefs(forwardedRef, ref);
-
- const thumb = thumbRef.current;
- const input = ref.current;
// This Hook Provides the native behaviour that the input range type would have around the "change" event.
// When a user changes the value of the slider. The change event is emitted.
useEffect(() => {
+ // for test works where ref is passed into the useEffect
+ const thumb = thumbRef.current;
+ const input = ref.current;
// thumb should be rendered before adding any eventListeners
if (thumb) {
// The interactionEndHandler is used to ensure that the input value is updated
@@ -193,7 +193,7 @@ const BubbleInput = forwardRef(
};
}
return () => {};
- }, [thumb, input, value]);
+ }, [thumbRef, ref, value]);
/**
* We purposefully do not use `type="hidden"` here otherwise forms that
From 376b3590b7b2ab168a535e8cc399cb1c039fe7a4 Mon Sep 17 00:00:00 2001
From: metalix2 <matthewr@skyscanner.net>
Date: Tue, 22 Oct 2024 09:51:28 +0100
Subject: [PATCH 07/11] support touch events for mobile and update tests
---
examples/bpk-component-modal-v2/examples.tsx | 3 ++
examples/bpk-component-modal/examples.tsx | 3 ++
examples/bpk-component-slider/examples.js | 2 +-
.../bpk-component-checkbox/src/form-test.js | 8 ++--
.../src/form-test.tsx | 6 +--
.../bpk-component-input/src/form-test.tsx | 4 +-
.../bpk-component-nudger/src/form-test.tsx | 12 ++---
.../src/form-test.js | 6 +--
packages/bpk-component-radio/src/form-test.js | 40 ++++++-----------
.../bpk-component-select/src/form-test.js | 44 ++++++++-----------
.../bpk-component-slider/src/BpkSlider.tsx | 13 +++---
.../bpk-component-slider/src/form-test.tsx | 5 +--
.../src/form-test.tsx | 8 +---
.../bpk-component-switch/src/form-test.tsx | 8 ++--
14 files changed, 66 insertions(+), 96 deletions(-)
diff --git a/examples/bpk-component-modal-v2/examples.tsx b/examples/bpk-component-modal-v2/examples.tsx
index 2a680ce7bb..a15ae158f6 100644
--- a/examples/bpk-component-modal-v2/examples.tsx
+++ b/examples/bpk-component-modal-v2/examples.tsx
@@ -22,6 +22,7 @@ import { BpkButtonV2 } from '../../packages/bpk-component-button';
import { BpkModalV2, MODAL_STYLING } from '../../packages/bpk-component-modal';
import BpkText, { TEXT_STYLES } from '../../packages/bpk-component-text';
import { cssModules, withDefaultProps } from '../../packages/bpk-react-utils';
+import { RangeSliderExample } from '../bpk-component-slider/examples';
import type { Props as BpkModalProps } from '../../packages/bpk-component-modal/src/BpkModalV2/BpkModal';
@@ -124,6 +125,8 @@ const DefaultExample = (initiallyOpen: boolean) => (
This is a default modal using the HTML dialog element. You can put
anything you want in here.
</Paragraph>
+ <RangeSliderExample />
+ <input type="range" min="1" max="100" />
</ModalContainer>
);
diff --git a/examples/bpk-component-modal/examples.tsx b/examples/bpk-component-modal/examples.tsx
index b3bce1cab8..0b534d3ce1 100644
--- a/examples/bpk-component-modal/examples.tsx
+++ b/examples/bpk-component-modal/examples.tsx
@@ -35,6 +35,7 @@ import BpkModal, {
import { BpkNavigationBarButtonLink } from '../../packages/bpk-component-navigation-bar';
import BpkText, { TEXT_STYLES } from '../../packages/bpk-component-text';
import { cssModules, withDefaultProps } from '../../packages/bpk-react-utils';
+import { RangeSliderExample } from '../bpk-component-slider/examples';
import STYLES from './examples.module.scss';
@@ -145,6 +146,8 @@ const ModalContainer = (props: ContainerProps) => {
const DefaultExample = (isOpen: boolean) => (
<ModalContainer title="Modal title" closeLabel="Close modal" isOpen={isOpen} >
This is a default modal. You can put anything you want in here.
+ <RangeSliderExample />
+ <input type="range" min="1" max="100" />
</ModalContainer>
);
diff --git a/examples/bpk-component-slider/examples.js b/examples/bpk-component-slider/examples.js
index 5ae6c6fc38..27024cfe81 100644
--- a/examples/bpk-component-slider/examples.js
+++ b/examples/bpk-component-slider/examples.js
@@ -63,7 +63,7 @@ class SliderContainer extends Component {
<BpkSlider
min={min}
max={time ? 59 : 100}
- step={1}
+ step={0.5}
onChange={this.handleChange}
{...this.props}
value={this.state.value}
diff --git a/packages/bpk-component-checkbox/src/form-test.js b/packages/bpk-component-checkbox/src/form-test.js
index 5906cf36a2..a75eb72c88 100644
--- a/packages/bpk-component-checkbox/src/form-test.js
+++ b/packages/bpk-component-checkbox/src/form-test.js
@@ -16,7 +16,7 @@
* limitations under the License.
*/
-import { useState, useEffect } from 'react';
+import { useState } from 'react';
import { render, screen } from '@testing-library/react';
import userEvent from '@testing-library/user-event';
@@ -57,10 +57,7 @@ describe('BpkCheckbox form test', () => {
const formValidation = jest.fn();
const Wrap = () => {
// state is required to force react to update and re-render the component.
- const [isChecked, setIsChecked] = useState(false);
- useEffect(() => {
- document.addEventListener('change', formValidation);
- }, []);
+ const [isChecked, setIsChecked] = useState(false);
return (
<form data-testid="form">
<BpkCheckbox
@@ -75,6 +72,7 @@ describe('BpkCheckbox form test', () => {
);
};
render(<Wrap />);
+ document.addEventListener('change', formValidation);
const checkbox = screen.getByTestId('mycheckbox');
expect(checkbox).not.toBeChecked();
diff --git a/packages/bpk-component-datepicker/src/form-test.tsx b/packages/bpk-component-datepicker/src/form-test.tsx
index 01e5f9cd87..fb9dc81cc8 100644
--- a/packages/bpk-component-datepicker/src/form-test.tsx
+++ b/packages/bpk-component-datepicker/src/form-test.tsx
@@ -17,7 +17,7 @@
*/
-import { useEffect, useState } from 'react';
+import { useState } from 'react';
import { render, screen } from '@testing-library/react';
import userEvent from '@testing-library/user-event';
@@ -122,9 +122,6 @@ describe('BpkDatepicker form test', () => {
const formValidation = jest.fn();
const Wrap = () => {
const [calendarDate, setCalendarDate] = useState(new Date(2020, 2, 19));
- useEffect(() => {
- document.addEventListener('change', formValidation);
- }, []);
return (
<form data-testid="form">
<BpkDatepicker
@@ -157,6 +154,7 @@ describe('BpkDatepicker form test', () => {
};
render(<Wrap />);
+ document.addEventListener('change', formValidation);
const inputField = screen.getByRole('textbox', {
name: /Thursday, 19th March 2020/i,
diff --git a/packages/bpk-component-input/src/form-test.tsx b/packages/bpk-component-input/src/form-test.tsx
index 25f629d1e2..f5b50fb5fc 100644
--- a/packages/bpk-component-input/src/form-test.tsx
+++ b/packages/bpk-component-input/src/form-test.tsx
@@ -65,9 +65,6 @@ describe('BpkInput', () => {
const Wrap = () => {
// state is required to force react to update and re-render the component.
const [inputTest, setInputTest] = useState('');
- useEffect(() => {
- document.addEventListener('change', formValidation);
- }, []);
return (
<form data-testid="form">
<BpkInput
@@ -85,6 +82,7 @@ describe('BpkInput', () => {
};
render(<Wrap />);
+ document.addEventListener('change', formValidation);
const textInput = screen.getByTestId('myInput');
const form = screen.getByTestId('form');
diff --git a/packages/bpk-component-nudger/src/form-test.tsx b/packages/bpk-component-nudger/src/form-test.tsx
index 96835b8f2a..a90826807a 100644
--- a/packages/bpk-component-nudger/src/form-test.tsx
+++ b/packages/bpk-component-nudger/src/form-test.tsx
@@ -16,7 +16,7 @@
* limitations under the License.
*/
-import { useEffect, useState } from 'react';
+import { useState } from 'react';
import { render, screen } from '@testing-library/react';
import userEvent from '@testing-library/user-event';
@@ -70,9 +70,6 @@ describe('BpkNudger form test', () => {
const Wrap = () => {
// state is required to force react to update and re-render the component.
const [nudgerValue, setNudgerValue] = useState(5);
- useEffect(() => {
- document.addEventListener('change', formValidation);
- }, []);
return (
<form data-testid="form">
<BpkNudger
@@ -92,7 +89,8 @@ describe('BpkNudger form test', () => {
};
render(<Wrap />);
-
+ document.addEventListener('change', formValidation);
+
const minusButton = screen.getByRole('button', { name: 'Decrease' });
const numInput = screen.getByTestId('myNudger');
@@ -111,9 +109,6 @@ describe('BpkNudger form test', () => {
const Wrap = () => {
// state is required to force react to update and re-render the component.
const [nudgerValue, setNudgerValue] = useState(5);
- useEffect(() => {
- document.addEventListener('change', formValidation);
- }, []);
return (
<form data-testid="form">
<BpkNudger
@@ -133,6 +128,7 @@ describe('BpkNudger form test', () => {
};
render(<Wrap />);
+ document.addEventListener('change', formValidation);
const numInput = screen.getByTestId('myNudger');
const button = screen.getByRole('button', { name: 'Submit' });
diff --git a/packages/bpk-component-phone-input/src/form-test.js b/packages/bpk-component-phone-input/src/form-test.js
index a274a0d3bb..1fd7b356d6 100644
--- a/packages/bpk-component-phone-input/src/form-test.js
+++ b/packages/bpk-component-phone-input/src/form-test.js
@@ -16,7 +16,7 @@
* limitations under the License.
*/
-import { useEffect, useState } from 'react';
+import { useState } from 'react';
import { render, screen } from '@testing-library/react';
import userEvent from '@testing-library/user-event';
@@ -90,9 +90,6 @@ describe('BpkPhoneInput form test', () => {
const Wrap = () => {
// state is required to force react to update and re-render the component.
const [inputValue, setInputValue] = useState('');
- useEffect(() => {
- document.addEventListener('change', formValidation);
- }, []);
return (
<form data-testid="form">
<BpkPhoneInput
@@ -108,6 +105,7 @@ describe('BpkPhoneInput form test', () => {
};
render(<Wrap />);
+ document.addEventListener('change', formValidation);
const phoneInput = screen.getByTestId('myInput');
const form = screen.getByTestId('form');
diff --git a/packages/bpk-component-radio/src/form-test.js b/packages/bpk-component-radio/src/form-test.js
index 5522857a63..356c668312 100644
--- a/packages/bpk-component-radio/src/form-test.js
+++ b/packages/bpk-component-radio/src/form-test.js
@@ -16,10 +16,6 @@
* limitations under the License.
*/
-/* @flow strict */
-
-import { useEffect } from 'react';
-
import { render, screen } from '@testing-library/react';
import userEvent from '@testing-library/user-event';
@@ -51,29 +47,21 @@ describe('BpkRadio form test', () => {
it('should emit change event when toggled', async () => {
const formValidation = jest.fn();
- const Wrap = () => {
- useEffect(() => {
- document.addEventListener('change', formValidation);
- }, []);
- return (
- <form data-testid="form">
- <BpkRadio
- type="radio"
- name="radio"
- value="One"
- data-testid="myradio"
- />
- <BpkRadio
- type="radio"
- name="radio"
- value="Two"
- data-testid="myradio2"
- />
- <button type="submit">Submit</button>
- </form>
- );
- };
+ const Wrap = () => (
+ <form data-testid="form">
+ <BpkRadio type="radio" name="radio" value="One" data-testid="myradio" />
+ <BpkRadio
+ type="radio"
+ name="radio"
+ value="Two"
+ data-testid="myradio2"
+ />
+ <button type="submit">Submit</button>
+ </form>
+ );
+
render(<Wrap />);
+ document.addEventListener('change', formValidation);
const radio = screen.getByTestId('myradio');
const radio2 = screen.getByTestId('myradio2');
diff --git a/packages/bpk-component-select/src/form-test.js b/packages/bpk-component-select/src/form-test.js
index fcb7a61bf3..bc14feec68 100644
--- a/packages/bpk-component-select/src/form-test.js
+++ b/packages/bpk-component-select/src/form-test.js
@@ -16,8 +16,6 @@
* limitations under the License.
*/
-import { useEffect } from 'react';
-
import { render, screen } from '@testing-library/react';
import userEvent from '@testing-library/user-event';
@@ -65,30 +63,24 @@ describe('BpkSelect form test', () => {
it('should emit change event on option selection that calls formValidation', async () => {
const formValidation = jest.fn();
- const Wrap = () => {
- useEffect(() => {
- document.addEventListener('change', formValidation);
- }, []);
- return (
- <form data-testid="form">
- <BpkSelect
- id="fruits"
- name="fruits"
- defaultValue="apples"
- data-testid="myselect"
- >
- <option value="apples">Apples</option>
- <option data-testid="select-option" value="oranges">
- Oranges
- </option>
- <option value="pears">Pears</option>
- <option value="tomatoes">Tomatoes</option>
- </BpkSelect>
- <button type="submit">Submit</button>
- </form>
- );
- };
- render(<Wrap />);
+
+ render(<form data-testid="form">
+ <BpkSelect
+ id="fruits"
+ name="fruits"
+ defaultValue="apples"
+ data-testid="myselect"
+ >
+ <option value="apples">Apples</option>
+ <option data-testid="select-option" value="oranges">
+ Oranges
+ </option>
+ <option value="pears">Pears</option>
+ <option value="tomatoes">Tomatoes</option>
+ </BpkSelect>
+ <button type="submit">Submit</button>
+ </form>);
+ document.addEventListener('change', formValidation);
const select = screen.getByTestId('myselect');
diff --git a/packages/bpk-component-slider/src/BpkSlider.tsx b/packages/bpk-component-slider/src/BpkSlider.tsx
index 4eb5cffa43..bd50e618b4 100644
--- a/packages/bpk-component-slider/src/BpkSlider.tsx
+++ b/packages/bpk-component-slider/src/BpkSlider.tsx
@@ -146,11 +146,11 @@ const BubbleInput = forwardRef(
if (thumb) {
// The interactionEndHandler is used to ensure that the input value is updated
// and change event fired when the user stops interacting with the thumb
- const interactionEndHandler = (event: MouseEvent | KeyboardEvent) => {
+ const interactionEndHandler = (event: MouseEvent | KeyboardEvent | TouchEvent) => {
if (
input &&
- // if it's a mouse event or arrow key event
- ((event as MouseEvent).button > -1 ||
+ // if it's a mouse event, touch event or arrow key event
+ ((event as MouseEvent).button > -1 || (event as TouchEvent).touches.length > -1 ||
['ArrowLeft', 'ArrowRight', 'ArrowUp', 'ArrowDown'].includes(
(event as KeyboardEvent).key,
))
@@ -164,7 +164,7 @@ const BubbleInput = forwardRef(
};
// the focusInHandler is used to add event listeners to the document when the thumb is in focus
- // Allows us to track at which moment the user
+ // Allows us to track at which moment the user focuses on the thumb
const focusInHandler = () => {
// The Controller is needed as we may click more than once when in focus (clicking along the line of slider to move the thumb to that position).
const controller = new AbortController();
@@ -173,10 +173,13 @@ const BubbleInput = forwardRef(
once: true, // not necessary to fire more than once and will restart on the next focusin
});
- // These two EventListeners signal the end of the interaction with the thumb
+ // These three EventListeners signal the end of the interaction with the thumb
document.addEventListener('click', interactionEndHandler, { // needed on document as users can drag the thumb while out of the thumb elements mouse area
signal: controller.signal,
});
+ thumb.addEventListener('touchend', interactionEndHandler, {
+ signal: controller.signal,
+ });
thumb.addEventListener('keyup', interactionEndHandler, {
signal: controller.signal,
});
diff --git a/packages/bpk-component-slider/src/form-test.tsx b/packages/bpk-component-slider/src/form-test.tsx
index 238cd3f7cb..b668a9cda9 100644
--- a/packages/bpk-component-slider/src/form-test.tsx
+++ b/packages/bpk-component-slider/src/form-test.tsx
@@ -70,9 +70,7 @@ describe('BpkSlider form-test for single-thumb slider', () => {
const formValidation = jest.fn();
const Wrap = () => {
const [sliderValue, setSliderValue] = useState<number[]>([0, 100]); // two-thumb slider with min and max value
- useEffect(() => {
- document.addEventListener('change', formValidation);
- }, []);
+
return (
<form data-testid="form">
<BpkSlider
@@ -92,6 +90,7 @@ describe('BpkSlider form-test for single-thumb slider', () => {
);
};
render(<Wrap />);
+ document.addEventListener('change', formValidation);
const sliderThumbMin = screen.getByLabelText('min');
expect(sliderThumbMin).toBeInTheDocument();
diff --git a/packages/bpk-component-split-input/src/form-test.tsx b/packages/bpk-component-split-input/src/form-test.tsx
index 957d9bfe33..a08c923e63 100644
--- a/packages/bpk-component-split-input/src/form-test.tsx
+++ b/packages/bpk-component-split-input/src/form-test.tsx
@@ -16,7 +16,7 @@
* limitations under the License.
*/
-import { useEffect, useState } from 'react';
+import { useState } from 'react';
import { render, screen } from '@testing-library/react';
import userEvent from '@testing-library/user-event';
@@ -71,11 +71,6 @@ it('should emit change event when text has been entered and blurred', async () =
const formValidation = jest.fn();
const Wrap = () => {
const [inputTest, setInputTest] = useState('');
-
- useEffect(() => {
- document.addEventListener('change', formValidation);
- }, []);
-
return (
<form data-testid="form">
<BpkSplitInput
@@ -94,6 +89,7 @@ it('should emit change event when text has been entered and blurred', async () =
};
render(<Wrap />);
+ document.addEventListener('change', formValidation);
const splitInputs = screen.getAllByTestId('mySplitInput');
const form = screen.getByTestId('form');
diff --git a/packages/bpk-component-switch/src/form-test.tsx b/packages/bpk-component-switch/src/form-test.tsx
index 6663054c86..6ecd466df5 100644
--- a/packages/bpk-component-switch/src/form-test.tsx
+++ b/packages/bpk-component-switch/src/form-test.tsx
@@ -16,7 +16,7 @@
* limitations under the License.
*/
-import { useState, useEffect } from 'react';
+import { useState } from 'react';
import { render, screen } from '@testing-library/react';
import userEvent from '@testing-library/user-event';
@@ -59,9 +59,6 @@ describe('BpkSwitch form test', () => {
const Wrap = () => {
// state is required to force react to update and re-render the component.
const [isChecked, setIsChecked] = useState(false);
- useEffect(() => {
- document.addEventListener('change', formValidation);
- }, []);
return (
<form data-testid="form">
<BpkSwitch
@@ -76,7 +73,8 @@ describe('BpkSwitch form test', () => {
);
};
render(<Wrap />);
-
+ document.addEventListener('change', formValidation);
+
const mySwitch = screen.getByTestId('myswitch');
expect(mySwitch).not.toBeChecked();
expect(formValidation).not.toHaveBeenCalled();
From c3544a838399e537257c74290c00e1ecf44dcc65 Mon Sep 17 00:00:00 2001
From: metalix2 <matthewr@skyscanner.net>
Date: Tue, 22 Oct 2024 09:54:51 +0100
Subject: [PATCH 08/11] clean up
---
packages/bpk-component-slider/src/BpkSlider.tsx | 17 +++++++++++------
packages/bpk-component-slider/src/form-test.tsx | 2 +-
2 files changed, 12 insertions(+), 7 deletions(-)
diff --git a/packages/bpk-component-slider/src/BpkSlider.tsx b/packages/bpk-component-slider/src/BpkSlider.tsx
index bd50e618b4..6da9a48715 100644
--- a/packages/bpk-component-slider/src/BpkSlider.tsx
+++ b/packages/bpk-component-slider/src/BpkSlider.tsx
@@ -139,25 +139,29 @@ const BubbleInput = forwardRef(
// This Hook Provides the native behaviour that the input range type would have around the "change" event.
// When a user changes the value of the slider. The change event is emitted.
useEffect(() => {
- // for test works where ref is passed into the useEffect
+ // for tests to work the ref is passed into the useEffect rather than the variable of ref.current.
const thumb = thumbRef.current;
const input = ref.current;
// thumb should be rendered before adding any eventListeners
if (thumb) {
// The interactionEndHandler is used to ensure that the input value is updated
// and change event fired when the user stops interacting with the thumb
- const interactionEndHandler = (event: MouseEvent | KeyboardEvent | TouchEvent) => {
+ const interactionEndHandler = (
+ event: MouseEvent | KeyboardEvent | TouchEvent,
+ ) => {
if (
input &&
// if it's a mouse event, touch event or arrow key event
- ((event as MouseEvent).button > -1 || (event as TouchEvent).touches.length > -1 ||
+ ((event as MouseEvent).button > -1 ||
+ (event as TouchEvent).touches.length > -1 ||
['ArrowLeft', 'ArrowRight', 'ArrowUp', 'ArrowDown'].includes(
(event as KeyboardEvent).key,
))
) {
// parseFloat works for both integers and floats and the Slider supports decimal stepping. e.g. 0 - 1
const newValue = parseFloat(input.getAttribute('value') as string);
- if (value !== newValue) { // newValue is changed if the slider has moved, clicking away from the thumb will not fire the change event
+ if (value !== newValue) {
+ // newValue is changed if the slider has moved, clicking away from the thumb will not fire the change event
setNativeValue(input, newValue);
}
}
@@ -166,7 +170,7 @@ const BubbleInput = forwardRef(
// the focusInHandler is used to add event listeners to the document when the thumb is in focus
// Allows us to track at which moment the user focuses on the thumb
const focusInHandler = () => {
- // The Controller is needed as we may click more than once when in focus (clicking along the line of slider to move the thumb to that position).
+ // The Controller is needed as we may click more than once when in focus (clicking along the line of slider to move the thumb to that position).
const controller = new AbortController();
// On focusout we can then abort the event listeners attached to the thumb and document
thumb.addEventListener('focusout', () => controller.abort(), {
@@ -174,7 +178,8 @@ const BubbleInput = forwardRef(
});
// These three EventListeners signal the end of the interaction with the thumb
- document.addEventListener('click', interactionEndHandler, { // needed on document as users can drag the thumb while out of the thumb elements mouse area
+ document.addEventListener('click', interactionEndHandler, {
+ // needed on document as users can drag the thumb while out of the thumb elements mouse area
signal: controller.signal,
});
thumb.addEventListener('touchend', interactionEndHandler, {
diff --git a/packages/bpk-component-slider/src/form-test.tsx b/packages/bpk-component-slider/src/form-test.tsx
index b668a9cda9..46b01bec47 100644
--- a/packages/bpk-component-slider/src/form-test.tsx
+++ b/packages/bpk-component-slider/src/form-test.tsx
@@ -16,7 +16,7 @@
* limitations under the License.
*/
-import { useEffect, useState } from 'react';
+import { useState } from 'react';
import { render, screen } from '@testing-library/react';
import userEvent from '@testing-library/user-event';
From a21757f80fa2b4a11f0e9badf1ec0b86e02258b5 Mon Sep 17 00:00:00 2001
From: metalix2 <matthewr@skyscanner.net>
Date: Tue, 22 Oct 2024 09:55:47 +0100
Subject: [PATCH 09/11] undo example changes
---
examples/bpk-component-modal-v2/examples.tsx | 3 ---
examples/bpk-component-modal/examples.tsx | 3 ---
2 files changed, 6 deletions(-)
diff --git a/examples/bpk-component-modal-v2/examples.tsx b/examples/bpk-component-modal-v2/examples.tsx
index a15ae158f6..2a680ce7bb 100644
--- a/examples/bpk-component-modal-v2/examples.tsx
+++ b/examples/bpk-component-modal-v2/examples.tsx
@@ -22,7 +22,6 @@ import { BpkButtonV2 } from '../../packages/bpk-component-button';
import { BpkModalV2, MODAL_STYLING } from '../../packages/bpk-component-modal';
import BpkText, { TEXT_STYLES } from '../../packages/bpk-component-text';
import { cssModules, withDefaultProps } from '../../packages/bpk-react-utils';
-import { RangeSliderExample } from '../bpk-component-slider/examples';
import type { Props as BpkModalProps } from '../../packages/bpk-component-modal/src/BpkModalV2/BpkModal';
@@ -125,8 +124,6 @@ const DefaultExample = (initiallyOpen: boolean) => (
This is a default modal using the HTML dialog element. You can put
anything you want in here.
</Paragraph>
- <RangeSliderExample />
- <input type="range" min="1" max="100" />
</ModalContainer>
);
diff --git a/examples/bpk-component-modal/examples.tsx b/examples/bpk-component-modal/examples.tsx
index 0b534d3ce1..b3bce1cab8 100644
--- a/examples/bpk-component-modal/examples.tsx
+++ b/examples/bpk-component-modal/examples.tsx
@@ -35,7 +35,6 @@ import BpkModal, {
import { BpkNavigationBarButtonLink } from '../../packages/bpk-component-navigation-bar';
import BpkText, { TEXT_STYLES } from '../../packages/bpk-component-text';
import { cssModules, withDefaultProps } from '../../packages/bpk-react-utils';
-import { RangeSliderExample } from '../bpk-component-slider/examples';
import STYLES from './examples.module.scss';
@@ -146,8 +145,6 @@ const ModalContainer = (props: ContainerProps) => {
const DefaultExample = (isOpen: boolean) => (
<ModalContainer title="Modal title" closeLabel="Close modal" isOpen={isOpen} >
This is a default modal. You can put anything you want in here.
- <RangeSliderExample />
- <input type="range" min="1" max="100" />
</ModalContainer>
);
From 0f99e9772e27f8316d4f83240dc8a176c77f5fe1 Mon Sep 17 00:00:00 2001
From: metalix2 <matthewr@skyscanner.net>
Date: Tue, 22 Oct 2024 09:56:10 +0100
Subject: [PATCH 10/11] revert step
---
examples/bpk-component-slider/examples.js | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/examples/bpk-component-slider/examples.js b/examples/bpk-component-slider/examples.js
index 27024cfe81..5ae6c6fc38 100644
--- a/examples/bpk-component-slider/examples.js
+++ b/examples/bpk-component-slider/examples.js
@@ -63,7 +63,7 @@ class SliderContainer extends Component {
<BpkSlider
min={min}
max={time ? 59 : 100}
- step={0.5}
+ step={1}
onChange={this.handleChange}
{...this.props}
value={this.state.value}
From 542f09a967ba1af9e8379046c4778c6c5fb27d26 Mon Sep 17 00:00:00 2001
From: metalix2 <matthewr@skyscanner.net>
Date: Tue, 22 Oct 2024 10:18:30 +0100
Subject: [PATCH 11/11] fix test
---
packages/bpk-component-slider/src/BpkSlider.tsx | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/packages/bpk-component-slider/src/BpkSlider.tsx b/packages/bpk-component-slider/src/BpkSlider.tsx
index 6da9a48715..526f4d462f 100644
--- a/packages/bpk-component-slider/src/BpkSlider.tsx
+++ b/packages/bpk-component-slider/src/BpkSlider.tsx
@@ -153,7 +153,7 @@ const BubbleInput = forwardRef(
input &&
// if it's a mouse event, touch event or arrow key event
((event as MouseEvent).button > -1 ||
- (event as TouchEvent).touches.length > -1 ||
+ (event as TouchEvent).touches?.length > -1 ||
['ArrowLeft', 'ArrowRight', 'ArrowUp', 'ArrowDown'].includes(
(event as KeyboardEvent).key,
))