From 33cbe6dabf0991dad121651b7162afaba351f6e4 Mon Sep 17 00:00:00 2001 From: Dvir Hazout Date: Sat, 11 Jul 2020 18:19:01 +0300 Subject: [PATCH 1/4] [Select] Allow multiple select using shift key --- .../material-ui/src/Select/SelectInput.js | 24 ++++++++++++++++++- 1 file changed, 23 insertions(+), 1 deletion(-) diff --git a/packages/material-ui/src/Select/SelectInput.js b/packages/material-ui/src/Select/SelectInput.js index 27446e29a57061..a017c04ba43479 100644 --- a/packages/material-ui/src/Select/SelectInput.js +++ b/packages/material-ui/src/Select/SelectInput.js @@ -67,6 +67,7 @@ const SelectInput = React.forwardRef(function SelectInput(props, ref) { }); const inputRef = React.useRef(null); + const lastItemIndex = React.useRef(null); const [displayNode, setDisplayNode] = React.useState(null); const { current: isOpenControlled } = React.useRef(openProp != null); const [menuMinWidthState, setMenuMinWidthState] = React.useState(); @@ -169,8 +170,29 @@ const SelectInput = React.forwardRef(function SelectInput(props, ref) { if (multiple) { newValue = Array.isArray(value) ? value.slice() : []; const itemIndex = value.indexOf(child.props.value); - if (itemIndex === -1) { + if ( + event.shiftKey && + lastItemIndex.current && + lastItemIndex.current !== child.props.value && + value.indexOf(lastItemIndex.current) > -1 + ) { + const optionValues = React.Children.map(children, (c) => c.props.value); + const startOptionIndex = optionValues.indexOf(lastItemIndex.current); + const endOptionIndex = optionValues.indexOf(child.props.value); + + if (startOptionIndex > -1 && endOptionIndex > -1) { + let range = []; + if (endOptionIndex > startOptionIndex) { + range = optionValues.slice(startOptionIndex, endOptionIndex + 1); + } else { + range = optionValues.slice(endOptionIndex, startOptionIndex + 1); + } + // Selected range chould contain selected values so we filter them out if exists. + newValue.push(...range.filter((v) => !value.some((vv) => areEqualValues(v, vv)))); + } + } else if (itemIndex === -1) { newValue.push(child.props.value); + lastItemIndex.current = child.props.value; } else { newValue.splice(itemIndex, 1); } From f7a104c471b549562ffc80308fc60337c5019573 Mon Sep 17 00:00:00 2001 From: Dvir Hazout Date: Sat, 11 Jul 2020 18:35:31 +0300 Subject: [PATCH 2/4] fix: Better ref semantic and a comment typo fix --- packages/material-ui/src/Select/SelectInput.js | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/packages/material-ui/src/Select/SelectInput.js b/packages/material-ui/src/Select/SelectInput.js index a017c04ba43479..fb3c85e9f549d7 100644 --- a/packages/material-ui/src/Select/SelectInput.js +++ b/packages/material-ui/src/Select/SelectInput.js @@ -67,7 +67,7 @@ const SelectInput = React.forwardRef(function SelectInput(props, ref) { }); const inputRef = React.useRef(null); - const lastItemIndex = React.useRef(null); + const lastSelectedItemValue = React.useRef(null); const [displayNode, setDisplayNode] = React.useState(null); const { current: isOpenControlled } = React.useRef(openProp != null); const [menuMinWidthState, setMenuMinWidthState] = React.useState(); @@ -172,12 +172,12 @@ const SelectInput = React.forwardRef(function SelectInput(props, ref) { const itemIndex = value.indexOf(child.props.value); if ( event.shiftKey && - lastItemIndex.current && - lastItemIndex.current !== child.props.value && - value.indexOf(lastItemIndex.current) > -1 + lastSelectedItemValue.current && + lastSelectedItemValue.current !== child.props.value && + value.indexOf(lastSelectedItemValue.current) > -1 ) { const optionValues = React.Children.map(children, (c) => c.props.value); - const startOptionIndex = optionValues.indexOf(lastItemIndex.current); + const startOptionIndex = optionValues.indexOf(lastSelectedItemValue.current); const endOptionIndex = optionValues.indexOf(child.props.value); if (startOptionIndex > -1 && endOptionIndex > -1) { @@ -187,12 +187,12 @@ const SelectInput = React.forwardRef(function SelectInput(props, ref) { } else { range = optionValues.slice(endOptionIndex, startOptionIndex + 1); } - // Selected range chould contain selected values so we filter them out if exists. + // Selected range could contain selected values so we filter them out if they exists. newValue.push(...range.filter((v) => !value.some((vv) => areEqualValues(v, vv)))); } } else if (itemIndex === -1) { newValue.push(child.props.value); - lastItemIndex.current = child.props.value; + lastSelectedItemValue.current = child.props.value; } else { newValue.splice(itemIndex, 1); } From 8f9a71f1d6f05a8455cd24a1d995ffd46e39bf96 Mon Sep 17 00:00:00 2001 From: Dvir Hazout Date: Sun, 12 Jul 2020 16:53:38 +0300 Subject: [PATCH 3/4] fix: handle deselect range of options --- .../material-ui/src/Select/SelectInput.js | 27 ++++++++++--------- 1 file changed, 15 insertions(+), 12 deletions(-) diff --git a/packages/material-ui/src/Select/SelectInput.js b/packages/material-ui/src/Select/SelectInput.js index fb3c85e9f549d7..14449b33b0d1b5 100644 --- a/packages/material-ui/src/Select/SelectInput.js +++ b/packages/material-ui/src/Select/SelectInput.js @@ -67,7 +67,7 @@ const SelectInput = React.forwardRef(function SelectInput(props, ref) { }); const inputRef = React.useRef(null); - const lastSelectedItemValue = React.useRef(null); + const lastSelectedItem = React.useRef(null); const [displayNode, setDisplayNode] = React.useState(null); const { current: isOpenControlled } = React.useRef(openProp != null); const [menuMinWidthState, setMenuMinWidthState] = React.useState(); @@ -172,29 +172,32 @@ const SelectInput = React.forwardRef(function SelectInput(props, ref) { const itemIndex = value.indexOf(child.props.value); if ( event.shiftKey && - lastSelectedItemValue.current && - lastSelectedItemValue.current !== child.props.value && - value.indexOf(lastSelectedItemValue.current) > -1 + lastSelectedItem.current && + !areEqualValues(lastSelectedItem.current[1], child.props.value) ) { const optionValues = React.Children.map(children, (c) => c.props.value); - const startOptionIndex = optionValues.indexOf(lastSelectedItemValue.current); + const startOptionIndex = optionValues.indexOf(lastSelectedItem.current[1]); const endOptionIndex = optionValues.indexOf(child.props.value); if (startOptionIndex > -1 && endOptionIndex > -1) { - let range = []; - if (endOptionIndex > startOptionIndex) { - range = optionValues.slice(startOptionIndex, endOptionIndex + 1); + const range = + endOptionIndex > startOptionIndex + ? optionValues.slice(startOptionIndex, endOptionIndex + 1) + : optionValues.slice(endOptionIndex, startOptionIndex + 1); + + if (lastSelectedItem.current[0]) { + // In case of multiple value selection, make sure to set unique values in the `newValue`. + newValue.push(...range.filter((v) => !value.some((vv) => areEqualValues(v, vv)))); } else { - range = optionValues.slice(endOptionIndex, startOptionIndex + 1); + newValue = newValue.filter((v) => !range.some((vv) => areEqualValues(v, vv))); } - // Selected range could contain selected values so we filter them out if they exists. - newValue.push(...range.filter((v) => !value.some((vv) => areEqualValues(v, vv)))); } } else if (itemIndex === -1) { newValue.push(child.props.value); - lastSelectedItemValue.current = child.props.value; + lastSelectedItem.current = [true, child.props.value]; } else { newValue.splice(itemIndex, 1); + lastSelectedItem.current = [false, child.props.value]; } } else { newValue = child.props.value; From e5540dcc4cc81441eae9ab0e6bc2787d8b16b33e Mon Sep 17 00:00:00 2001 From: Dvir Hazout Date: Sun, 12 Jul 2020 16:54:18 +0300 Subject: [PATCH 4/4] chore: add tests to shift multiple select/deselect of multiple options --- .../material-ui/src/Select/Select.test.js | 86 +++++++++++++++++++ 1 file changed, 86 insertions(+) diff --git a/packages/material-ui/src/Select/Select.test.js b/packages/material-ui/src/Select/Select.test.js index cd36a6618b13ec..e8da96b5579487 100644 --- a/packages/material-ui/src/Select/Select.test.js +++ b/packages/material-ui/src/Select/Select.test.js @@ -1048,4 +1048,90 @@ describe(' { + onChange(target.value); + setValue(target.value); + }} + > + France + Germany + China + Israel + Italy + + ); + } + + it('should select range of values', () => { + const handleChange = spy(); + const { getAllByRole } = render(); + + fireEvent.click(getAllByRole('option')[0]); + fireEvent.click(getAllByRole('option')[3], { shiftKey: true }); + + expect(handleChange.args[1][0]).to.deep.equal(['france', 'germany', 'china', 'israel']); + }); + + it('should select range of values in reverse direction', () => { + const handleChange = spy(); + const { getAllByRole } = render(); + + fireEvent.click(getAllByRole('option')[3]); + fireEvent.click(getAllByRole('option')[0], { shiftKey: true }); + + expect(handleChange.args[1][0]).to.deep.equal(['israel', 'france', 'germany', 'china']); + }); + + it('should select range of values and keep uniqueness', () => { + const handleChange = spy(); + const { getAllByRole } = render( + , + ); + + fireEvent.click(getAllByRole('option')[0]); + fireEvent.click(getAllByRole('option')[3], { shiftKey: true }); + + expect(handleChange.args[1][0]).to.deep.equal(['germany', 'france', 'china', 'israel']); + }); + + it('should deselect range of values', () => { + const handleChange = spy(); + const { getAllByRole } = render( + , + ); + + fireEvent.click(getAllByRole('option')[1]); + fireEvent.click(getAllByRole('option')[3], { shiftKey: true }); + + expect(handleChange.args[1][0]).to.deep.equal(['france', 'italy']); + }); + + it('should deselect range of values in reverse direction', () => { + const handleChange = spy(); + const { getAllByRole } = render( + , + ); + + fireEvent.click(getAllByRole('option')[3]); + fireEvent.click(getAllByRole('option')[1], { shiftKey: true }); + + expect(handleChange.args[1][0]).to.deep.equal(['france', 'italy']); + }); + }); });