notree-md / notree

Share your knowledge
MIT License
2 stars 1 forks source link

refactor: oop-cleanup #12

Closed mpryor closed 1 year ago

mpryor commented 1 year ago

I think there are some opportunities for refactoring/cleaning up some of the classes.

I'm going to create a draft PR to explore some of this and may update this to a real PR later.

mpryor commented 1 year ago

I noticed that the SVGElements class doesn't appear to really do anything - mindgraph/draw is rendering with canvas directly. You can delete a lot of the functionality from the class immediately with no change to the output.

The only thing that it really seems to do currently is wrap the radius property, which can just be accessed/derived at render time in the canvas - https://github.com/jollyjerr/mindgraph/pull/12/commits/5a2dcc5e9ad9bdf87b41323cf070abcf53587700#diff-85da206e66d53748eee13584274555243888de57e6a7d1e3e45e4a4f08e13cc7R91

mpryor commented 1 year ago

A few more changes:

mpryor commented 1 year ago

So...

I merged in the other branch so this doesn't drift too far (plus I wanted to take advantage of some of the testing features).

The latest commit (https://github.com/jollyjerr/mindgraph/pull/12/commits/0e755fd0990125adaea54f6e8066ded96828b240) introduced a pretty significant refactor. I'm curious to see how you feel about it, as it changes how some of the public classes are used currently.

Here's an example of a how user would use them, now -https://github.com/jollyjerr/mindgraph/blob/0e755fd0990125adaea54f6e8066ded96828b240/packages/draw/example/main.ts

The intent of this change is to remove the coupling between the Artist and the Simulation. This allows the user to pass in whatever arbitrary nodes/links data to the artist to render it - giving more flexibility for the future.

Another large change in this latest commit is the handling of clicking and mouse move events. I've completely ripped out the uniqueColorToNode logic and just use the following collision detection logic instead - https://github.com/jollyjerr/mindgraph/pull/12/commits/0e755fd0990125adaea54f6e8066ded96828b240#diff-1b6ad78e77b4b2649a857d61600007964174ba5ee1a6d0b69517fd3b9c27e151R126

Very curious to see your thoughts on these most recent changes!

mpryor commented 1 year ago

Okay... so these last few commits may be overboard.

I've been trying to figure out the responsibilities of the Artist class vs the Canvas class, and I think I've dialed it in a little bit in a way that opens these up so that this library can be used as a framework for people to really create their own custom "MindGraph" type things.

A previous commit kinda shows what I'm thinking with how a user could take advantage of these classes - https://github.com/jollyjerr/mindgraph/blob/fc863e471f13c18f3c3375665db7a703aa890e1f/packages/draw/example/main.ts

The new renderables module provides a couple of baked-in objects that a user could leverage. If they aren't happy with them, they can now define their own object that implements the Drawable interface, allowing them to implement their own custom drawing and collision detection logic. See the NodeTriangle class that the user defined, allowing them to translate the simulation objects into Renderable triangles at runtime. Also this refactoring lets the user define onClick / onHover handling on a per element basis.

To your point in the conversation, adding all this granularity and flexibility has kinda hurt the UX, so I'm thinking maybe we have something like a MindGraph toplevel class that acts as a user-friendly wrapper for people who don't want to add or customize anything - https://github.com/jollyjerr/mindgraph/blob/0058e911b05f46d918812ba2d56ee82b5189733c/packages/draw/src/mindgraph.ts and usage: https://github.com/jollyjerr/mindgraph/blob/78ed4b96b407735267e5430ce9e471292d579ce4/packages/draw/example/main.ts

I think this PR is pretty much ready for review

mpryor commented 1 year ago

@jollyjerr - updated to address all the nits, I think she's ready