Create project and feed refactor #41
Conversation
Codecov Report
@@ Coverage Diff @@
## dev #41 +/- ##
========================================
+ Coverage 5.23% 5.25% +0.02%
========================================
Files 324 328 +4
Lines 15405 15580 +175
Branches 4649 4680 +31
========================================
+ Hits 806 819 +13
- Misses 12449 12602 +153
- Partials 2150 2159 +9
Continue to review full report at Codecov.
|
|
|
||
| _onInputChange = memoize( | ||
| (fieldName: string) => (evt: Event & {target: HTMLInputElement}) => { | ||
| const updatedState: State = update(this.state, { |
There was a problem hiding this comment.
this.setState doesn't need to be used in conjunction with update. this.setState will only modify the key you specify in the object you pass in.
There was a problem hiding this comment.
Actually, I do think I want to use it here because I am modifying a subkey of a root key of the state. In this case I'm modifying this.state.model[fieldName].
There was a problem hiding this comment.
Ah, you're totally right. I did not read that carefully.
| }) | ||
| } | ||
|
|
||
| _onInputChange = memoize( |
There was a problem hiding this comment.
Why use memoize here and in other onChange functions?
There was a problem hiding this comment.
When memoize is used, the handler function does not always need to be recreated which results is less times that a re-render is needed. This pattern is used all over analysis-ui.
| this.setState({ validation }) | ||
| } | ||
|
|
||
| render () { |
There was a problem hiding this comment.
Why not do something similar to CreateProject's use of GeneralSettings and include a shared component here? It seems like that approach would be more maintainable (and reduce duplicated code).
There was a problem hiding this comment.
It is different enough that it would get too unwieldy to account for all the differences. The main difference is that all of the fields in the existing FeedSourceSettings component automatically save upon change. In the create version, nothing is saved until you hit the save button.
|
@evansiroky here's a mockup of where CreateFeedSource might work better (i.e., replacing the
|
|
I'll refactor the CreateFeedSource containers and components so that it'll replace the FeedSourceTable. |
|
@landonreed did we still want to include this? |
|
@evansiroky, I think we should include it still, but I must admit I'm a bit afraid of what the merge conflicts will reveal. |
…ct/feed components
|
No more merge conflicts. Should be ready to go. |
|
Reassigning to Landon as #255 has some requested changes. Also, need the requesting changes review status to change to approved. :) |
Create project and feed tweaks
|
More changes to come per catalogueglobal/datatools-server#126 (comment). |
|
This PR is now dependent on #271 and ibi-group/datatools-server#151. |
|
Should be ready to merge. |
Conflicts: lib/manager/actions/projects.js
Refactor the creation of projects and feed sources to occur on separate pages.