stellasia / neomap

A Neo4j Desktop application to visualize nodes with geographic attributes on a map.
GNU General Public License v3.0
106 stars 13 forks source link

Bluesky #68

Closed okjodom closed 4 years ago

okjodom commented 4 years ago

This is a blue-sky PR that touches on a bunch of open issues. I looked at tackling just the commented out tests, quickly realizing that the current redux dependency causes a lot of pain to that end. In general this PR ended up being a clean up of the codebase, partially fixing issues, and hopefully moving repo to a state from which further issues can be worked on independently with minimal cross blocks.

Change Areas

PS: PR is not meant for check in as is. Let me know your thoughts on the proposed changes, thereafter we can consider breaking it into atomic changes for check in

stellasia commented 4 years ago

Hi @jodom , thank you so much for your time on this!

I have introduced redux recently but see no problem in removing it in favor of most recent React functionalities (also discussed with @wagnerjt ) (and yes, that's where I had to (cowardly) comment tests :-( ) Regarding tests, we definitely need something to test user interactions, so that's great.

I'll take a deeper look at the code and let you know if I have any concern.

okjodom commented 4 years ago

Waiting for feedback on this change proposal. One thing I could do this coming weekend is split out the code into smaller pieces, starting with removal of redux + package bump, then code restructure and tests

wagnerjt commented 4 years ago

Sorry I've been away for too long! I will review these changes tonight!

wagnerjt commented 4 years ago

@jodom -- Everything looks great to me! You have done a number of todo items for me as well as cleaning up some state/props from child components ๐Ÿ‘. I like the singleton neo4jservice you made as well as stubbing out some tests via jest. Good work!

One thing we will run into with neo4j 4.0+, is the multi db per session. Neomap currently only supports the default neo4j db, and I was hoping to upgrade the deps to use the new react context to help with this!

@stellasia -- let me know if you want to review any part of this with you

stellasia commented 4 years ago

@wagnerjt , thank you for your time and review!

@jodom , yes, the plan looks good to me. I would split the last step into two parts if it doesn't imply doing stuffs twice:

  1. Package update (including react + remove redux)
  2. Update test suite
  3. Code improvement (Neo4j singleton service....)
okjodom commented 4 years ago

Sounds good. I'll ping with PRs over the next couple of days ๐Ÿ‘๐Ÿฟ