-
Notifications
You must be signed in to change notification settings - Fork 526
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Adds ability to view and create notes from the customer details page #755
Conversation
wip adds ability to create note wip
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
woohoooo can't wait for this! 👯
just a few comments, curious to get your thoughts :)
} | ||
|
||
return ( | ||
<> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit - this wrapper seems unnecessary
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh yeah, good catch. There were multiple elements, but I consolidated them under Box
and forgot to delete this.
{customerNotes.length === 0 ? ( | ||
<Empty image={Empty.PRESENTED_IMAGE_SIMPLE} /> | ||
) : ( | ||
<Flex |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this flex wrapper needed here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, not anymore!
return note.id === otherNote.id; | ||
}); | ||
|
||
this.setState({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks like this will run even if the deletion fails -- i think i prefer to just use the db as the source of truth and just refetch all notes whenever a note is created/updated/deleted (this way we also don't need to guarantee that the response from the create/update methods includes the same preloaded associations)
e.g. here we would just do something like
try {
await API.deleteCustomerNote(note.id);
await this.refreshCustomerNotes();
} catch (error) {
logger.error('Failed to delete customer note', error);
}
|
||
handleCreateNote = async (note: CustomerNote) => { | ||
const {customerNotes} = this.state; | ||
this.setState({customerNotes: [note, ...customerNotes]}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as below for deletions -- let's just refresh the notes from the API (then we won't need this line in the create
endpoint: https://github.com/papercups-io/papercups/pull/755/files#diff-922e3c0541ce8156aae4c07452f58ec476a493d03ca4dc7a11ee69a355007a51R43)
|
||
fetchCustomerNotes = async () => { | ||
this.setState({isLoading: true}); | ||
const customerNotes = await API.fetchCustomerNotes(this.props.customerId); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we wrap this in a try/catch
?
import {CustomerNote} from '../../types'; | ||
import {formatRelativeTime} from '../../utils'; | ||
import * as API from '../../api'; | ||
import dayjs from 'dayjs'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit -- let's make sure external imports are grouped together at the top 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉
Description
The last major feature left for V2 of the customer details page is the ability to view and create notes which we now can do with this commit 🎉
Screenshots
CleanShot.2021-04-20.at.14.39.53.mp4
Checklist
mix test
mix format