Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

LG-3959 Combobox value change #2182

Merged
merged 28 commits into from
Jan 29, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
0e45912
mv combobox types
TheSonOfThomp Jan 24, 2024
6ce8b62
Moves /Users/adam.thompson/Documents/GitHub/leafygreen-ui/packages/co…
TheSonOfThomp Jan 24, 2024
eae6bb8
Restores /Users/adam.thompson/Documents/GitHub/leafygreen-ui/packages…
TheSonOfThomp Jan 24, 2024
37140f1
Moves /Users/adam.thompson/Documents/GitHub/leafygreen-ui/packages/co…
TheSonOfThomp Jan 24, 2024
36465c5
Restores /Users/adam.thompson/Documents/GitHub/leafygreen-ui/packages…
TheSonOfThomp Jan 24, 2024
6531991
Moves /Users/adam.thompson/Documents/GitHub/leafygreen-ui/packages/co…
TheSonOfThomp Jan 24, 2024
feee981
Restores /Users/adam.thompson/Documents/GitHub/leafygreen-ui/packages…
TheSonOfThomp Jan 24, 2024
560ac67
Moves /Users/adam.thompson/Documents/GitHub/leafygreen-ui/packages/co…
TheSonOfThomp Jan 24, 2024
6659bc5
Restores /Users/adam.thompson/Documents/GitHub/leafygreen-ui/packages…
TheSonOfThomp Jan 24, 2024
f2eea40
Break out component props into separate files
TheSonOfThomp Jan 24, 2024
c25fe32
rm new test
TheSonOfThomp Jan 24, 2024
61a4d48
Create yellow-items-marry.md
TheSonOfThomp Jan 24, 2024
bc51bc3
mv type files to subcomponents
TheSonOfThomp Jan 24, 2024
c471c30
fix ts
TheSonOfThomp Jan 24, 2024
bbb867d
Update yellow-items-marry.md
TheSonOfThomp Jan 24, 2024
f7de5f2
validate
TheSonOfThomp Jan 24, 2024
743eb8c
adds tests
TheSonOfThomp Jan 24, 2024
4cdb166
Adds `diff` argument to `onChange`
TheSonOfThomp Jan 24, 2024
2406da6
Update Combobox.tsx
TheSonOfThomp Jan 24, 2024
eb524c6
Create ten-pandas-play.md
TheSonOfThomp Jan 24, 2024
c835faf
Update isValueCurrentSelection.ts
TheSonOfThomp Jan 24, 2024
265a04e
Merge branch 'main' into adam/lg-3959-combobox-value-change
TheSonOfThomp Jan 24, 2024
1d641eb
Merge branch 'main' into adam/lg-3959-combobox-value-change
bruugey Jan 25, 2024
df74938
Merge branch 'main' into adam/lg-3959-combobox-value-change
TheSonOfThomp Jan 26, 2024
bc0a03b
Update ten-pandas-play.md
TheSonOfThomp Jan 26, 2024
4fb6813
Update Combobox.spec.tsx
TheSonOfThomp Jan 26, 2024
063c289
Merge branch 'main' into adam/lg-3959-combobox-value-change
TheSonOfThomp Jan 29, 2024
95ff2a4
Merge branch 'main' into adam/lg-3959-combobox-value-change
TheSonOfThomp Jan 29, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 25 additions & 0 deletions .changeset/ten-pandas-play.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
---
'@leafygreen-ui/combobox': minor
---

Combobox `onChange` callback now receives a 2nd argument. Use this argument to determine what value was inserted or deleted from a multiselect value.
TheSonOfThomp marked this conversation as resolved.
Show resolved Hide resolved
[JIRA Ticket](https://jira.mongodb.org/browse/LG-3959)

Example:
```tsx
<Combobox
multiselect
value={['apple', 'banana']}
onChange={(val, diff) => {
console.log(value) // ['apple']
console.log(diff); // { diffType: 'delete', value: 'banana' }
}}
/>
```

```ts
interface DiffObject {
TheSonOfThomp marked this conversation as resolved.
Show resolved Hide resolved
diffType: 'insert' | 'delete';
value: string | Array<string>;
}
```
107 changes: 97 additions & 10 deletions packages/combobox/src/Combobox/Combobox.spec.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -541,6 +541,31 @@ describe('packages/combobox', () => {
}
});

test('Clicking an option fires onChange', () => {
const onChange = jest.fn();
const { openMenu } = renderCombobox(select, {
onChange,
});
const { optionElements } = openMenu();
expect(optionElements).not.toBeUndefined();
const option3 = (optionElements as HTMLCollectionOf<HTMLLIElement>)[2];
act(() => {
userEvent.click(option3);
});

if (select === 'multiple') {
expect(onChange).toHaveBeenCalledWith(
expect.arrayContaining(['carrot']),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this correct - an array containing an array with the string 'carrot'?
I'd expect arrayContaining('carrot')

Copy link
Collaborator Author

@TheSonOfThomp TheSonOfThomp Jan 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah it's confusing. Its essentially "array containing these elements: [...]"
(There's a case where I call expect.arrayContaining(['apple', 'banana']) )

https://jestjs.io/docs/expect#expectarraycontainingarray

expect.objectContaining({
diffType: 'insert',
value: 'carrot',
}),
);
} else {
expect(onChange).toHaveBeenCalledWith('carrot');
}
});

testSingleSelect('Clicking selected option closes menu', async () => {
const { openMenu } = renderCombobox(select, {
initialValue: 'apple',
Expand Down Expand Up @@ -749,6 +774,32 @@ describe('packages/combobox', () => {
});
});

testMultiSelect(
'Clicking chip X button fires onChange with diff',
async () => {
const onChange = jest.fn();
const initialValue = ['apple', 'banana', 'carrot'];
const { queryChipsByName } = renderCombobox(select, {
onChange,
initialValue,
});
const appleChip = queryChipsByName('Apple');
expect(appleChip).not.toBeNull();
const appleChipButton = appleChip!.querySelector('button')!;
userEvent.click(appleChipButton);
await waitFor(() => {
expect(appleChip).not.toBeInTheDocument();
expect(onChange).toHaveBeenCalledWith(
expect.arrayContaining(['banana', 'carrot']),
expect.objectContaining({
diffType: 'delete',
value: 'apple',
}),
);
});
},
);

testMultiSelect('Clicking chip text focuses the chip', () => {
const initialValue = ['apple', 'banana', 'carrot'];
const { queryChipsByName, queryAllChips } = renderCombobox(select, {
Expand Down Expand Up @@ -842,9 +893,18 @@ describe('packages/combobox', () => {
const { inputEl, openMenu } = renderCombobox(select, { onChange });
openMenu();
userEvent.type(inputEl!, '{arrowdown}{enter}');
expect(onChange).toHaveBeenCalledWith(
select === 'single' ? 'banana' : ['banana'],
);

if (select === 'multiple') {
expect(onChange).toHaveBeenCalledWith(
['banana'],
expect.objectContaining({
diffType: 'insert',
value: 'banana',
}),
);
} else {
expect(onChange).toHaveBeenCalledWith('banana');
}
});

test('does not fire onClear handler', () => {
Expand Down Expand Up @@ -1359,7 +1419,8 @@ describe('packages/combobox', () => {
*/
describe('onClear', () => {
test('Clear button calls onClear callback', () => {
const initialValue = select === 'multiple' ? ['apple'] : 'apple';
const initialValue =
select === 'multiple' ? ['apple', 'banana'] : 'apple';
const onClear = jest.fn();
const { clearButtonEl } = renderCombobox(select, {
initialValue,
Expand All @@ -1370,7 +1431,8 @@ describe('packages/combobox', () => {
});

test('Clear button does not force the menu to reopen', () => {
const initialValue = select === 'multiple' ? ['apple'] : 'apple';
const initialValue =
select === 'multiple' ? ['apple', 'banana'] : 'apple';
const onClear = jest.fn();
const { clearButtonEl, queryByRole } = renderCombobox(select, {
initialValue,
Expand All @@ -1381,8 +1443,9 @@ describe('packages/combobox', () => {
expect(queryByRole('listbox')).not.toBeInTheDocument();
});

test('Clear button clears the value of the input', () => {
const initialValue = select === 'multiple' ? ['apple'] : 'apple';
test('Clear button clears the value of the input', async () => {
const initialValue =
select === 'multiple' ? ['apple', 'banana'] : 'apple';
const { inputEl, clearButtonEl, queryChipsByName } = renderCombobox(
select,
{
Expand All @@ -1396,10 +1459,34 @@ describe('packages/combobox', () => {
expect(inputEl).toHaveValue('Apple');
}

act(() => {
userEvent.click(clearButtonEl!);
userEvent.click(clearButtonEl!);
await waitFor(() => {
expect(inputEl).toHaveValue('');
});
expect(inputEl).toHaveValue('');
});

test('Clear button calls onChange callback', () => {
const onChange = jest.fn();
const initialValue =
select === 'multiple' ? ['apple', 'banana'] : 'apple';
const { clearButtonEl } = renderCombobox(select, {
initialValue,
onChange,
});

userEvent.click(clearButtonEl!);

if (select === 'multiple') {
expect(onChange).toHaveBeenCalledWith(
[],
expect.objectContaining({
diffType: 'delete',
value: ['apple', 'banana'],
}),
);
} else {
expect(onChange).toHaveBeenCalledWith(null);
}
});

test('Focus returns to input element after clear button is clicked', async () => {
Expand Down
39 changes: 16 additions & 23 deletions packages/combobox/src/Combobox/Combobox.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ import { InternalComboboxOption } from '../ComboboxOption';
import {
ComboboxElement,
ComboboxSize,
DiffObject,
getNullSelection,
onChangeType,
Overflow,
Expand All @@ -62,6 +63,7 @@ import {
getValueForDisplayName,
} from '../utils';

import { isValueCurrentSelection } from './utils/isValueCurrentSelection';
import {
baseComboboxStyles,
baseInputElementStyle,
Expand Down Expand Up @@ -224,6 +226,11 @@ export function Combobox<M extends boolean>({
if (isMultiselect(selection)) {
// We know M is true here
const newSelection: SelectValueType<true> = clone(selection);
const multiselectOnChange = onChange as onChangeType<true>;
const diff: DiffObject = {
diffType: 'delete',
value: value ?? selection,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this be newSelection

Copy link
Collaborator Author

@TheSonOfThomp TheSonOfThomp Jan 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, since in the case that value is null it means we're deleting all elements. So newSelection is []

So the call would be something like:

  onChange={(val, diff) => {
    console.log(val) // [] (i.e. newSelection)
    console.log(diff); // { diffType: 'delete', value: ['apple', 'banana'] }
  }} 

};

if (isNull(value)) {
newSelection.length = 0;
Expand All @@ -234,38 +241,23 @@ export function Combobox<M extends boolean>({
} else {
// add to array
newSelection.push(value);
diff.diffType = 'insert';
// clear text
setInputValue('');
}
}
setSelection(newSelection as SelectValueType<M>);
(onChange as onChangeType<true>)?.(
newSelection as SelectValueType<true>,
);
multiselectOnChange?.(newSelection, diff);
} else {
const newSelection: SelectValueType<M> = value as SelectValueType<M>;
setSelection(newSelection);
(onChange as onChangeType<false>)?.(
newSelection as SelectValueType<false>,
);
const newSelection: SelectValueType<false> = value;
const singleSelectOnChange = onChange as onChangeType<false>;
setSelection(newSelection as SelectValueType<M>);
singleSelectOnChange?.(newSelection);
}
},
[isMultiselect, onChange, selection],
);

/**
* Returns whether a given value is included in, or equal to, the current selection
* @param value the option value to check
*/
const isValueCurrentSelection = useCallback(
(value: string): boolean => {
return isMultiselect(selection)
? selection.includes(value)
: value === selection;
},
[isMultiselect, selection],
);

/**
* Returns whether given text is included in, or equal to, the current selection.
* Similar to `isValueCurrentSelection`, but assumes the text argument is the `displayName` for the selection
Expand All @@ -274,9 +266,9 @@ export function Combobox<M extends boolean>({
const isTextCurrentSelection = useCallback(
(text: string): boolean => {
const value = getValueForDisplayName(text, allOptions);
return isValueCurrentSelection(value);
return isValueCurrentSelection(value, selection);
},
[allOptions, isValueCurrentSelection],
[allOptions, selection],
);

/**
Expand Down Expand Up @@ -841,6 +833,7 @@ export function Combobox<M extends boolean>({
}, []);

// When controlled value changes, update the selection
// TODO: use useControlledValue
useEffect(() => {
if (!isUndefined(value) && value !== prevValue) {
if (isNull(value)) {
Expand Down
14 changes: 14 additions & 0 deletions packages/combobox/src/Combobox/utils/isValueCurrentSelection.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
import { SelectValueType } from '../../types';

/**
* Returns whether a given value is included in, or equal to, the current selection
* @param value the option value to check
*/
export const isValueCurrentSelection = (
value: string,
selection: SelectValueType<boolean> | null,
): boolean => {
return Array.isArray(selection)
? selection.includes(value)
: value === selection;
};
8 changes: 7 additions & 1 deletion packages/combobox/src/types/Combobox.types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -68,14 +68,20 @@ export type SelectValueType<M extends boolean> = M extends true
? Array<string>
: string | null;

/** Represents an element that was added or removed from the multiselect value array */
export interface DiffObject {
diffType: 'insert' | 'delete';
value: string | Array<string>;
}

/**
* Callback event fired when the value changes
*
* Type varies depending on the value of `multiselect`
*/
// TODO: onChange signature should match the native event handler signature
export type onChangeType<M extends boolean> = M extends true
? (value: SelectValueType<true>) => void
? (value: SelectValueType<true>, diff?: DiffObject) => void
: (value: SelectValueType<false>) => void;

/**
Expand Down
1 change: 1 addition & 0 deletions packages/combobox/src/types/index.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
export {
ComboboxElement,
ComboboxSize,
DiffObject,
getNullSelection,
onChangeType,
Overflow,
Expand Down
Loading