Added support for custom dragging and dragged class names

[bvaughn]

Nifty library! Thanks for creating it. :)

I noticed that className can be used to specify a custom class (eg for CSS modules) but there is no way to specify a custom is-dragging or is-dragged class. This PR adds that functionality (and tests).

Note that npm test is broken in master (and won't pass until PR #176 is merged).

Note that npm run lint also fails in master:

# FIXME this is usually global
flow check
.flowconfig:16 Unsupported option: "esproposal.class_instance_fields"
make: *** [lint] Error 4

[STRML]

Thanks @bvaughn. Will review shortly.

Re: Your Unsupported option error, be sure you're on the latest Flow version (npm i -g flow-bin).

[STRML]

How about we make classNameDragging and classNameDragged's value in defaultProps equal to react-draggable-dragging and react-draggable-dragged.

[STRML]

This also needs propTypes.

[bvaughn]

By propTypes do you mean the dontSetMe warning?

[bvaughn]

Sure thing on values overriding the default. I thought you specifically didn't want that approach since you weren't doing that with className but I think it works out nicer anyway! :)

I can't set them in defaultProps since they're supposed to be defined on the children, but I can set them up as either-or (eg `childProps.classNameDragging || 'react-draggable-dragging') if that's what you mean?

[STRML]

Ah no, don't set them on the children. You'll get a React 15.2 warning about undefined properties anyhow. Set them on the Draggable itself.

[bvaughn]

FYI, updating flow-type changes the lint errors, but I still get some:

$ npm run lint

> react-draggable@2.1.2 lint /Users/brianvaughn/Documents/git/react-draggable
> make lint

# FIXME this is usually global
flow check
lib/Draggable.es6:293
293:   render(): React.Element {
                 ^^^^^^^^^^^^^ Element. Application of polymorphic type needs <list of 1 argument>. (Can use `*` for inferrable ones)

lib/DraggableCore.es6:339
339:   render(): React.Element {
                 ^^^^^^^^^^^^^ Element. Application of polymorphic type needs <list of 1 argument>. (Can use `*` for inferrable ones)

[STRML]

I'm not doing it with className because I wanted the className to always at least contain 'react-draggable' so you can easily set catchall styles. For dragging/dragged classes, it is much less likely imo that you would change them and thus you would be aware.

I still don't really see a good reason to do this - why would you need it? Why not just set a className and use selectors like .myDraggable.react-draggable-dragged?

[STRML]

Re: Flow errors rebase this on master.

[bvaughn]

I'm not doing it with className because I wanted the className to always at least contain 'react-draggable' so you can easily set catchall styles. For dragging/draggedclasses, it is much less likely imo that you would change them and thus you would be aware.

I still don't really see a good reason to do this - why would you need it?

Seems like folks could set className='react-draggable and-something-else' if they wanted catch-all styles. In practice I think this just pollutes with a global class name. Seems like overriding would be better. :)

[STRML]

Using a "global" classname is a very common thing to do in many libs. I still don't see the utility but if the patch is very small I will still consider it.

[STRML]

So this should be <Draggable classNameDragging="foo">. Don't set it on the child.

[bvaughn]

Happy to make this change, but considering this and this it seemed inconsistent to require className to be a child property but classNameDragged and classNameDragging to be self properties.

I'm not sure why className was ever a child property anyway, but I didn't want to suggest a non-backwards-compatible Api change.

[bvaughn]

Now I'm a bit confused. You suggested default, overridable values for the dragging and dragged variants, but not for the default class. Why the inconsistency? :)

Yes, using global class names is very common. But so are CSS modules. Seems like if a user specifies a custom class name there's no reason to also tack on a default. But again- this change was your suggestion initially, no? I didn't propose it with my initial PR.

The only reason for this PR is so that applications using CSS modules can avoid having to add :global(...) to use react-draggable.

[STRML]

Ah okay, that makes sense re :global(...) in CSS modules.

I'd propose this API for draggable with their values in defaultProps:

defaultClassName: 'react-draggable'
defaultClassNameDragging: 'react-draggable-dragging'
defaultClassNameDragged: 'react-draggable-dragged'

Then we just reference those when building the final name.

[bvaughn]

Sure! Sounds reasonable.

Please let me know if the updated PR matches your expectations. :)

[STRML]

👍 Great!

[bvaughn]

Thanks for talking through things @STRML :)

[STRML]

Np. Very simple in the merged commit, which I like. Appreciate the contribution