Migrate Layers to React Functions #80
Conversation
There was a problem hiding this comment.
Hi @JoDoM , sorry for the longest review ever! This PR looks good to me overall. I have made two important comments regarding the neo4j driver syntax, otherwise it fails (tested on my laptop).
Also, the I can't have both the list of available labels and the list of available properties at the same time, in the best scenario, only one of these two lists is filled with data. This is not the case on the master branch.
src/components/Layer.js
Outdated
| Show query | ||
| </Button> | ||
|
|
||
| <Button variant="success" type="submit" onClick={handleUpdateLayer} > |
There was a problem hiding this comment.
I know it was from a previous PR, but realized afterwards so just putting the comment here:
I think it should be to the user to decide whether he wants to create or update a layer, the app should know whether the layer already exists or not.
There was a problem hiding this comment.
And actually, the addLayer and updateLayer functions could be merged (we don't have to worry about performances, we'll never have hundreds of layers).
There was a problem hiding this comment.
I was more concerned with single responsibility for each declared function rather than perf :)
There was a problem hiding this comment.
I think it should be to the user to decide whether he wants to create or update a layer, the app should know whether the layer already exists or not.
Exactly. Having two buttons explicitly require a user to make the decision if they want to update or create new. Functionally, that decision also means a user can quickly clone a new layer out of an old one if that's desirable
I see. Working on this issue. Marked this PR as draft until I resolve |
|
Publishing to run CI job. This branch has a new react-select bug. Pointers for when I get time to resolve: |
Closes issue #55
Layer.formsfilePlease review after PR #79 .