-
Notifications
You must be signed in to change notification settings - Fork 166
LG-10785: Move full address entry PO search into the @18f/identity-address-search package #9234
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
Changes from all commits
d444af7
af66293
01eb255
bf875dc
ab660b0
d96c43b
d421bd5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,6 +6,7 @@ import type { LocationQuery, FormattedLocation } from '@18f/identity-address-sea | |
| import FullAddressSearchInput from './full-address-search-input'; | ||
|
|
||
| function FullAddressSearch({ | ||
| usStatesTerritories, | ||
| registerField, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I understand why you decided to pass in
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's a good point. For reference how this works in identity-idp is:
As-is we'll have to redefine them in the identity-site repo An alternative is that we could write a script to read from the Ruby file and write to a javascript file, and then publish the generated javascript file as part of an npm package. And then in the Ruby file add a file comment saying something like,
I'm going to go ahead and merge this PR. If we do refactor it in some way I think it should be part of a different ticket. But let me know your thoughts about it
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think a new ticket is a good idea, extra work and out of scope to get this working in identity-sites. I think both are fine. I don't think the list will change much over time just as long as we can think of a way to try to ensure both get updated in the future |
||
| locationsURL, | ||
| handleLocationSelect, | ||
|
|
@@ -30,6 +31,7 @@ function FullAddressSearch({ | |
| <PageHeading>{t('in_person_proofing.headings.po_search.location')}</PageHeading> | ||
| <p>{t('in_person_proofing.body.location.po_search.po_search_about')}</p> | ||
| <FullAddressSearchInput | ||
| usStatesTerritories={usStatesTerritories} | ||
| registerField={registerField} | ||
| onFoundLocations={( | ||
| address: LocationQuery | null, | ||
|
|
||
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 like that you shortened file name. Now component name is the same as the file name.