-
Notifications
You must be signed in to change notification settings - Fork 0
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
Get Buildings Route #196
Get Buildings Route #196
Conversation
Visit the preview URL for this PR (updated for commit 7b57b04): https://blueprintsupportivehousing--pr196-kelly-get-buildings-iz7aduro.web.app (expires Sun, 03 Dec 2023 19:16:03 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: f6bcdba7452bf82a6ec1a299c37d1bdff7870d09 |
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.
Very cool 😎 I found a couple nits but the main functionality works great!
Also, I noticed the notion ticket for this talks about being able to get a specific building using the id, is that no longer part of this ticket?
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.
Everything works as expected besides the edit - if you make a new resident/log record and then try and edit it, the building doesn't appear until you close the modal and open it a second time. I think I ran into this issue last term and it had to do with how I was initially setting the state when the component rendered. Should be good to after this is fixed though 👨🍳
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.
so with the log record edit, the previous bug I mentioned is still there. i left a comment on how to resolve it and then this should be good to merge
@@ -202,6 +209,7 @@ const EditLog = ({ | |||
|
|||
const initializeValues = () => { | |||
// set state variables | |||
getBuildingsOptions(); |
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.
so instead of calling this here, you should call this in LogRecordsTable.tsx
. the issue is that the state doesn't update in time, so when the building ID from the log record is being matched with these options, they don't exist yet (when you close and re-open the options have loaded ands appear)
there's a function there called getLogEntryOptions
that you can pop this in and then pass it as a prop to this component which should resolve things
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.
muy bien
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.
très bien
Notion task link
Get Buildings Route
Implementation description
BUILDINGS
variable with the buildings fetched from the databaseSteps to test
Checklist