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

Potential changes for major revision #211

Closed
voliva opened this issue Apr 16, 2019 · 4 comments
Closed

Potential changes for major revision #211

voliva opened this issue Apr 16, 2019 · 4 comments
Milestone

Comments

@voliva
Copy link

voliva commented Apr 16, 2019

Hey, thank you so much for this awesome library - I've been looking for something like this for so long.

However, I've seen parts of the structure of the code that I think that they can be improved, the main point being that it was a bit hard to follow what is a component doing as it uses class with method overriding, etc.

For that, I forked this repo, added some of the potential changes I thought they would make sense, and implemented just a few components to check that everything works as expected:

  • App
  • Window
  • VerticalBox
  • Entry
  • Area
    • Area.Group
    • Area.Rectangle
    • Area.Bezier

I documented the reasoning and results in here: PotentialChanges - I could try to paste it down here, but it's some markdown format I think it makes more sense to use github's reader.

Please, don't take this as an attack of some sort, I think it's all good work and I just wanted to point out things where I thought there was room for improvement. I'd be more than pleased to help if you want to transition some of those ideas into your project.

@kusti8
Copy link
Owner

kusti8 commented Apr 16, 2019 via email

@kusti8
Copy link
Owner

kusti8 commented May 5, 2019

I'm currently working on a v2 with a major rewrite of the project. I want to revisit this when I get closer. I agree with your idea, I just want to clean up the code so it is easier to read.

@kusti8
Copy link
Owner

kusti8 commented May 5, 2019

I'm going to be posing questions as I get to parts. About the top-level App component:

I agree that having windows there but not being showed goes against react, so I want to eliminate that. However, I don't think that eliminating it as a React component all together is a good idea. The way I see it if you have multiple windows with separate Windows being applied to a single root App by registering them similar to ReactDOM, then you loose a lot of the communication between those windows AFAIK.

I want to be as similar to the react-native syntax as possible in this rewrite. Some things will inevitably have to change since apps don't support windows etc. React-native has a single place where you register your app component: AppRegistry.registerComponent(appName, () => App);. I'm thinking of keeping that single registry, having a top level App component and then have nested Windows that can be updated like normal components (meaning no windows that are in the tree, but not shown).

@kusti8 kusti8 added this to the 2.0.0 milestone May 5, 2019
@kusti8 kusti8 mentioned this issue May 5, 2019
19 tasks
@kusti8
Copy link
Owner

kusti8 commented May 9, 2019

I've finished up most of the rewrite using a lot of your code and its much nicer. I can probably clean it up more later, but I'll track anything more in #213

@kusti8 kusti8 closed this as completed May 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Version 2 Rewrite
Awaiting triage
Development

No branches or pull requests

2 participants