-
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
Change address fields #117 #118
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #118 +/- ##
===========================================
- Coverage 91.91% 91.87% -0.04%
===========================================
Files 36 36
Lines 2077 2055 -22
Branches 602 610 +8
===========================================
- Hits 1909 1888 -21
+ Misses 166 165 -1
Partials 2 2 ☔ View full report in Codecov by Sentry. |
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.
A few things I noticed right now, with the types, currently if something is empty in the add dialogue e.g. the town it sends null
Ideally this would instead use undefined in typescript so that it isn't sent at all in the request. Its the same sort of thing with the edit requests.
The address display also needs some sort of logic when rendering as null fields appear like
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.
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.
Sorry I don't think the comment I made was clear on what to remove.
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.
You currently have two useState for setting the manufacturer details in the manufacturer component . You should only need one useState. (setManufacturer and setSelectedManufacturer)
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.
One last note on the types - also speaking to Joshua, we need to merge
ral-facilities/inventory-management-system-api#115 and #128 first as the user feedback is coming up.
Description
Changed address fields so that it can be used internationally. Updated tests accordingly
Testing instructions
Add a set up instructions describing how the reviewer should test the code
Agile board tracking
to be merged in only when ral-facilities/inventory-management-system-api#85 (comment) merged in
and to be merged in at the same time as ral-facilities/inventory-management-system-api#108
closes #117