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

feat(events): add positionChanged event to NodeModel #178

Merged
merged 1 commit into from
Feb 28, 2019
Merged

feat(events): add positionChanged event to NodeModel #178

merged 1 commit into from
Feb 28, 2019

Conversation

smeijer
Copy link
Contributor

@smeijer smeijer commented Mar 1, 2018

Checklist

  • The code has been run through pretty yarn run pretty
  • The tests pass on CircleCI
  • You have referenced the issue(s) or other PR(s) this fixes/relates-to
  • The PR Template has been filled out (see below)

What?

Trigger a positionChanged event when moving a Node that has the listener assigned.

Why?

Because I don't need to serialize the whole document structure, but I do want to sync the x/y position of nodes. It could even be used to synchronize multiple users (trough websockets), so they can collaborate on the same diagram.

How?

Added an event interface to the NodeModel and made it so that DiagramWidget informs the NodeModel when it's moving. Like all other events; it's up to the end user to throttle/debounce the emitted vents.

@alx
Copy link

alx commented Mar 7, 2018

Thanks for this feature, that's exactly what I was looking for.

I'm trying to install it with yarn but it then fails to find storm-react-diagrams module from my own code, would you have a tip to use it ?

# yarn add git+https://github.com/smeijer/react-diagrams.git#feature/add-position-changed-event
# yarn dev
...
[1] Error in ./src/components/Diagram/CustomNodeWidget.js
[1] Module not found: 'storm-react-diagrams' in /home/alx/code/client/src/components/Diagram                                           
[1]  @ ./src/components/Diagram/CustomNodeWidget.js 15:26-57

Thanks and have a nice day,

Alex

@smeijer
Copy link
Contributor Author

smeijer commented Mar 7, 2018

Yeah, unfortunately, due to the build-process and postPublish hooks, it's not really possible to include this library directly from github. You really need to install it from npm.

So I'm waiting for the merge as well :)

An alternative solution would be to include the dist files in the repo, but I'm not really feeling for that.

@alx
Copy link

alx commented Mar 7, 2018

thanks for your fast response ;)

@smeijer
Copy link
Contributor Author

smeijer commented Mar 7, 2018

Perhaps @dylanvorster can tell me what's blocking the merge? And when a new npm publish can be expected? I'm also waiting for the already merged #174

@dylanvorster
Copy link
Member

Hi @smeijer while at the surface this seems like a simple thing, I would prefer for us to call method such as setPosition which then fire the events with their new co-ordinates. I want to also be able to intercept the event and allow the developer to cancel the actions. This solution that you currently have works, but its part of a higher level problem concerning the event system. Thats why I have been thinking about it for bit. The problem with doing what I explain though, is that there is no real event bus and thus if you move 40 nodes, you will get 40 events. I therefore want to have a system inherent to the library to pool events together and then defer rendering until all the events have fired.

@dylanvorster
Copy link
Member

dylanvorster commented Mar 8, 2018

An alternative solution would be to include the dist files in the repo

Yeah this is not happening (but you know that already) :)

@smeijer
Copy link
Contributor Author

smeijer commented Mar 9, 2018

The problem with doing what I explain though, is that there is no real event bus and thus if you move 40 nodes, you will get 40 events.

Fair enough. And I couldn't agree more actually. The current selection events on selecting by drawing the selection box, as well as the double remove events, can be quite frustrating.

@jschroeter jschroeter mentioned this pull request Mar 16, 2018
@maxleiko
Copy link
Contributor

@smeijer create a dist branch and remove the dist/** from .gitignore then push it on GitHub.
You'll then be able to use your version with:

yarn add your_username/react-diagrams#dist

;)

@arnauddb
Copy link

Could you merge this PR please, that would be very useful :) ?

@dylanvorster
Copy link
Member

I can merge this for now, but this will be completely different when react-diagrams 6.0.0 comes out. Everyone here just needs to realise that while this works, its functionality thats implemented more in the fringe, and less at the core so this method will only fire if a very specific UI instruction is given. Ill merge it shortly, just busy at work.

@dylanvorster dylanvorster merged commit a950106 into projectstorm:master Feb 28, 2019
@smeijer
Copy link
Contributor Author

smeijer commented Feb 28, 2019

Nice. Thanks.

@smeijer smeijer deleted the feature/add-position-changed-event branch February 28, 2019 20:52
@smeijer smeijer restored the feature/add-position-changed-event branch February 28, 2019 20:52
@nikolenkoanton92
Copy link

@dylanvorster are you planning to do a future release of the package with these changes?

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.

6 participants