jc - Patch form that creates commons to supply startingDate to POST.
#50
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jacksoncooper
added
Attn-peer-CR
btk5h
suggested changes
Mar 9, 2022
| import BasicLayout from "main/layouts/BasicLayout/BasicLayout"; | ||
| import CommonsTable from 'main/components/Commons/CommonsTable'; | ||
| import { useBackend } from 'main/utils/useBackend'; | ||
| //import { useCurrentUser } from 'main/utils/currentUser' |
There was a problem hiding this comment.
Can we remove this commented import?
Comment on lines
+68
to
+70
| // Stryker disable next-line ArrayDeclaration : [columns] is a performance optimization | ||
| const memoizedColumns = React.useMemo(() => columns, [columns]); | ||
| const memoizedCommons = React.useMemo(() => commons, [commons]); |
There was a problem hiding this comment.
This doesn't actually memoize the columns or commons variables. The reference returned by memoizedColumns and memoizedCommons will change whenever the reference to columns or commons changes, which defeats the point of trying to memoize these fields.
There was a problem hiding this comment.
Shoot, sorry! Shouldn't have requested review, I forked and got a bunch of redundant files.
Comment on lines
+72
to
+74
| for (let readable of commons) { | ||
| readable.startingDate = new Date(readable.startingDate).toLocaleString(); | ||
| } |
There was a problem hiding this comment.
It would be cleaner to do this formatting in the accessor rather than modifying the input data.
| @@ -1,5 +1,6 @@ | |||
| import { render, waitFor, fireEvent } from "@testing-library/react"; | |||
| import OurTable from "main/components/OurTable"; | |||
| //import OurTable from "main/components/OurTable"; | |||
|
Closing, adding to #40. |
jacksoncooper
removed
the
Attn-peer-CR
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The
Commonstype was expanded in #30 to include astartingDatemember. The form dumps its elements into the body of a POST request to/api/commons/new, so we need to make sure the elements have the correctid(?) property. I'm struggling to figure out the internals ofreact-hook-form, and which values it uses for the POST.Forked from @Jacqueskim's branch that implements the page that lists commons for ease of testing. So we should merge #40 first.
Addresses #36.