-
Notifications
You must be signed in to change notification settings - Fork 115
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
LG-11082 - Add optional info alert to FullAddressSearch #9276
LG-11082 - Add optional info alert to FullAddressSearch #9276
Conversation
…sultsHeaderComponent prop onto FullAddressSearch so it can be passed in from help center
<PageHeading>{t('in_person_proofing.headings.po_search.location')}</PageHeading> | ||
<p>{t('in_person_proofing.body.location.po_search.po_search_about')}</p> | ||
</> | ||
)} |
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.
Although we are not using AddressSearch at the moment, we will be. I wanted to update this component to have the same logic as FullAddressSearch so they behave similar. That will make switching over easier
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 would be good if this change was covered with a spec, but probably of limited value until we get this back into use.
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.
I added tests around this show/hide logic with commit d3f6b40
@@ -21,9 +71,9 @@ describe('FullAddressSearch', () => { | |||
usStatesTerritories={usStatesTerritories} | |||
onFoundLocations={handleLocationsFound} | |||
locationsURL={locationsURL} |
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.
I created interface FullAddressSearchProps and as a result had to update these tests to satisfy the data type- that is the reason for the following updates in this test file
<PageHeading>{t('in_person_proofing.headings.po_search.location')}</PageHeading> | ||
<p>{t('in_person_proofing.body.location.po_search.po_search_about')}</p> | ||
</> | ||
)} |
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.
noInPersonLocationsDisplay: ComponentType<{ address: string }>; | ||
resultsHeaderComponent?: ComponentType; | ||
} | ||
|
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 like we had the InPersonLocationsProps interface in both types.d and in in-person-locations.tsx. I deleted this one and updated the one in types.d to have the complete prop list.
@@ -1,6 +1,6 @@ | |||
{ | |||
"name": "@18f/identity-address-search", | |||
"version": "3.0.0", | |||
"version": "3.1.0", | |||
"type": "module", | |||
"private": false, |
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.
My changes should not be breaking so I went with a minor update. Please confirm you agree, not breaking.
…82-UpdateFullAddressSearchWithResultsHeaderComponent
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.
My comments are non-blocking since they pertain to minor code quality issues, but I highly recommend implementing the suggested changes before merging.
locationsURL: string; | ||
usStatesTerritories: [string, string][]; | ||
registerField?: RegisterFieldCallback; | ||
usStatesTerritories: Array<Array<string>>; |
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.
Is there a reason we're making the type of usStatesTerritories
less specific? In cases like this, where the string at index zero and index one have specific meaning (abbreviation vs name), using the stricter type can be helpful in ensuring that the value is correctly used.
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.
@NavaTim I modified this value as I was getting errors with newly created props on FullAddressSearch. I am not able to resolve errors with this data type but was able to with string[][]
.
I had to change it for FullAddressSearchProps for usStateTerritories as well. Perhaps we can jump on a call if you have availability if you still think I should stick with [string, string][]
.
app/javascript/packages/address-search/components/full-address-search.spec.tsx
Outdated
Show resolved
Hide resolved
interface FullAddressSearchProps { | ||
disabled: boolean; | ||
handleLocationSelect: ((e: any, id: number) => Promise<void>) | null | undefined; | ||
locationsURL: string; | ||
noInPersonLocationsDisplay?: ComponentType<{ address: string }>; | ||
onFoundLocations: Dispatch<SetStateAction<FormattedLocation[] | null | undefined>>; | ||
registerField: RegisterFieldCallback; | ||
resultsHeaderComponent?: ComponentType; | ||
usStatesTerritories: Array<Array<string>>; | ||
} |
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 looks like a good use case for extending types since many of the properties are shared between these 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.
This is a great idea!
I looked into using this with FullAddressSearchProps - with AddressSearchProps as the extend but there is a prop on AddressSearchProps not needed by FullAddressSearchProp. I can create a third prop category and then extend it for both AddressSearchProp and FullAddressSearchProp listing only the unique props needed but think although nearly duplicate, keeping it as is might be best for readability and if we decide to remove on or the other down the road keeps it separate. Same with FullAddressSearchInputProps (extending FullAddressSearchProps).
<PageHeading>{t('in_person_proofing.headings.po_search.location')}</PageHeading> | ||
<p>{t('in_person_proofing.body.location.po_search.po_search_about')}</p> | ||
</> | ||
)} |
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 would be good if this change was covered with a spec, but probably of limited value until we get this back into use.
…82-UpdateFullAddressSearchWithResultsHeaderComponent
…82-UpdateFullAddressSearchWithResultsHeaderComponent
Published to |
🎫 Ticket
LG-11082 Refactor identity-site to use FullAddressSearch
Help Center Figma Mocks
🛠 Summary of changes
"@18f/identity-address-search
from3.0.0
to3.1.0
handleLocationSelect
optional prop toFullAddressSearch
and passed it intoInPersonLocations
FullAddressSearchProps
interface listFullAddressSearchInput
to includeInput
to avoid duplicate prop interface name along with alphabetized the arg listFullAddressSearch
andAddressSearch
to only display the Page Header and PO Search About Text when handleLocationsSelect is not null.📜 Testing Plan
Step 1: Running the application locally, enter the in-person proofing flow and confirm that the location results display as expected, specifically that no info alert displays, and that the "Select a Post Office..." message displays, along with the Select button for each post office returned in the list. This will be the view/flow from Login.gov
Step 2: This will be the view/flow from the Help Center. Edit
app/javascript/packages/document-capture/components/in-person-location-full-address-entry-post-office-search-step.tsx
.Add the Alert import near the top of the file, update the arg for handleLocationSelect to be null, pass in the resultsHeaderComponent prop and component to FullAddressSearch (See code snipped below 👇 )
👀 Screenshots
If relevant, include a screenshot or screen capture of the changes.
no changes
Ensure all behavior is working as you'd expect for Login.gov
PO Help Center (with no logic to Hide Header and PO Search About Text)
This is what Help Center looks like currently when using FullAddressSearch (not the version in this PR)
showing info alert and no "Select" messages or buttons (English)
Ensure all behavior is working as you'd expect (to be used in the Help Center)