search

kenshoo / react-multi-select

A Multi Select component built with and for React
https://kenshoo.github.io/react-multi-select
MIT License
120 stars 53 forks source link

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

Open yegor-babiy opened 5 years ago

yegor-babiy commented 5 years ago

Fixes #149

Proposed Changes

-

-

yegor-babiy commented 5 years ago

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

liorheber commented 5 years ago

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

liorheber commented 5 years ago

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

talyak commented 5 years ago

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

talyak commented 5 years ago

@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 commented 5 years ago

@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

yegor-babiy commented 5 years ago

@talyak can be merged?

yegor-babiy commented 5 years ago

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