flowhub / the-graph

SVG custom elements for FBP graph editing and visualization. Used in noflo/noflo-ui
https://flowhub.github.io/the-graph/demo-full.html
MIT License
1.01k stars 179 forks source link

Using React component classes #345

Open kristianmandrup opened 7 years ago

kristianmandrup commented 7 years ago

My polymer2 branch now has support for using ES6 classes with mixins.

Feel free to help upgrade the rest of the components to ES6 style

jonnor commented 7 years ago

Everything changed, not so easy to review...

jonnor commented 7 years ago

Extension of #344

kristianmandrup commented 7 years ago

I cloned the repo then pushed. Didn't fork, why commit stories might be out of sync. I documented most of the changes in a migration doc in root folder. Most everything should work.

kristianmandrup commented 7 years ago

No changes in behavior. Been working on network graph example only. Can draw connections between nodes as before.

jonnor commented 7 years ago

By migration doc, do you mean 'Polymer2-migration-notes.md'? That file is empty. Anyways,

To you intend for this to branch to merged into this repository? Do you intend to help maintain it if it gets merged?

If so there are a lot of things that needs to be fixed. But before we go to details, there are two major things that must be dealt with. For this to be at all possible to merge, it needs to:

1) Help to remove Polymer dependency (or at least not make it worse) 2) Not break existing functionality

1

This branch seems to add more dependencies on Polymer. And furthermore changes the version to a yet-to-be-released version. Do the React components still work with old Polymer? Can the components be used without Polymer? Does it get us closer to working without Polymer somehow?

2

I tested the examples/demo-network.html (in latest Chrome). Several pieces of basic functionality seems to be broken. For instance:

The other examples are completely broken, the intial graph does not even render. I tried to update to reflect the changed locations of some .js, and it still did not render.

This to me suggests that not much testing of existing features was done...

kristianmandrup commented 7 years ago

The essential (for my needs) functionality works for the network demo. IMO it needs a complete rewrite from scratch. Ideally much of the functionality extracted and not tightly integrated with React or any such framework. Yes, a mess mixing Polymer and React his way. Would be better done using custom elements or web components.

I'm fed up with web development tbh. Still working knee deep in garbage most of the time. Lost my motivation. Can't look at code any more... Need a break for at least 6 months. This code is garbage as well, sorry. Not sure if you are the original author. Can't stand to look at anymore garbage...

jonnor commented 7 years ago

Ok, I am glad you were able to massage the code to fit your purposes! Not a problem that you are not interested in maintaining, I just need to know, since I have to maintain it :)

I am not the original author no, I took over because we use it in flowhub/noflo-ui. The code might be shit (as is typical), but the code is used and useful, which is more important. More problematic is that there are no tests, and documentation of expected functionality is missing.

I don't believe in rewrites from scratch. Prefer to make it better iteratively. Some desired cleanups are docuemented in #314. Reducing framework dependencies is key. The benefits are small for this usecase, and the costs high due to their invasiveness (Polymer is real bad here) and API churn..

kristianmandrup commented 7 years ago

Hey, sorry, was in a bad mood when I replied before ;) I submitted migration notes now. Should be easy to migrate the Polymer classes to Rails classes I think. But yeah, I'm out of the game for a while.. would rather be a farmer than a developer at this point :p

jonnor commented 7 years ago

Shit happens :) Thank you for the migration notes!