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

Fixed mouse offsets (and small other fixes) #348

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

kelegorm
Copy link

@kelegorm kelegorm commented Apr 29, 2017

Fixed mouse offsets in all events. You can see issue when place the-graph in some position, not (0,0). In mouse events (context menu) there was not offset calculated, so context menu was in wrong place. Exception was in renderPreviewEdge method where was used offsets like this.props.app.state.offsetX, but it was useless - you have to set offsets manually.

So I created method to recalculate offsets each time. It's causing some work for mouse events but it's absolutely automatic. I can rework all events to use this.props.app.state.offsetX, but still problem is not solved until some recalculation for offsets is implemented.

Other changes includes few code style fixes to make functions looks consistent.

@kelegorm kelegorm changed the title Made on[Node/Edge]Selected handlers consistent Fixed mouse offsets (and small other fixes) Apr 29, 2017
@kelegorm
Copy link
Author

kelegorm commented Apr 29, 2017

To reproduce problem take https://flowhub.github.io/the-graph/examples/demo-full.html and set absolute position (in dev tools) for the-graph-editor like (256px, 59px) and try call context menu, create new edges and zoom.

@bergie bergie requested a review from forresto May 4, 2017 13:17
@forresto
Copy link
Collaborator

forresto commented May 4, 2017

I can rework all events to use this.props.app.state.offsetX

I'd prefer this path, and pushing that responsibility / calculation to the parent app. Seems expensive to do it on every event, but let's look at numbers. Could you profile dragging a node in your version and compare?

@kelegorm
Copy link
Author

kelegorm commented May 5, 2017

@forresto ok, I'll try.

@YanDoroshenko
Copy link

That issue is really annoying, please merge the guy's stuff.

@jonnor
Copy link
Member

jonnor commented Sep 10, 2017

Unfortunately this does not merge cleanly due to other changes like Polymer removal and CommonJS module compatability. I'm OK with an updated PR using the existing approach (calling the calculation inside event handlers).

@rcarranza-carecloud
Copy link

It would be really helpful to have this fix asap.

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.

5 participants