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

Desired cleanups #314

Open
18 of 21 tasks
jonnor opened this issue Dec 21, 2016 · 9 comments
Open
18 of 21 tasks

Desired cleanups #314

jonnor opened this issue Dec 21, 2016 · 9 comments

Comments

@jonnor
Copy link
Member

jonnor commented Dec 21, 2016

Right now the code has a couple of warts

  • Business logic intermixed with rendering
  • State is kept spread across in many different places
  • Cross-element dependencies (the-graph-nav gets an the-graph-editor)
  • No separation of state which affects the underlying graph, and what just affects the presentation/view
  • Pulls in all of NoFlo (incl the FBP runtime). Right now quite fat, and confusing becuase the-graph is not NoFlo-specific

Also, we need to get rid of Polymer, see #222

TODO

Concrete things

General cleanups

  • Split out the Graph things from NoFlo.
  • Use Graph library instead of NoFlo
  • Make usable via NPM (no Bower needed)
  • Remove dependency in the-graph-nav on editor element
  • Remove registerComponent() and getComponent(), in favor plain library property

Use CommonJS modules instead of communicating via TheGraph global

  • CommonJS build setup
  • Thumbnail and Nav use only CommonJS
  • Move .mixins to module, require() where used
  • Move .arcs to module, require() where used
  • Move geometry utilities TheGraph.find... out of the-graph.js
  • Move default/config out of the-graph.js
  • Change factories to be local to module, required when needed. Expose as TheGraph.$module.factories for overriding, probably with legacy mappping in place

React modernizaton

  • Remove React.createFactory() usage, in favor of exposing the class and using React.createElement()

Polymer-removal

  • Move loading of FBP graph out of the-graph-editor
  • Make the-graph-thumb usable without Polymer wrapping
  • Make the-graph-nav usable without Polymer wrapping
  • Remove use of PolymerGestures in favor of HammerJS
  • Make the-graph-editor usable without any Polymer wrapping
  • Move the Polymer elements over to noflo-ui, remove the dependency
  • Kill unused the-graph Polymer element, use React directly in the-graph-editor

Later

  • Remove setState use for setSelectedEdges etc, just use props

Loose guidelines

Generally how to approach things

  • Add tests before making changes. Missing automated tests #313
  • Move logic from Polymer .HTML components down into .JS files.
  • Document responsibility of each module/class
  • Minimize area of public APIs
  • Ensure that interactivity is only done in -nav and -editor (not -thumb and the-graph)
  • Introduce a dedicated "ViewState" for keeping track of state which affects View.
    Pan/zoom/scroll, selection
  • Introduce a dedicated "State". Contains primarily the NoFlo/FBP graph
  • Move the library/component code into part of State
@jonnor
Copy link
Member Author

jonnor commented Dec 21, 2016

An open question is how to do gestures without Polymer.

@forresto
Copy link
Collaborator

@forresto
Copy link
Collaborator

the-graph-editor / the-graph was supposed to be like the-graph-nav / the-graph-thumb, with navigation separated from rendering. Good idea to make those separate (React) components.

@jonnor
Copy link
Member Author

jonnor commented Dec 21, 2016

Fair points. Keeping interactivity away from rendering is good. Updated description.

@forresto
Copy link
Collaborator

Reason for including noflo originally was to listen to changes from elsewhere (p2p) --

componentDidMount: function () {
// To change port colors
this.props.graph.on("addEdge", this.resetPortRoute);
this.props.graph.on("changeEdge", this.resetPortRoute);
this.props.graph.on("removeEdge", this.resetPortRoute);
this.props.graph.on("removeInitial", this.resetPortRoute);
// Listen to noflo graph object's events
this.props.graph.on("changeNode", this.markDirty);
this.props.graph.on("changeInport", this.markDirty);
this.props.graph.on("changeOutport", this.markDirty);
this.props.graph.on("endTransaction", this.markDirty);
ReactDOM.findDOMNode(this).addEventListener("the-graph-node-remove", this.removeNode);
},

@jonnor
Copy link
Member Author

jonnor commented Dec 21, 2016

Yes, and that is still legit. Its just that this functionality should move outside of NoFlo so it can be shared more nicely.

@jonnor
Copy link
Member Author

jonnor commented Jan 5, 2017

the-graph 0.5.x no longer depends on NoFlo, using new fbp-graph library instead #316

@jonnor
Copy link
Member Author

jonnor commented Jan 6, 2017

#320 has a lot of cleanups that reduces dependency on Polymer elements a lot.

@jonnor
Copy link
Member Author

jonnor commented Jan 6, 2017

Things that still are a bit unclear:

  • component library handling
  • event handling
  • selection (nodes, edges) handling

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants