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

Change API to make configureDragDrop static #54

Closed
4 of 5 tasks
gaearon opened this issue Feb 8, 2015 · 2 comments
Closed
4 of 5 tasks

Change API to make configureDragDrop static #54

gaearon opened this issue Feb 8, 2015 · 2 comments
Milestone

Comments

@gaearon
Copy link
Member

gaearon commented Feb 8, 2015

I propose we make configureDragDrop static, and pass current component as the first argument to each method. This change is a stepping stone to fix endDrag not firing on unmounted component (#38, we'll pass null as component) and also a stepping stone to allow “resurfacing” drag sources (#53, we'll pass another component as component).

I'd like this change to be implemented in a backward-compatible way with a deprecation warning. We will make it a breaking change in 1.0 with #48.

  • Support static configureDragDrop and pass component as the first argument to all methods
  • Update the docs to use the new way
  • Add a deprecation warning with upgrade instructions but still allow old-style configureDragDrop definitions (without component parameter)
  • Make sure deprecation warning is logged once per class and includes class name (like React's own warnings)
  • Prepare a separate PR that removes deprecation warning and makes it a breaking change. We will do that on 1.0 branch.

Any comments or volunteers?

@hakanderyal @nelix

@gaearon gaearon added this to the v0.7 milestone Feb 8, 2015
@gaearon
Copy link
Member Author

gaearon commented Feb 8, 2015

To clarify. Before:

var Card = React.createClass({
  mixins: [DragDropMixin],

  configureDragDrop(registerType) {
    registerType(ItemTypes.CARD, {
      dragSource: {
        beginDrag() {
          return {
            item: {
              id: this.props.id,
              children: <Card {...this.props}/>
            }
          };
        }
      },

      dropTarget: {
        over(item) {
          this.props.moveCard(item.id, this.props.id);
        }
      }
    });
  },

After:

var Card = React.createClass({
  mixins: [DragDropMixin],

  statics: {
    configureDragDrop(registerType) {
      registerType(ItemTypes.CARD, {
        dragSource: {
          beginDrag(component) {
            return {
              item: {
                id: component.props.id,
                children: <Card {...component.props}/>
              }
            };
          }
        },

        dropTarget: {
          over(component, item) {
            component.props.moveCard(item.id, component.props.id);
          }
        }
      });
    }
  }

We may want to have a less nested API later, but this is not the point. The point is using component instead of this so we can later be free to pass null or another component if we wish so.

This breaks the use case when this is accessed directly in configureDragDrop but it's a rare use case and seems only useful in contrived examples.

nelix pushed a commit to nelix/react-dnd that referenced this issue Feb 8, 2015
nelix pushed a commit to nelix/react-dnd that referenced this issue Feb 10, 2015
nelix pushed a commit to nelix/react-dnd that referenced this issue Feb 10, 2015
gaearon added a commit that referenced this issue Feb 10, 2015
@gaearon
Copy link
Member Author

gaearon commented Feb 10, 2015

Released in react-dnd@0.6.4.

@gaearon gaearon closed this as completed Feb 10, 2015
kkga pushed a commit that referenced this issue Apr 21, 2015
Added passing width to groupHeaders and fixed some props of it
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

No branches or pull requests

1 participant