Skip to content
This repository has been archived by the owner on Jun 17, 2024. It is now read-only.

Move lightweight components to React functional components #76

Merged
merged 9 commits into from
Sep 8, 2020
Merged

Move lightweight components to React functional components #76

merged 9 commits into from
Sep 8, 2020

Conversation

okjodom
Copy link

@okjodom okjodom commented Sep 4, 2020

Everything except Map and Layer, refactored as react functional components with hooks.

  • Existing tests still pass,
  • additional coverage

stellasia
stellasia previously approved these changes Sep 6, 2020
Copy link
Owner

@stellasia stellasia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks very nice!

};
// This blocks render indefinitely if the driver is never resolved.
// A better pattern would be to render anyway without driver,
// only calling getDriver() when a module wants to use the driver.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem here is that the Layer component needs data from the graph to render (to get the list of labels or whether neo4j-spatial is available).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this true when we load data from file?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we start off with a blank layer that doesn't render on the map, driver comes into play when we create new / update layer. IMO that's when we need to check for the driver, and first layer to check for driver should create an instance usable by any other subsequent ones.

We still get side pane and empty map experience even if we didn't have a driver. At create in case we cant boot up a driver, could we possibly provide a flow for troubleshooting/manually establishing a graph connection within neo4j desktop?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, this does not apply when we load data from file.

But it does appear before the layer create/update, since we not only get data for the map, but also data to configure the layer. In terms of UX, it is important to keep this flow. I suggest that we move this discussion to a new issue though, it is not directly related to this PR.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For tracking, discussion about this topic moved to:
#78

@stellasia stellasia dismissed their stale review September 6, 2020 13:09

Conflicts after last PR was merged;

@okjodom
Copy link
Author

okjodom commented Sep 6, 2020

@stellasia, I resolved merge conflict in pr. You can review again

@stellasia stellasia merged commit 4c6a140 into stellasia:master Sep 8, 2020
@stellasia stellasia mentioned this pull request Sep 8, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants