Issue #149 - It is possible to select more than maxSelectedItems when by yegor-babiy · Pull Request #158 · kenshoo/react-multi-select

yegor-babiy · 2019-07-10T11:02:52Z

#Fixes #149

Proposed Changes

… using Shift

yegor-babiy · 2019-07-10T13:13:00Z

@liorheber please take a look and what do you think?

liorheber · 2019-07-16T17:08:36Z

Sorry for the delay here.
I'll be going over the PR tomorrow.

liorheber · 2019-07-30T17:57:23Z

@talyak I've been having a hard time getting to this PR.
Any chance you'd be willing to take a look?

talyak · 2019-07-31T04:26:18Z

@liorheber, @yegor-babiy - I will review it today.

talyak · 2019-07-31T09:41:22Z

@yegor-babiy - why not to change the following function like this:

export const getMinMaxIndexes = (
currentIndex,
firstItemShiftSelected,
maxSelectedItems
) => {
const range = Math.abs(firstItemShiftSelected - currentIndex) + 1;
if (range > maxSelectedItems) {
return firstItemShiftSelected > currentIndex
? {
minIndex: firstItemShiftSelected - maxSelectedItems + 1,
maxIndex: firstItemShiftSelected
}
: {
minIndex: firstItemShiftSelected,
maxIndex: firstItemShiftSelected + maxSelectedItems - 1
};
}
return firstItemShiftSelected > currentIndex
? { minIndex: currentIndex, maxIndex: firstItemShiftSelected }
: { minIndex: firstItemShiftSelected, maxIndex: currentIndex };
};

yegor-babiy · 2019-07-31T10:22:23Z

@talyak
first of all codeclimate will say that it's too complicated function with more than 5 operations and about logic in this case you don't count selected items that was selected outside range before

talyak · 2019-08-01T05:00:14Z

src/components/multi_select_state.js

+      } = this.state;
+
+      const initialInterval = getMinMaxIndexes(index, firstItemShiftSelected);
+      const outsideIntervalSelectedItems = getSelectedItemsOutsideInterval(


all this logic should happen just when the Shift pressed, right?
also, I prefer to extract all the logic to the utils.
the MinMax function will give the interval and will do all the necessary calculation in it.

Updated, I believe that it looks better, thanks for your feedback and please review again

talyak · 2019-08-06T04:35:09Z

src/components/multi_select_state_utils.js

+) =>
+  selectedItems.filter(selectedItem => {
+    const index = sourceItems.findIndex(item => item.id === selectedItem.id);
+    return !isWithin(index, interval) || selectedItem.disabled;


why do u need the disable check? can u give me scenario?

Sure, please see attached screen

talyak · 2019-08-06T04:36:26Z

src/components/multi_select_state_utils.js

+) => {
+  const { minIndex, maxIndex } = interval;
+  const shouldBeSelect = outsideSelectedItems + (maxIndex - minIndex);
+  return maxSelectedItems > 0 && maxSelectedItems <= shouldBeSelect


why you need to check > 0 ? is it not enough to do it like this:
maxSelectedItems && maxSelectedItems <= shouldBeSelect

talyak

some small comments, sorry.
this logic is complicated and I want to keep it as clear as possible.

talyak · 2019-08-08T04:33:00Z

src/components/multi_select_state_utils.js

+  const shouldBeSelect =
+    outsideSelected + Math.abs(firstItemShiftSelected - currentIndex) + 1;
+
+  if (maxSelected && maxSelected <= shouldBeSelect) {


can u extract this logic to function with clear name, something like: getIndexesWhenShiftSelected ?

talyak · 2019-08-08T04:34:11Z

src/components/multi_select_state_utils.js

+  selectedItems,
+  maxSelected
+) => {
+  const outsideSelected = getSelectedItemsOutsideInterval(


numberOfItemsOutsideSelected ?

talyak · 2019-08-08T04:39:05Z

src/components/multi_select_state_utils.js

+    items,
+    selectedItems
+  ).length;
+  const shouldBeSelect =


sumItemsToSelect or sumItemsShouldBeSelect ?

talyak · 2019-08-08T04:39:43Z

src/components/multi_select_state_utils.js

+    outsideSelected + Math.abs(firstItemShiftSelected - currentIndex) + 1;
+
+  if (maxSelected && maxSelected <= shouldBeSelect) {
+    const availableToSelect = maxSelected - outsideSelected - 1;


sumItemsAllowToSelect ?

yegor-babiy · 2019-08-13T13:07:50Z

@talyak can be merged?

yegor-babiy · 2019-08-22T08:31:11Z

@liorheber @talyak guess it can be merged,
I made all your recommendation

yegor-babiy assigned MaksimDrobchak and liorheber Jul 10, 2019

Issue #149 - It is possible to select more than maxSelectedItems when…

9a4c39f

… using Shift

yegor-babiy force-pushed the fix-multi-selection-issue branch from 4589986 to 9a4c39f Compare July 10, 2019 12:17

yegor-babiy assigned AmirAsaraf Jul 16, 2019

talyak reviewed Aug 6, 2019

View reviewed changes

Fix review issues

4667207

talyak reviewed Aug 8, 2019

View reviewed changes

Replace names on more meaningful

ebee557

Merge branch 'master' into fix-multi-selection-issue

08ad50c

Conversation

yegor-babiy commented Jul 10, 2019

Proposed Changes

Uh oh!

yegor-babiy commented Jul 10, 2019

Uh oh!

liorheber commented Jul 16, 2019

Uh oh!

liorheber commented Jul 30, 2019

Uh oh!

talyak commented Jul 31, 2019

Uh oh!

talyak commented Jul 31, 2019

Uh oh!

yegor-babiy commented Jul 31, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

talyak left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yegor-babiy commented Aug 13, 2019

Uh oh!

yegor-babiy commented Aug 22, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants