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

Added new prop: keepSelectionOrder #162

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

baptwaels
Copy link
Contributor

Feature

Proposed Changes

  • Keep selection order in destination list

@baptwaels
Copy link
Contributor Author

Hi @liorheber @yegor-babiy,

What do you think of this one ?

);
}

destinationItems = selectedItems.filter(
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you find case when destinationItems is not empty? becouse I can't find use case for it, I think it's redundancy

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, I haven't. Since it was some legacy code, I was frightened to remove it.

if (keepSelectionOrder) {
// In order to keep selection order on the list,
// First, iterate threw already selectedItems
alreadySelectedItems = selectedItems.filter(item => findItem(item, items));
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. alreadySelectedItems is the same that contains in selectedItems, I think doesn't make seanse to do such filtering
  2. your fix doesn't work when I use selection with Shift

@yegor-babiy
Copy link
Contributor

Hi @liorheber @yegor-babiy,

What do you think of this one ?

I am ok, but we need to be sure that we will cover all use cases,
@liorheber @talyak WDYT?

@baptwaels baptwaels force-pushed the feat-add-keep-selection-order-props branch from c50759b to 678755e Compare August 12, 2019 05:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants