unclecheese / react-selectable

A component for react that allows mouse selection of child items
MIT License
142 stars 72 forks source link

Add `selectingClassName` option for `<SelectableGroup />` component #48

Open danilvalov opened 7 years ago

danilvalov commented 7 years ago

Please, add selectingClassName option for <SelectableGroup /> component. Often we need to know about the selection process in CSS (for special effects and rules).

danilvalov commented 7 years ago

Done in https://github.com/unclecheese/react-selectable/pull/49

unclecheese commented 7 years ago

Thanks very much for this! I like the idea, and I certainly see the use case, but It feels to me like it's just plugging a hole created by a bigger problem, and that being the encapsulated internal state for isBoxSelecting. What would be ideal is to trigger an event on that state change. How would you feel about something like this:

let className;

const mySelectionHandler = (items) => {
  console.log('you selected', items);
  className='not-selecting';
};
const myBeginSelectionHandler = () => className='selecting';

<SelectableGroup
  onSelection={mySelectionHandler}
  onBeginSelection={myBeginSelectionHandler}
  className={className} 
/>

Point being that, right now, it's className, but there's bound to be other cases for mutating props based on the selecting state.

What do you think?

danilvalov commented 7 years ago

@unclecheese You are right. We need to extend the library API. But if we add a onBeginSelection handler, then we need to add a onEndSelection handler. And developers needs to use both handlers to add and remove a selection class (and they need to use the react state for it). This will make the library less convenient (two handlers for one action). I think we need to add both solutions (adding/removing a class + adding handlers to extend the API. Developers will be able to use the necessary solution for each case.

danilvalov commented 7 years ago

@unclecheese I extended the component API in the following PR: https://github.com/unclecheese/react-selectable/pull/51

What do you think about the solution?