Create Log BE/FE Integration #129
Conversation
| // Ideally we should be storing this information in the database | ||
| const BUILDINGS = [ | ||
| { label: "144", value: "144 Erb St. West" }, | ||
| { label: "362", value: "362 Erb St. West" }, | ||
| { label: "402", value: "402 Erb St. West" }, | ||
| ]; | ||
|
|
||
| const ALERT_DATA: AlertDataOptions = { |
There was a problem hiding this comment.
Helen merged in a PR that added a custom alert component. I'm okay with switching the alerts here after this pr is merged- this comment is more so a reminder to do that 🚗
| @@ -68,12 +125,17 @@ const CreateLog = () => { | |||
| }), | |||
| ); | |||
| const [building, setBuilding] = useState(""); | |||
| const [resident, setResident] = useState(""); | |||
| const [resident, setResident] = useState(-1); | |||
There was a problem hiding this comment.
why use -1 instead of an empty state?
There was a problem hiding this comment.
would agree, I think because these are all strings anyways it's easier to make them empty strings
There was a problem hiding this comment.
I wanted to store the value of each field rather than their string representation (since the string representation isn't very useful in context of the API)
There was a problem hiding this comment.
All of my comments are general non-blocking but if you don't have time we can always address them later
Would recommend just briefly testing everything works after you fix the merge conflicts since they're generationally bad (can ping me or @Safewaan to help as well)
Also make sure to update any docs on API's changed when you get the chance
Thanks for getting this in 👨🍳 🧨
| } | ||
|
|
||
| // Helper to get the currently logged in user | ||
| const getCurUserSelectOption = () => { |
There was a problem hiding this comment.
me copy pasta-ing the code below into a separate util function:
| @@ -68,12 +125,17 @@ const CreateLog = () => { | |||
| }), | |||
| ); | |||
| const [building, setBuilding] = useState(""); | |||
| const [resident, setResident] = useState(""); | |||
| const [resident, setResident] = useState(-1); | |||
There was a problem hiding this comment.
would agree, I think because these are all strings anyways it's easier to make them empty strings
| @@ -161,9 +241,9 @@ const CreateLog = () => { | |||
| }), | |||
There was a problem hiding this comment.
Also a bunch of defaulted -1's here, if there's a reason for this than you can keep it but otherwise I would make them empty strings
| const residentsData = await ResidentAPIClient.getResidents(true, 1, 1) | ||
|
|
||
| if (residentsData && residentsData.residents.length !== 0) { | ||
| // TODO: Remove the type assertions here |
There was a problem hiding this comment.
Could you clarify this?
There was a problem hiding this comment.
We're leveraging non-null type assertions on those properties (below the comment IIRC) that we should probably refactor / remove at some point ; I included them for now to a) make my life easier with some of the merge conflicts and b) workaround to speed this up ; I've already made an issue to track this here - #148
| import { getLocalStorageObjProperty } from "../utils/LocalStorageUtils"; | ||
| import baseAPIClient from "./BaseAPIClient"; | ||
|
|
There was a problem hiding this comment.
Rehomed these guys into the LogRecordAPIClient
GitHub Issue link
Create Log BE/FE Integration
Implementation description
Steps to test
What should reviewers focus on?
Checklist