Added dontClearSelection feature

leopoldjoy commented 8 years ago

Added dontClearSelection feature that, when enabled, makes all new selections add to the already selected items, except for selections that contain only previously selected items—in this case it unselects those items.

leopoldjoy commented 8 years ago

Note: I also included documentation for this feature.

leopoldjoy commented 8 years ago

Added ability to select single elements by clicking on them (without dragging).

unclecheese commented 8 years ago

Thanks for all your work on this. I have a couple of concerns.

I'm not seeing any changes to the source files. Did you commit those?
For the dontClearSelection prop, my goal was to keep this component relatively stateless. I worry that this could result in an endless build-up of state, and it makes the component much less deterministic. It seems to me it would have to be coupled with an API for clearing the state, too.
On the click events, is there a reason you couldn't implement your own onClick handler for your selectable items? Is the selectable component interfering with that? Again, I really want the component to do only one thing, so if it's managing your click events, that's a concern for me. If the issue is that it's just blocking you from clicking, then that's a bug.

leopoldjoy commented 8 years ago

@unclecheese Thanks for the feedback!

I restructured the dontClearSelection feature as well as the item click selection feature. I also added a _clearSelections API as you suggested. Additionally, I properly bundled all of the changes with web-pack, as you had noted.

Note of course that the component is not entirely stateless, however I don't see any easy way to achieve this functionality with an entirely stateless design. What do you think?

I'm open to refactoring further if you have any additional concerns. Please let me know if there is anything else I can do.

leopoldjoy commented 8 years ago

@unclecheese Please let me know how you feel about these features.

I have additionally added support for touch-devices as well as a duringSelection callback feature which allows the user to optionally pass in a function that will be called repeatedly during selection. The duringSelection callback gets passed the items currently under the selector box. This callback can be useful, because it allows the user to style the items currently under the selector as the selection occurs. I haven't committed either of these features to my fork yet. Depending on how you feel, I would be happy to commit all or just some of these. (Maybe just click support and mobile support?)

Thank you, I'm really hoping to make some additions to this very useful package.

unclecheese commented 8 years ago

Hi @leopoldjoy,

Thanks so much for your work on this. I really do appreciate it.

After some thought, I'm not feeling too keen on most of these changes. The primary reason being that I do not want to increase the API surface of this module. I really would like to maintain a tight boundary around that, and have it do only one thing. In a previous version of this module, I was managing click events, maintaining state, and mutating child components. All of that sounds and looks great when it serves your use case, but what I found is that it got in the way with other projects that didn't have the same requirements. The spirit of ReactJS centres on single-purpose components that can be composed to do more things when necessary. As far as I can tell, click events and persisting the list of selected items are both features that could be implemented fairly trivially in the user space.

I am, however, quite interested in your support for touch events, as that's one area I haven't covered. Would you mind resubmitting this with the touch support as the only new feature?

In terms of the other features, I would encourage you to create a separate plugin that declares react-selectable as a dependency and injects those features using composition. Either that, of just feel free to maintain your own fork.

Thanks again for all your work, and feel free to run any new proposals by me. I love having contributions!

leopoldjoy commented 8 years ago

@unclecheese I submitted a new pull request with the touch-based events addition only.

unclecheese / react-selectable

Added dontClearSelection feature #11