Issue #149 - It is possible to select more than maxSelectedItems when #158
Conversation
yegor-babiy
force-pushed
the
fix-multi-selection-issue
branch
from
4589986 to
9a4c39f
Compare
|
@liorheber please take a look and what do you think? |
|
Sorry for the delay here. |
|
@talyak I've been having a hard time getting to this PR. |
|
@liorheber, @yegor-babiy - I will review it today. |
|
@yegor-babiy - why not to change the following function like this: export const getMinMaxIndexes = ( |
|
@talyak |
src/components/multi_select_state.js
Outdated
| } = this.state; | ||
|
|
||
| const initialInterval = getMinMaxIndexes(index, firstItemShiftSelected); | ||
| const outsideIntervalSelectedItems = getSelectedItemsOutsideInterval( |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Updated, I believe that it looks better, thanks for your feedback and please review again
| ) => | ||
| selectedItems.filter(selectedItem => { | ||
| const index = sourceItems.findIndex(item => item.id === selectedItem.id); | ||
| return !isWithin(index, interval) || selectedItem.disabled; |
There was a problem hiding this comment.
why do u need the disable check? can u give me scenario?
There was a problem hiding this comment.
Sure, please see attached screen
| ) => { | ||
| const { minIndex, maxIndex } = interval; | ||
| const shouldBeSelect = outsideSelectedItems + (maxIndex - minIndex); | ||
| return maxSelectedItems > 0 && maxSelectedItems <= shouldBeSelect |
There was a problem hiding this comment.
why you need to check > 0 ? is it not enough to do it like this:
maxSelectedItems && maxSelectedItems <= shouldBeSelect
| const shouldBeSelect = | ||
| outsideSelected + Math.abs(firstItemShiftSelected - currentIndex) + 1; | ||
|
|
||
| if (maxSelected && maxSelected <= shouldBeSelect) { |
There was a problem hiding this comment.
can u extract this logic to function with clear name, something like: getIndexesWhenShiftSelected ?
| selectedItems, | ||
| maxSelected | ||
| ) => { | ||
| const outsideSelected = getSelectedItemsOutsideInterval( |
There was a problem hiding this comment.
numberOfItemsOutsideSelected ?
| items, | ||
| selectedItems | ||
| ).length; | ||
| const shouldBeSelect = |
There was a problem hiding this comment.
sumItemsToSelect or sumItemsShouldBeSelect ?
| outsideSelected + Math.abs(firstItemShiftSelected - currentIndex) + 1; | ||
|
|
||
| if (maxSelected && maxSelected <= shouldBeSelect) { | ||
| const availableToSelect = maxSelected - outsideSelected - 1; |
|
@talyak can be merged? |
|
@liorheber @talyak guess it can be merged, |
#Fixes #149
Proposed Changes