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

Using React component classes #345

Open
kristianmandrup opened this issue Apr 14, 2017 · 9 comments
Open

Using React component classes #345

kristianmandrup opened this issue Apr 14, 2017 · 9 comments

Comments

@kristianmandrup
Copy link

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
Copy link
Member

jonnor commented Apr 18, 2017

Everything changed, not so easy to review...

  • Are there changes in behavior, or is it the same?
  • Which features are tested?
  • Anything which needs to be done before it could be merged and used in production apps?

@jonnor
Copy link
Member

jonnor commented Apr 18, 2017

Extension of #344

@kristianmandrup
Copy link
Author

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
Copy link
Author

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

@jonnor
Copy link
Member

jonnor commented Apr 18, 2017

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:

  • No panning of graph with either touch or mouse
  • No context menu when right clicking on either

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
Copy link
Author

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
Copy link
Member

jonnor commented Apr 19, 2017

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
Copy link
Author

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
Copy link
Member

jonnor commented Apr 19, 2017

Shit happens :) Thank you for the migration notes!

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

No branches or pull requests

2 participants