Skip to content

Commit 76188e9

Browse files
based-ghostbased-ghost
committed
fix: onKeyDown property was recieving incorrect first parameter of event.type instead of event; new property menuId added - this is
what is referenced as the value for the input control's aria-controls and aria-owns attributes
1 parent e09b32b commit 76188e9

File tree

14 files changed

+118
-123
lines changed

14 files changed

+118
-123
lines changed

‎.eslintrc

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,12 @@
11
{
22
"parser": "@typescript-eslint/parser",
33
"extends": [
4-
"plugin:react/recommended",
4+
"plugin:react-hooks/recommended",
55
"plugin:@typescript-eslint/recommended"
66
],
77
"plugins": [
8+
"react",
89
"@typescript-eslint",
9-
"react-hooks",
1010
"prettier"
1111
],
1212
"env": {
@@ -39,7 +39,6 @@
3939
},
4040
"settings": {
4141
"react": {
42-
"pragma": "React",
4342
"version": "detect"
4443
}
4544
}

‎README.md

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -114,8 +114,9 @@ All properties are technically optional (with a few having default values). Very
114114
115115
| Property | Type | Default | Description
116116
:---|:---|:---|:---
117-
| `inputId`| string | `undefined` | The id of the autosize search input
118-
|`selectId`| string | `undefined` | The id of the parent div
117+
| `inputId`| string | `undefined` | The id of the autosize search input control
118+
|`selectId`| string | `undefined` | The id of the parent select container element
119+
|`menuId`| string | `undefined` | The id of the menu container element
119120
|`ariaLabel`| string | `undefined` | Aria label (for assistive tech)
120121
|`isMulti`| bool | `false` | Does the control allow for multiple selections (defaults to single-value mode)
121122
|`async`| bool | `false` | Is the component in 'async' mode - when in 'async' mode, updates to the input search value will NOT cause the effect `useMenuOptions` to execute (this effect parses `options` into stateful value `menuOptions`)

‎__tests__/AutosizeInput.test.tsx

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,7 @@ const createAutosizeInputProps = () => {
3030
const props: AutosizeInputProps = {
3131
inputValue: '',
3232
readOnly: false,
33+
menuOpen: false,
3334
onBlur: onBlurSpy,
3435
onFocus: onFocusSpy,
3536
onChange: onChangeSpy,

‎__tests__/Select.test.tsx

Lines changed: 27 additions & 50 deletions
Original file line numberDiff line numberDiff line change
@@ -34,74 +34,34 @@ test('container elements have static className value (enables styling via classi
3434
expect(getByTestId(MENU_CONTAINER_TESTID!)).toHaveClass(MENU_CONTAINER_CLS);
3535
});
3636

37-
test('id attributes are added to DOM if defined ("selectId" and "inputId" props)', async () => {
38-
const inputId = 'test-input-id';
39-
const selectId = 'test-select-id';
40-
const props = { inputId, selectId };
37+
test('id attributes are added to DOM if defined ("menuId", "selectId", "inputId" props)', async () => {
38+
const props = {
39+
menuId: 'test-menu-id',
40+
inputId: 'test-input-id',
41+
selectId: 'test-select-id'
42+
};
4143
const { getByTestId } = renderSelect(props);
42-
43-
expect(getByTestId(SELECT_CONTAINER_TESTID!)).toHaveAttribute('id', selectId);
44-
expect(getByTestId(AUTOSIZE_INPUT_TESTID!)).toHaveAttribute('id', inputId);
44+
expect(getByTestId(MENU_CONTAINER_TESTID!)).toHaveAttribute('id', props.menuId);
45+
expect(getByTestId(AUTOSIZE_INPUT_TESTID!)).toHaveAttribute('id', props.inputId);
46+
expect(getByTestId(SELECT_CONTAINER_TESTID!)).toHaveAttribute('id', props.selectId);
4547
});
4648

47-
// NOTE: element.not.toBeVisible() relies on access to CSS style sheets to check 'display: none;' - CSS-in-JS breaks this
48-
/* test('menu is not visible by default', async () => {
49-
const { getByTestId } = renderSelect();
50-
expect(getByTestId(MENU_CONTAINER_TESTID!)).not.toBeVisible();
51-
}); */
52-
53-
// NOTE: element.toBeVisible() relies on access to CSS style sheets to check 'display: none;' - CSS-in-JS breaks this
54-
/* test('menu is visible after the "mouseDown" event is fired on the control container', async () => {
55-
const { getByTestId } = renderSelect();
56-
57-
fireEvent.mouseDown(getByTestId(CONTROL_CONTAINER_TESTID!));
58-
expect(getByTestId(MENU_CONTAINER_TESTID!)).toBeVisible();
59-
}); */
60-
6149
test('"onInputFocus" callback should be fired when input is focused (if a defined function)', async () => {
6250
const onFocusSpy = jest.fn();
6351
const props = { onInputFocus: onFocusSpy };
6452
const { getByTestId } = renderSelect(props);
65-
6653
fireEvent.focus(getByTestId(AUTOSIZE_INPUT_TESTID!));
6754
expect(onFocusSpy).toBeCalled();
6855
});
6956

70-
// NOTE: element.not.toBeVisible() relies on access to CSS style sheets to check 'display: none;' - CSS-in-JS breaks this
71-
/* test('when "openMenuOnFocus" = true (the default value), then onFocus of input should open the menu', async () => {
72-
const props = { openMenuOnFocus: true };
73-
const { getByTestId } = renderSelect(props);
74-
75-
fireEvent.focus(getByTestId(AUTOSIZE_INPUT_TESTID!));
76-
expect(getByTestId(MENU_CONTAINER_TESTID!)).toBeVisible();
77-
}); */
78-
7957
test('"onInputBlur" callback should be fired on blur (if a defined function)', async () => {
8058
const onBlurSpy = jest.fn();
8159
const props = { onInputBlur: onBlurSpy };
8260
const { getByTestId } = renderSelect(props);
83-
8461
fireEvent.blur(getByTestId(AUTOSIZE_INPUT_TESTID!));
8562
expect(onBlurSpy).toBeCalled();
8663
});
8764

88-
// NOTE: element.not.toBeVisible() relies on access to CSS style sheets to check 'display: none;' - CSS-in-JS breaks this
89-
/* test('when "isDisabled" = true, the DOM elements render as expected to prevent user interaction', async () => {
90-
const onFocusSpy = jest.fn();
91-
const props = {
92-
isDisabled: true,
93-
onInputFocus: onFocusSpy,
94-
};
95-
96-
const { getByTestId } = renderSelect(props);
97-
98-
// mouseDown event should exit function when disabled (should not open the menu/focus input - which it normally would)
99-
// onFocus callback should not run with mouseDown on control
100-
fireEvent.mouseDown(getByTestId(CONTROL_CONTAINER_TESTID!));
101-
expect(getByTestId(MENU_CONTAINER_TESTID!)).not.toBeVisible();
102-
expect(onFocusSpy).not.toBeCalled();
103-
}); */
104-
10565
test('toggling the menu to open/close fires corresponding callbacks "onMenuOpen" and "onMenuClose" (if they are defined functions)', async () => {
10666
const onMenuOpenSpy = jest.fn();
10767
const onMenuCloseSpy = jest.fn();
@@ -124,4 +84,21 @@ test('toggling the menu to open/close fires corresponding callbacks "onMenuOpen"
12484
test('When "lazyLoadMenu" property = true, then menu components are only rendered in DOM when "menuOpen" state = true', async () => {
12585
const { queryByTestId } = renderSelect({ lazyLoadMenu: true });
12686
expect(queryByTestId(MENU_CONTAINER_TESTID!)).toBeNull();
127-
});
87+
});
88+
89+
// NOTE: element.not.toBeVisible() relies on access to CSS style sheets to check 'display: none;' - CSS-in-JS breaks this
90+
/* test('when "isDisabled" = true, the DOM elements render as expected to prevent user interaction', async () => {
91+
const onFocusSpy = jest.fn();
92+
const props = {
93+
isDisabled: true,
94+
onInputFocus: onFocusSpy,
95+
};
96+
97+
const { getByTestId } = renderSelect(props);
98+
99+
// mouseDown event should exit function when disabled (should not open the menu/focus input - which it normally would)
100+
// onFocus callback should not run with mouseDown on control
101+
fireEvent.mouseDown(getByTestId(CONTROL_CONTAINER_TESTID!));
102+
expect(getByTestId(MENU_CONTAINER_TESTID!)).not.toBeVisible();
103+
expect(onFocusSpy).not.toBeCalled();
104+
}); */

‎package.json

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -48,14 +48,14 @@
4848
],
4949
"devDependencies": {
5050
"@babel/cli": "^7.19.3",
51-
"@babel/core": "^7.20.2",
51+
"@babel/core": "^7.20.5",
5252
"@babel/plugin-transform-runtime": "^7.19.6",
5353
"@babel/preset-env": "^7.20.2",
5454
"@babel/preset-react": "^7.18.6",
5555
"@babel/preset-typescript": "^7.18.6",
5656
"@rollup/plugin-babel": "^6.0.3",
5757
"@rollup/plugin-replace": "^5.0.1",
58-
"@rollup/plugin-typescript": "^9.0.2",
58+
"@rollup/plugin-typescript": "^10.0.1",
5959
"@storybook/addon-storysource": "^6.5.13",
6060
"@storybook/addons": "^6.5.13",
6161
"@storybook/builder-webpack5": "^6.5.13",
@@ -71,8 +71,8 @@
7171
"@types/react-dom": "^18.0.9",
7272
"@types/react-window": "^1.8.5",
7373
"@types/styled-components": "^5.1.26",
74-
"@typescript-eslint/eslint-plugin": "^5.44.0",
75-
"@typescript-eslint/parser": "^5.44.0",
74+
"@typescript-eslint/eslint-plugin": "^5.45.0",
75+
"@typescript-eslint/parser": "^5.45.0",
7676
"babel-jest": "^29.3.1",
7777
"babel-loader": "^9.1.0",
7878
"babel-plugin-styled-components": "^2.0.7",
@@ -95,7 +95,7 @@
9595
"react-toastify": "^9.1.1",
9696
"react-window": "^1.8.8",
9797
"rimraf": "^3.0.2",
98-
"rollup": "^3.4.0",
98+
"rollup": "^3.5.0",
9999
"rollup-plugin-terser": "^7.0.2",
100100
"styled-components": "^5.3.6",
101101
"typescript": "^4.9.3",
@@ -123,6 +123,6 @@
123123
"chromatic": "chromatic --force-rebuild --auto-accept-changes"
124124
},
125125
"dependencies": {
126-
"@babel/runtime": "^7.20.1"
126+
"@babel/runtime": "^7.20.6"
127127
}
128128
}

‎src/Select.tsx

Lines changed: 32 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -21,15 +21,16 @@ import {
2121
FUNCTIONS,
2222
EMPTY_ARRAY,
2323
DEFAULT_THEME,
24-
SELECT_WRAPPER_ATTRS,
2524
PAGE_SIZE_DEFAULT,
2625
PLACEHOLDER_DEFAULT,
2726
LOADING_MSG_DEFAULT,
27+
SELECT_CONTAINER_CLS,
2828
CONTROL_CONTAINER_CLS,
2929
FOCUSED_OPTION_DEFAULT,
3030
NO_OPTIONS_MSG_DEFAULT,
3131
MENU_ITEM_SIZE_DEFAULT,
3232
MENU_MAX_HEIGHT_DEFAULT,
33+
SELECT_CONTAINER_TESTID,
3334
CONTROL_CONTAINER_TESTID
3435
} from './constants';
3536
import type {
@@ -56,9 +57,10 @@ import { isBoolean, isFunction, isPlainObject, mergeDeep, suppressEvent, normali
5657

5758
type SelectProps = Readonly<{
5859
async?: boolean;
60+
menuId?: string;
5961
inputId?: string;
60-
pageSize?: number;
6162
selectId?: string;
63+
pageSize?: number;
6264
isMulti?: boolean;
6365
ariaLabel?: string;
6466
required?: boolean;
@@ -127,7 +129,7 @@ interface ControlWrapperProps extends Pick<SelectProps, 'isInvalid' | 'isDisable
127129
isFocused: boolean;
128130
}
129131

130-
const SelectWrapper = styled.div.attrs(SELECT_WRAPPER_ATTRS)`
132+
const SelectWrapper = styled.div`
131133
position: relative;
132134
box-sizing: border-box;
133135
${({ theme }) => theme.select.css}
@@ -180,6 +182,7 @@ const ControlWrapper = styled.div<ControlWrapperProps>`
180182
const Select = forwardRef<SelectRef, SelectProps>((
181183
{
182184
async,
185+
menuId,
183186
isMulti,
184187
inputId,
185188
selectId,
@@ -374,7 +377,8 @@ const Select = forwardRef<SelectRef, SelectProps>((
374377
}, [isMulti, closeMenuOnSelect, blurInputOnSelect, removeSelectedOption]);
375378

376379
/**
377-
* useImperativeHandle.
380+
* `React.useImperativeHandle`
381+
*
378382
* Exposed API methods/properties available on a ref instance of this Select.tsx component.
379383
* Dependency list passed as the third param to re-create the handle when one of them updates.
380384
*/
@@ -406,15 +410,19 @@ const Select = forwardRef<SelectRef, SelectProps>((
406410
);
407411

408412
/**
409-
* useMountEffect
410-
* If autoFocus = true, focus the control following initial mount.
413+
* `useMountEffect`
414+
*
415+
* If 'autoFocus' true, focus the control following initial mount
411416
*/
412417
useMountEffect(() => {
413-
autoFocus && focusInput();
418+
if (autoFocus) {
419+
focusInput();
420+
}
414421
});
415422

416423
/**
417-
* useEffect
424+
* `React.useEffect`
425+
*
418426
* If 'onSearchChange' function is defined, run as callback when the stateful debouncedInputValue
419427
* updates check if onChangeEvtValue ref is set true, which indicates the inputValue change was triggered by input change event
420428
*/
@@ -426,7 +434,8 @@ const Select = forwardRef<SelectRef, SelectProps>((
426434
}, [onSearchChangeFn, onSearchChangeIsFn, debouncedInputValue]);
427435

428436
/**
429-
* useUpdateEffect
437+
* `useUpdateEffect`
438+
*
430439
* Handle passing 'selectedOption' value(s) to onOptionChange callback function prop (if defined)
431440
*/
432441
useUpdateEffect(() => {
@@ -442,7 +451,8 @@ const Select = forwardRef<SelectRef, SelectProps>((
442451
}, [onOptionChangeFn, onOptionChangeIsFn, isMulti, selectedOption]);
443452

444453
/**
445-
* useUpdateEffect
454+
* `useUpdateEffect`
455+
*
446456
* Handle clearing focused option if menuOptions array has 0 length;
447457
* Handle menuOptions changes - conditionally focus first option and do scroll to first option;
448458
* Handle reseting scroll pos to first item after the previous search returned zero results (use prevMenuOptionsLen)
@@ -472,7 +482,7 @@ const Select = forwardRef<SelectRef, SelectProps>((
472482
}
473483
};
474484

475-
// Only Multiselect mode supports value focusing (ArrowRight || ArrowLeft)
485+
// only Multiselect mode supports value focusing (ArrowRight || ArrowLeft)
476486
const focusValueOnArrowKey = (key: string): void => {
477487
if (!hasSelectedOptions) return;
478488

@@ -492,7 +502,7 @@ const Select = forwardRef<SelectRef, SelectProps>((
492502
: 0;
493503
}
494504

495-
const nextFocusedVal = focusedIdx >= 0
505+
const nextFocusedVal = focusedIdx > -1
496506
? selectedOption[focusedIdx].value!
497507
: null;
498508

@@ -533,8 +543,8 @@ const Select = forwardRef<SelectRef, SelectProps>((
533543
const handleOnKeyDown = (e: KeyboardEvent<HTMLElement>): void => {
534544
if (isDisabled) return;
535545

536-
if (isFunction(onKeyDown)) {
537-
onKeyDown(e.key, inputValue, focusedOption);
546+
if (onKeyDown) {
547+
onKeyDown(e, inputValue, focusedOption);
538548
if (e.defaultPrevented) return;
539549
}
540550

@@ -563,7 +573,7 @@ const Select = forwardRef<SelectRef, SelectProps>((
563573
focusOptionOnArrowKey(OptionIndexEnum.PAGEDOWN);
564574
break;
565575
}
566-
// Handle spacebar keydown events
576+
// handle spacebar keydown events
567577
case ' ': {
568578
if (inputValue) return;
569579

@@ -685,15 +695,14 @@ const Select = forwardRef<SelectRef, SelectProps>((
685695

686696
const flexValueWrapper = !!isMulti && hasSelectedOptions;
687697
const showClear = !!isClearable && !isDisabled && hasSelectedOptions;
688-
const inputReadOnly = isDisabled || !isSearchable || !!focusedMultiValue;
689698

690699
return (
691700
<ThemeProvider theme={theme}>
692701
<SelectWrapper
693702
id={selectId}
694-
aria-controls={inputId}
695-
aria-expanded={menuOpen}
696703
onKeyDown={handleOnKeyDown}
704+
className={SELECT_CONTAINER_CLS}
705+
data-testid={SELECT_CONTAINER_TESTID}
697706
>
698707
<ControlWrapper
699708
ref={controlRef}
@@ -719,15 +728,18 @@ const Select = forwardRef<SelectRef, SelectProps>((
719728
<AutosizeInput
720729
id={inputId}
721730
ref={inputRef}
731+
menuId={menuId}
732+
menuOpen={menuOpen}
722733
required={required}
723734
ariaLabel={ariaLabel}
735+
isInvalid={isInvalid}
724736
inputValue={inputValue}
725-
readOnly={inputReadOnly}
726737
onBlur={handleOnInputBlur}
727738
onFocus={handleOnInputFocus}
728739
onChange={handleOnInputChange}
729740
ariaLabelledBy={ariaLabelledBy}
730741
hasSelectedOptions={hasSelectedOptions}
742+
readOnly={!isSearchable || !!focusedMultiValue}
731743
/>
732744
</ValueWrapper>
733745
<IndicatorIcons
@@ -744,6 +756,7 @@ const Select = forwardRef<SelectRef, SelectProps>((
744756
/>
745757
</ControlWrapper>
746758
<Menu
759+
id={menuId}
747760
menuRef={menuRef}
748761
menuOpen={menuOpen}
749762
isLoading={isLoading}

0 commit comments

Comments
 (0)