Flavor Stash / Notes #75
Conversation
… added FlavorStash/Note component
# Conflicts: # src/reducers/flavor.js # src/reducers/flavor.test.js # src/reducers/index.js # src/sagas/flavor.js # src/sagas/flavor.test.js # src/sagas/index.js
…n, update; refactored note saga to delete a note with text set null (empty note)
…stash ui improvements
Codecov Report
@@ Coverage Diff @@
## master #75 +/- ##
==========================================
- Coverage 83.19% 80.75% -2.44%
==========================================
Files 88 92 +4
Lines 1660 1850 +190
==========================================
+ Hits 1381 1494 +113
- Misses 279 356 +77
Continue to review full report at Codecov.
|
src/components/FlavorStash/Note.js
Outdated
| super(props); | ||
|
|
||
| this.state = { | ||
| editingNote: false, |
src/components/FlavorStash/Note.js
Outdated
| ); | ||
| } | ||
|
|
||
| handleNoteEditor() { |
src/components/FlavorStash/Note.js
Outdated
| } else { | ||
| actions.createNote(values); | ||
| } | ||
| this.setState({ updatedNote: note || '' }); |
There was a problem hiding this comment.
Why are we copying the note to local component state?
There was a problem hiding this comment.
It's mostly for user experience: it makes the update instant instead of having to pull it back from the API. But it also prevents the need for another GET request.
There was a problem hiding this comment.
I've refactored this, when you create or update a note it updates the collection in the reducer
src/components/FlavorStash/Note.js
Outdated
| <Col md="12"> | ||
| { | ||
| /* eslint-disable-next-line no-sync */ | ||
| unified() |
There was a problem hiding this comment.
My two cents, this shouldn't be done synchronously or inline. Maybe use an async function that updates local component state with the parsed result?
There was a problem hiding this comment.
I haven't been able to get this to work with async. I also haven't been able to find an async example, they all show using this method
src/components/FlavorStash/Note.js
Outdated
| </Button> | ||
| <Button | ||
| onClick={e => this.handleNoteEditor(false, e)} | ||
| className="button-animation button--cancel" |
There was a problem hiding this comment.
This is likely something for a different pr, but I think this class name should be button--animated
src/components/FlavorStash/Note.js
Outdated
| <Fragment> | ||
| { | ||
| /* eslint-disable-next-line no-sync */ | ||
| unified() |
There was a problem hiding this comment.
Same thing here with the inline/sync parsing.
src/components/FlavorStash/Note.js
Outdated
| export const mapDispatchToProps = dispatch => ({ | ||
| actions: bindActionCreators( | ||
| { | ||
| ...noteActions |
There was a problem hiding this comment.
No need to spread if it's the only action group being used.
There was a problem hiding this comment.
Also fixed in the page as well.
| expect(tree).toMatchSnapshot(); | ||
| }); | ||
|
|
||
| it('calls requestStash on mount', () => { |
There was a problem hiding this comment.
Should be requestNote right?
src/pages/user/FlavorStash.js
Outdated
| date(d) { | ||
| const s = new Date(d); | ||
|
|
||
| return s.toLocaleDateString(); |
There was a problem hiding this comment.
Use date-fns format for this - docs
There was a problem hiding this comment.
This seems to be something we should probably work on and integrate later. date-fns doesn't seem to be flexible enough to be used within the scope of this PR.
src/pages/user/FlavorStash.js
Outdated
| editingStash: false, | ||
| removed: {}, | ||
| usage: {}, | ||
| viewingNote: {} |
There was a problem hiding this comment.
This doesn't seem to be used, is it leftover?
src/pages/user/FlavorStash.js
Outdated
| </InputGroup.Text> | ||
| </InputGroup.Prepend> | ||
| <Form.Control {...input} type="number" step="0.1" /> | ||
| <InputGroup.Text id="inputGroupAppend">%</InputGroup.Text> |
There was a problem hiding this comment.
Shouldn't need the id= attribute here.
| <Form.Group as={Col}> | ||
| <InputGroup> | ||
| <InputGroup.Prepend> | ||
| <InputGroup.Text id="inputGroupPrepend"> |
There was a problem hiding this comment.
Shouldn't need the id= attribute here or above.
# Conflicts: # src/pages/user/FlavorStash.js
src/components/FlavorStash/Note.js
Outdated
| </Col> | ||
| </Form.Row> | ||
| ) : null} | ||
| <Field name="userId"> |
There was a problem hiding this comment.
Can these fields be pulled at submit-time rather than populated as hidden fields now?
There was a problem hiding this comment.
I removed them and added a commit, their values are pre-filled and submitted with the form. I mainly just had them there for readability in the generated page source.
# Conflicts: # src/pages/user/FlavorStash.js # src/pages/user/FlavorStash.test.js # src/pages/user/__snapshots__/FlavorStash.test.js.snap
User's Flavor Stash, with editors for min/max percentage and notes. Each flavor gets a single note, which utilizes markdown for rich text.