#3333 create reimbursement request timeline #3372
Conversation
jessegarcia1
force-pushed
the
#3333-create-RRtimeline
branch
from
8fd7056 to
07a57e3
Compare
dreifusjack
changed the title
| ); | ||
| }; | ||
|
|
||
| const EventSection: React.FC<EventSectionProps> = ({ event, isLast }) => { |
There was a problem hiding this comment.
I think you can also render the connector line in the ReimbursementRequestTimeline component itself (tracking events.index), that way, you don't have to pass isLast to the EventSection.
There was a problem hiding this comment.
This looks great so far! If you look at the mock the top event on the timeline should have a dashed line leading to it. Also for dates there will be a time associated with the date as well, so you might want to test that out in your mock data and play around with font sizes
| reimbursementRequestId: string; | ||
| } | ||
|
|
||
| interface EventSectionProps { |
There was a problem hiding this comment.
I would rename both this component and FirstSection to be a little more specific. Something like CommentsSection and CreateNewCommentSection
There was a problem hiding this comment.
Unresolving this to rename the props interfaces
src/frontend/src/pages/FinancePage/FinanceComponents/ReimbursementRequestTimeline.tsx
Outdated
Show resolved
Hide resolved
dreifusjack
left a comment
dreifusjack
left a comment
There was a problem hiding this comment.
Great work! I added some commits to fix some things up. I think TimelineCommentModal and CommentModal should be merged into the same component since TimelineCommentModal is essentially just a wrapper
src/frontend/src/pages/FinancePage/FinanceComponents/CommentModal.tsx
Outdated
Show resolved
Hide resolved
| export const useCreateReimbursementRequestComment = (reimbursementRequestId: string) => { | ||
| const queryClient = useQueryClient(); | ||
| return useMutation<ReimbursementRequestComment, Error, ReimbursementRequestCommentPayload>( | ||
| ['reimbursement-requests', 'create'], |
There was a problem hiding this comment.
This needs to be 'reimbursement-requests', or the timeline doesn't update when a user creates a new one, because we are getting the comments from a single reimbursement request.
| comment: yup | ||
| .string() | ||
| .required('Comment is required') | ||
| .test('comment exists', 'Comment field cannot be empty', (value) => { |
There was a problem hiding this comment.
I believe this test and the next one are redundant -- should be covered by .required
| mode: 'onChange' | ||
| }); | ||
|
|
||
| const handleConfirm = async (data: { comment: string; reimbursementRequestId: string }) => { |
There was a problem hiding this comment.
nit: you should be able to remove data: here, and then the values will be destructured (no need for data.[whatever]
| reimbursementRequestId: string; | ||
| } | ||
|
|
||
| interface EventSectionProps { |
There was a problem hiding this comment.
Unresolving this to rename the props interfaces
|
|
||
| const CommentsSection: React.FC<EventSectionProps> = ({ comment, isLast }) => { | ||
| const commentTime = new Date(comment.dateCreated).toLocaleTimeString(); | ||
| const newCommentTime = commentTime.slice(0, -6) + commentTime.slice(-3); |
There was a problem hiding this comment.
add a quick comment explaining this. also, would honesly prefer you use let here and. just reassign commentTime
walker-sean
left a comment
walker-sean
left a comment
There was a problem hiding this comment.
Nice work with the previously requested changes! Main concern right now: the timeline should have it's own section with spacing and a header like the mocks, also would be nice to have some margin below it
| setTimelineCommentModalShow(true); | ||
| }} | ||
| > | ||
| <Typography fontWeight={'regular'}>Send a Follow-Up Message!</Typography> |
There was a problem hiding this comment.
Make this a link so that the cursor will change over it
| <Stack direction="column" alignItems="center" spacing={0.5}> | ||
| <CreateNewCommentSection reimbursementRequestId={reimbursementRequestId} comments={comments} /> | ||
| {comments.map((comment, index) => ( | ||
| <CommentsSection comment={comment} isLast={comments.length - 1 === index} key={index} /> |
There was a problem hiding this comment.
"Comment" is a little bit of a misnomer when I think about it. This is really for activity tracking. One such activity could be a follow-up comment. If it is a follow up, it should say "____ followed up: ____"
Changes
added RR timeline
Screenshots
Checklist
It can be helpful to check the
ChecksandFiles changedtabs.Please review the contributor guide and reach out to your Tech Lead if anything is unclear.
Please request reviewers and ping on slack only after you've gone through this whole checklist.
yarn.lockchanges (unless dependencies have changed)Closes #3333