-
Notifications
You must be signed in to change notification settings - Fork 4
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
51 updated api client #54
Conversation
Azure Static Web Apps: Your stage site is ready! Visit it here: https://witty-ocean-02e42dd0f-54.eastus2.azurestaticapps.net |
tags?: string; | ||
created_at: string; | ||
id: string | number; |
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.
IMPORTANT NOTE TO WHOMEVER GENERATES THE API CLIENT: this id needs to be MANUALLY altered after client generation. restful-react
types this to {}
which is not correct. The source swagger JSON has this:
id:
title: Id
anyOf:
- type: string
format: uuid
- type: integer
So basically this is an int or a guid, all good... But the {}
that gets generated basically means any non-nullish value, which is wrong. Will file an issue with restful-react
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.
This is pretty unfortunate. It looks somewhat related to this issue contiamo/restful-react#317
Is there any way we can get the backend to return a single type for this ID? It seems like it should just be a string type like the other IDs being returned in the schema.
This manual step of updating the ID in the client here will be very easy to miss.
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.
It won't be easy to miss - compilation will fail. It just won't be obvious as to how to resolve it, lol. Hrm, maybe a README update? Not perfect, but might help.
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.
Oh yeah, a failing build makes it more obvious. A readme update would probably help to resolve in the interim.
Still wondering why it's typed like this on the backend. Any insight @egamble01?
@@ -15,6 +15,7 @@ export const vaccineAvailabilityHandlers = [ | |||
inputType: 1, | |||
tags: "high-risk", | |||
created_at: "2021-05-01T21:07:28.232Z", | |||
timeslots: [], |
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.
Upcoming ticket to supply mock timeslot data should populate this
@@ -19,7 +19,7 @@ export function PharmacyList(props: Props) { | |||
error, | |||
} = useListVaccineAvailabilityApiV1VaccineAvailabilityGet({ | |||
queryParams: { | |||
postalCode: props.postalCode, | |||
postal_code: props.postalCode, |
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.
Hrm, guess we went snake case on the back end.
@@ -7,7 +7,7 @@ import Iframe from "react-iframe"; | |||
interface PharmacyProps { | |||
// Id used for creating React keys | |||
// eslint-disable-next-line react/no-unused-prop-types | |||
id: string; | |||
id: string | number; |
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.
Why do we need the ID here? I don't see it being used. The key prop that is set in the PharmacyList
should be sufficient to setting the react key.
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.
Well, the key prop is set in PharmacyList
like this:
pharmacyList.map((pharmacy) => {
return (
<PharmacyCard
key={pharmacy.id}
Address could be used instead? Dunno if phone or website would be appropriate?
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.
Ah I see now.. since pharmacyList
is depending on the props from PharmacyCard
. I guess this is ok right now. My point was more about this interface having a prop that would be never used in the component.
The ID coming from the server is probably the most appropriate key here still I'd imagine.
@@ -0,0 +1,6 @@ | |||
.wrapper { |
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.
Can we use CSS modules here to avoid global css clashing across containers and components.
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.
Wait, this shouldn't have been included at all, what the heck
UPDATE - removed as its an old source control artifact...
Azure Static Web Apps: Your stage site is ready! Visit it here: https://witty-ocean-02e42dd0f-54.eastus2.azurestaticapps.net |
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.
LGTM, left some replies, nothing blocking.
Azure Static Web Apps: Your stage site is ready! Visit it here: https://witty-ocean-02e42dd0f-54.eastus2.azurestaticapps.net |
Azure Static Web Apps: Your stage site is ready! Visit it here: https://witty-ocean-02e42dd0f-54.eastus2.azurestaticapps.net |
No description provided.