Skip to content

Add offsetParent prop for alternative offset calculations #173

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

Closed
wants to merge 1 commit into from

Conversation

aeneasr
Copy link
Contributor

@aeneasr aeneasr commented Jul 17, 2016

Allows nodes to use the body as origin instead of the parent. This is useful, when the parent's position is changing. When used, resolves #170

This issue was introduced by a398097

please 👀

@aeneasr
Copy link
Contributor Author

aeneasr commented Jul 18, 2016

The issue with the failing tests is that React has changed the way propType warnings are being thrown. It now provides a backtrace. On master the tests fail likewise. I am not sure how to fix this, because there are different error messages being thrown:

Expected spy error to have been called with

[ 'Warning: Failed prop type: Invalid prop transform passed to Draggable - do not set this, set it on the child.' ]

but actual calls were

[ 'Warning: Failed prop type: Invalid prop transform passed to Draggable - do not set this, set it on the child.

        in Draggable' ],

[ 'Warning: Failed prop type: Invalid prop transform passed to DraggableCore - do not set this, set it on the child.

        in DraggableCore (created by Draggable)

        in Draggable' ].

The only way I can think of right now is to replace toHaveBeenCalledWith with toHaveBeenCalled

@acusti
Copy link
Contributor

acusti commented Jul 19, 2016

The tests are fixed by #176

@aeneasr
Copy link
Contributor Author

aeneasr commented Jul 19, 2016

ah thanks, I will rebase once merged

@STRML
Copy link
Collaborator

STRML commented Jul 19, 2016

Thanks for this, I am reviewing. I'm looking for a simpler solution, though. Will get back to you shortly.

@STRML
Copy link
Collaborator

STRML commented Jul 19, 2016

I'd prefer an offsetParent prop rather than this boolean so it can be generic to the user's needs.

@aeneasr
Copy link
Contributor Author

aeneasr commented Jul 19, 2016

@STRML sounds good, will do!

@aeneasr aeneasr force-pushed the allow-body-control branch 2 times, most recently from c0dbe54 to 29d35b2 Compare July 19, 2016 17:03
@aeneasr
Copy link
Contributor Author

aeneasr commented Jul 19, 2016

Ok, it's rebased and `offsetParent chang eis included, thanks for the review :)

* `offsetParent`, if set, uses the passed DOM node to compute drag offsets
* instead of using the parent node.
*/
offsetParent: PropTypes.bool,
Copy link
Collaborator

@STRML STRML Jul 19, 2016

Choose a reason for hiding this comment

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

This isn't a bool. Unfortunately there's no dom node propType so it should be something like:

offsetParent: function(props, propName) {
  if (process.browser && props[propName] && props[propName].nodeType !== 1) {
    throw new Error("Draggable's offsetParent must be a DOM Node.");
  }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah right, sorry I overlooked when changing that.

@STRML
Copy link
Collaborator

STRML commented Jul 19, 2016

Please rebase again as #176 was merged.

@aeneasr aeneasr force-pushed the allow-body-control branch 4 times, most recently from 9c553e6 to 8c2b209 Compare July 19, 2016 17:47
@STRML STRML changed the title Resolves regression issue by introducing useBodyAsOrigin to <DraggableCore> Add offsetParent prop for alternative offset calculations Jul 19, 2016
@aeneasr aeneasr force-pushed the allow-body-control branch from 8c2b209 to 33e0a04 Compare July 19, 2016 17:53
…Core>

Allows nodes to use an arbitrary node as origin instead of the parent. This is useful, when the parent's position is changing. When used, resolves react-grid-layout#170

This issue was introduced by a398097
@aeneasr aeneasr force-pushed the allow-body-control branch from 33e0a04 to 3d98be3 Compare July 19, 2016 17:55
@aeneasr
Copy link
Contributor Author

aeneasr commented Jul 19, 2016

Sorry, I updated the code quite hasty previously. Everything should be ok now, I hope. Thanks for your patience

const offsetParentRect = node.offsetParent === document.body ? {left: 0, top: 0} : offsetParent.getBoundingClientRect();
export function offsetXYFromParentOf(evt: {clientX: number, clientY: number}, node: HTMLElement & {offsetParent: HTMLElement}, draggableCore: DraggableCore): ControlPosition {
const offsetParent = draggableCore.props.offsetParent || node.offsetParent || document.body;
const offsetParentRect = offsetParent ? {left: 0, top: 0} : offsetParent.getBoundingClientRect();
Copy link
Collaborator

Choose a reason for hiding this comment

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

This condition will always be true.

I'm going to refactor this a bit and push. Thanks for the contribution.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Perfect, thank you!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah right this should be offsetParent === document.body

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yep, but we can't even use document.body, we have to use node.ownerDocument.

Almost done, just writing a test to verify.

@STRML STRML mentioned this pull request Jul 19, 2016
STRML added a commit that referenced this pull request Jul 19, 2016
@STRML
Copy link
Collaborator

STRML commented Jul 19, 2016

Pushed in #179!

@STRML STRML closed this Jul 19, 2016
@aeneasr
Copy link
Contributor Author

aeneasr commented Jul 20, 2016

Thanks @STRML !!

@aeneasr aeneasr deleted the allow-body-control branch July 20, 2016 06:38
@JohnyGemityg
Copy link

JohnyGemityg commented Oct 9, 2016

Hi everybody,

can someone help me please how to use this feature? How can i pass html node, when it is not rendered yet?

  <GraphElement
            id={element.id}
            key={element.id}
            posX={element.cords.x}
            posY={element.cords.y}
            parrentOffset={ReactDom.findDOMNode(this)} // Will work after rerender ....
            onElementJointDragStart={() => this.props.onJointDragStart(element.id)}
            onElementJointDragMove={(event, data) => this.props.onJointDragMove(element.id, { x: data.x, y: data.y })}
          />

Thanks

@acusti
Copy link
Contributor

acusti commented Oct 9, 2016

Like you pointed out, you need the parent to have been mounted before you can pass the node as parentOffset. Best method is to get the ref via the callback form, then either set the ref on state to trigger a rerender or use this.forceUpdate. Something like:

setRef = (ref) => {
  this.rootRef = ref;
  this.forceUpdate();
},

render() {
  // First comes your parent html; I just put a div as an example
  return (
    <div className="parent" ref={this.setRef}>
      { this.rootRef ? (
        <GraphElement
            id={element.id}
            key={element.id}
            posX={element.cords.x}
            posY={element.cords.y}
            parentOffset={this.rootRef}
            onElementJointDragStart={() => this.props.onJointDragStart(element.id)}
            onElementJointDragMove={(event, data) => this.props.onJointDragMove(element.id, { x: data.x, y: data.y })}
        />
      ) : null }
    </div>
  );
}

@JohnyGemityg
Copy link

Thank you for quick replay.

It works. I just added a condition to prevent a loop.

setRef(ref) {
    if (this.graphDom === undefined) {
      this.graphDom = ref;
      this.forceUpdate();
    }
  }

But I am not sure if this is the correct way how to work with react.

@STRML
Copy link
Collaborator

STRML commented Oct 9, 2016

Usually you do something like this:

componentDidMount() {
  this.setState({mounted: true});
}

render() {
  ...
  ref="graphElement"
  parentOffset={this.state.mounted ? this.refs.graphElement : null}
  ...
}

@acusti
Copy link
Contributor

acusti commented Oct 10, 2016

String refs are discouraged because of lack of composability / HOC flexibility (reference)

@rhalff
Copy link

rhalff commented Oct 10, 2016

Also wonder whether you actually need to pass the parentOffset.

The drag handler will also receive deltaX and deltaY and is probably what you need to calculate a new position.

onDrag (ev, data) {

  ....

  const coords = {
     x: old.x + data.deltaX,
     y: old.y + data.deltaY
  }
}

@JohnyGemityg
Copy link

Thank you all for your tips and tricks. I think ref should be a function. Toggle the mounted variable in state seems to be a good solution. I need this cause I have draggable elements within another draggable element and all shoud have the same parentOffset.

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.

dragging elements on left border causes deltaX to jump between negative and positive values
5 participants