-
Notifications
You must be signed in to change notification settings - Fork 14
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
#1658 - Address fields display for institutions and ministry users #2070
Conversation
* address component represents an entry in the array. | ||
* @param address address to be formatted. | ||
* @returns string array with ordered address components. | ||
* @example |
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.
👌
// Address line 2 | ||
formattedAddress.push(address.addressLine2); | ||
} | ||
// Country, province, postal code. |
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.
// City, ... ?
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.
Adjusted.
// Country, province, postal code. | ||
const cityPostalCountry: string[] = []; | ||
cityPostalCountry.push(address.city); | ||
if (address.provinceState) { |
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.
Just for my info: I see that provinceState
is optional in Address
. Shouldn't it be mandatory?
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 is not mandatory because the addressees outside Canada when we do not capture this information.
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.
Looks good. Just minor comments.
:key="addressLine" | ||
data-cy="institutionAddress1" | ||
> | ||
{{ addressLine }} | ||
</span> | ||
</v-col> | ||
|
||
<!-- Address 2 --> | ||
<v-col> |
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.
for my info, so, there won't be address 2 ?
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.
Not as a separate column. It was supposed to be the "Address Line 2" (if I am not wrong).
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.
mm, actually I think, the old plan was that there can be 2 addresses for the institution (although we don't have anything in place to capture the 2nd address now), Address 1
and Address 2
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.
Yes, maybe that also. There was a plan for a sort of home address
/mailing address
but it was dropped a long time ago.
Either way, seems to be pretty simple now.
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. just a small question
Kudos, SonarCloud Quality Gate passed!
|
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.
👍
orderBy
to the query returning locations because every time that one item was edited they kept switching orders.