[NoJira][BpkSlider] - Fix native event handling for the Slider (#3645)

* Fix native event handling for the Slider * Updated tests * restrict keyup to arrow keys * update readme * refactor and comments * fix tests * support touch events for mobile and update tests * clean up * undo example changes * revert step * fix test --------- Co-authored-by: metalix2 <[email protected]>
Skyscanner · Oct 25, 2024 · 6d53c2e · 6d53c2e
1 parent 4e34364
commit 6d53c2e
Show file tree

Hide file tree

Showing 14 changed files with 154 additions and 125 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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/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.