-
Notifications
You must be signed in to change notification settings - Fork 167
LG-7927 AddressSearch (draft) component relays results to USPS location search #7385
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 29 commits
58f26ff
14924cc
5f5d5b4
7eb507d
5caebcd
09e5d77
a0ae26a
0c4f507
93e8871
b872679
43cfed2
f715f35
01a3538
842085b
4f27e91
2459a3e
f7e221a
9a53438
e450a67
fe0ae4f
cadd296
62e5fff
42f31ec
d71c237
01b0edd
a134f67
f4f5aaa
0d4684e
279762c
f2608d7
011a2bd
8efd0f7
ea0895a
c48bd3f
7430c7c
1a673cc
6d7107c
eceb0ba
3534a2a
0fbf794
fca9490
e619635
2f0997b
f1ca342
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 |
|---|---|---|
| @@ -0,0 +1,48 @@ | ||
| import { createRef } from 'react'; | ||
|
allthesignals marked this conversation as resolved.
|
||
| import { render } from '@testing-library/react'; | ||
| import TextInput from './text-input'; | ||
|
|
||
| describe('TextInput', () => { | ||
| it('renders with an associated label', () => { | ||
| const { getByLabelText } = render(<TextInput label="Input" />); | ||
|
|
||
| const input = getByLabelText('Input'); | ||
|
|
||
| expect(input).to.be.an.instanceOf(HTMLInputElement); | ||
| expect(input.classList.contains('usa-input')).to.be.true(); | ||
| }); | ||
|
|
||
| it('uses an explicitly-provided ID', () => { | ||
| const customId = 'custom-id'; | ||
| const { getByLabelText } = render(<TextInput label="Input" id={customId} />); | ||
|
|
||
| const input = getByLabelText('Input'); | ||
|
|
||
| expect(input.id).to.equal(customId); | ||
| }); | ||
|
|
||
| it('applies additional given classes', () => { | ||
| const customClass = 'custom-class'; | ||
| const { getByLabelText } = render(<TextInput label="Input" className={customClass} />); | ||
|
|
||
| const input = getByLabelText('Input'); | ||
|
|
||
| expect([...input.classList.values()]).to.have.all.members(['usa-input', customClass]); | ||
| }); | ||
|
|
||
| it('applies additional input attributes', () => { | ||
| const type = 'password'; | ||
| const { getByLabelText } = render(<TextInput label="Input" type={type} />); | ||
|
|
||
| const input = getByLabelText('Input') as HTMLInputElement; | ||
|
|
||
| expect(input.type).to.equal(type); | ||
| }); | ||
|
|
||
| it('forwards ref', () => { | ||
| const ref = createRef<HTMLInputElement>(); | ||
| render(<TextInput label="Input" ref={ref} />); | ||
|
|
||
| expect(ref.current).to.be.an.instanceOf(HTMLInputElement); | ||
| }); | ||
| }); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,40 @@ | ||
| import { forwardRef } from 'react'; | ||
|
allthesignals marked this conversation as resolved.
|
||
| import type { InputHTMLAttributes, ForwardedRef } from 'react'; | ||
| import { useInstanceId } from '@18f/identity-react-hooks'; | ||
|
|
||
| export interface TextInputProps extends InputHTMLAttributes<HTMLInputElement> { | ||
| /** | ||
| * Text of label associated with input. | ||
| */ | ||
| label: string; | ||
|
|
||
| /** | ||
| * Optional explicit ID to use in place of default behavior. | ||
| */ | ||
| id?: string; | ||
|
|
||
| /** | ||
| * Additional class name to be applied to the input element. | ||
| */ | ||
| className?: string; | ||
| } | ||
|
|
||
| function TextInput( | ||
| { label, id, className, ...inputProps }: TextInputProps, | ||
| ref: ForwardedRef<HTMLInputElement>, | ||
| ) { | ||
| const instanceId = useInstanceId(); | ||
| const inputId = id ?? `text-input-${instanceId}`; | ||
| const classes = ['usa-input', className].filter(Boolean).join(' '); | ||
|
|
||
| return ( | ||
| <> | ||
| <label className="usa-label" htmlFor={inputId}> | ||
| {label} | ||
| </label> | ||
| <input ref={ref} className={classes} id={inputId} {...inputProps} /> | ||
| </> | ||
| ); | ||
| } | ||
|
|
||
| export default forwardRef(TextInput); | ||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,51 @@ | ||||||
| import { TextInput, Button } from '@18f/identity-components'; | ||||||
| import { request } from '@18f/identity-request'; | ||||||
| import { useState, useCallback, ChangeEvent } from 'react'; | ||||||
|
|
||||||
| interface Location { | ||||||
| street_address: string; | ||||||
| city: string; | ||||||
| state: string; | ||||||
| zip_code: string; | ||||||
|
allthesignals marked this conversation as resolved.
|
||||||
| address: string; | ||||||
| } | ||||||
|
|
||||||
| interface AddressSearchProps { | ||||||
| onAddressFound?: (location: Location) => void; | ||||||
|
allthesignals marked this conversation as resolved.
|
||||||
| } | ||||||
|
|
||||||
| export const ADDRESS_SEARCH_URL = '/api/addresses'; | ||||||
|
|
||||||
| function AddressSearch({ onAddressFound = () => {} }: AddressSearchProps) { | ||||||
| const [unvalidatedAddressInput, setUnvalidatedAddressInput] = useState(''); | ||||||
| const [addressQuery, setAddressQuery] = useState({} as Location); | ||||||
| const handleAddressSearch = useCallback(async () => { | ||||||
| const addressCandidates = await request(ADDRESS_SEARCH_URL, { | ||||||
|
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. For future consideration: I think it'd be valuable if One way I could see this working is to accept a generic parameter to
Suggested change
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. What's the effect if a network disconnect causes a
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. Probably something bad! I think the work we'll do in parallel on state management (Andrew's suggestion) involving SWR or something else will help standardize our network interactions... |
||||||
| method: 'POST', | ||||||
| headers: { 'Content-Type': 'application/json' }, | ||||||
| json: { address: unvalidatedAddressInput }, | ||||||
| }); | ||||||
|
|
||||||
| const [bestMatchedAddress] = addressCandidates; | ||||||
|
allthesignals marked this conversation as resolved.
Outdated
|
||||||
| setAddressQuery(bestMatchedAddress); | ||||||
| onAddressFound(bestMatchedAddress); | ||||||
| }, [unvalidatedAddressInput]); | ||||||
|
|
||||||
| return ( | ||||||
| <> | ||||||
| <TextInput | ||||||
| value={unvalidatedAddressInput} | ||||||
| onChange={(event: ChangeEvent) => { | ||||||
| const target = event.target as HTMLInputElement; | ||||||
|
|
||||||
| setUnvalidatedAddressInput(target.value); | ||||||
| }} | ||||||
| label="Search for an address" | ||||||
| /> | ||||||
| <Button onClick={() => handleAddressSearch()}>Search</Button> | ||||||
|
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. To make the most of
Suggested change
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. Just want to note that this is going to be very prone to race conditions if the user submits multiple times, as the results may arrive out of order form when they were sent. We should either cancel/nullify previous requests like we're doing in the other component, or consider a library like
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.
I'd like to dive deeper into understanding this because in my code I'm defining and memoizing handleAddressSearch once. The anonymous function here calls the memoized function, but React doesn't know how to track it because it's an anonymous function? Something like that?
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. Yeah, the anonymous function will create a new function each time, and because IIRC the default behavior is that React will always reconcile anyways unless descendent components are marked as pure components, and our usage of React is fairly minimal (we're not a single-page app), so I typically choose not to worry too much about Rough demo here: https://codepen.io/aduth/pen/eYKPZqL
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. The suggestion was applied, but just want to resurface the race condition concern from my other comment in this thread. I wouldn't necessarily consider it blocking, but I definitely expect it to produce some inconsistent buggy behavior.
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.
@aduth 100%, yep! thank you. i pinged some folks to make sure I do that in the next PRs. |
||||||
| <>{addressQuery.address}</> | ||||||
| </> | ||||||
| ); | ||||||
| } | ||||||
|
|
||||||
| export default AddressSearch; | ||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,10 +1,13 @@ | ||
| import { useState, useEffect, useCallback, useRef, useContext } from 'react'; | ||
| import { useI18n } from '@18f/identity-react-i18n'; | ||
| import { PageHeading, SpinnerDots } from '@18f/identity-components'; | ||
| import { request } from '@18f/identity-request'; | ||
| import BackButton from './back-button'; | ||
| import LocationCollection from './location-collection'; | ||
| import LocationCollectionItem from './location-collection-item'; | ||
| import AnalyticsContext from '../context/analytics'; | ||
| import AddressSearch from './address-search'; | ||
| import AppContext from '../context/app'; | ||
|
|
||
| interface PostOffice { | ||
|
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. should this interface match the postoffice struct?
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. hmm i'm not sure. there was some history here... the naming is pretty confusing. i need to go through and rename a bunch of things.
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. the other struct has some other fields that aren't in this interface. I think we'd need those same fields here in order to display all the information that we display for each location on the location page. |
||
| address: string; | ||
|
|
@@ -29,64 +32,20 @@ interface FormattedLocation { | |
| sundayHours: string; | ||
| weekdayHours: string; | ||
| } | ||
|
|
||
| interface RequestOptions { | ||
| /** | ||
| * Whether to send the request as a JSON request. Defaults to true. | ||
| */ | ||
| json?: boolean; | ||
|
|
||
| /** | ||
| * Whether to include CSRF token in the request. Defaults to true. | ||
| */ | ||
| csrf?: boolean; | ||
|
|
||
| /** | ||
| * Optional. HTTP verb used. Defaults to GET. | ||
| */ | ||
| method?: string; | ||
| interface LocationQuery { | ||
|
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. since the locationQuery and location interfaces have the same fields can we re-use one interface by pulling into a separate file?
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. Yes, let me look into it. It's very confusing (the naming is pretty bad). |
||
| streetAddress: string; | ||
| city: string; | ||
| state: string; | ||
| zipCode: string; | ||
| } | ||
|
|
||
| const DEFAULT_FETCH_OPTIONS = { csrf: true, json: true }; | ||
|
|
||
| const getCSRFToken = () => | ||
| document.querySelector<HTMLMetaElement>('meta[name="csrf-token"]')?.content; | ||
|
|
||
| const request = async ( | ||
| url: string, | ||
| body: BodyInit | object, | ||
| options: Partial<RequestOptions> = {}, | ||
| ) => { | ||
| const headers: HeadersInit = {}; | ||
| const mergedOptions: Partial<RequestOptions> = { | ||
| ...DEFAULT_FETCH_OPTIONS, | ||
| ...options, | ||
| }; | ||
|
|
||
| if (mergedOptions.csrf) { | ||
| const csrf = getCSRFToken(); | ||
| if (csrf) { | ||
| headers['X-CSRF-Token'] = csrf; | ||
| } | ||
| } | ||
|
|
||
| if (mergedOptions.json) { | ||
| headers['Content-Type'] = 'application/json'; | ||
| body = JSON.stringify(body); | ||
| } | ||
|
|
||
| const response = await window.fetch(url, { | ||
| method: mergedOptions.method, | ||
| headers, | ||
| body: body as BodyInit, | ||
| }); | ||
|
|
||
| return mergedOptions.json ? response.json() : response.text(); | ||
| }; | ||
|
|
||
| export const LOCATIONS_URL = '/verify/in_person/usps_locations'; | ||
|
|
||
| const getUspsLocations = () => request(LOCATIONS_URL, {}, { method: 'post' }); | ||
| const getUspsLocations = (address) => | ||
| request(LOCATIONS_URL, { | ||
| method: 'post', | ||
| json: { address }, | ||
| }); | ||
|
|
||
| const formatLocation = (postOffices: PostOffice[]) => { | ||
| const formattedLocations = [] as FormattedLocation[]; | ||
|
|
@@ -124,10 +83,12 @@ const prepToSend = (location: object) => { | |
| function InPersonLocationStep({ onChange, toPreviousStep }) { | ||
| const { t } = useI18n(); | ||
| const [locationData, setLocationData] = useState([] as FormattedLocation[]); | ||
| const [foundAddress, setFoundAddress] = useState({} as LocationQuery); | ||
| const [inProgress, setInProgress] = useState(false); | ||
| const [autoSubmit, setAutoSubmit] = useState(false); | ||
| const [isLoadingComplete, setIsLoadingComplete] = useState(false); | ||
| const { setSubmitEventMetadata } = useContext(AnalyticsContext); | ||
| const { arcgisSearchEnabled } = useContext(AppContext); | ||
|
allthesignals marked this conversation as resolved.
Outdated
|
||
|
|
||
| // ref allows us to avoid a memory leak | ||
| const mountedRef = useRef(false); | ||
|
|
@@ -156,7 +117,8 @@ function InPersonLocationStep({ onChange, toPreviousStep }) { | |
| } | ||
| const selected = prepToSend(selectedLocation); | ||
| setInProgress(true); | ||
| await request(LOCATIONS_URL, selected, { | ||
| await request(LOCATIONS_URL, { | ||
| json: selected, | ||
| method: 'PUT', | ||
| }) | ||
| .then(() => { | ||
|
|
@@ -181,26 +143,35 @@ function InPersonLocationStep({ onChange, toPreviousStep }) { | |
| [locationData, inProgress], | ||
| ); | ||
|
|
||
| const handleFoundAddress = useCallback((address) => { | ||
| setFoundAddress({ | ||
| streetAddress: address.street_address, | ||
| city: address.city, | ||
| state: address.state, | ||
| zipCode: address.zip_code, | ||
| }); | ||
| }, []); | ||
|
allthesignals marked this conversation as resolved.
|
||
|
|
||
| useEffect(() => { | ||
| let mounted = true; | ||
| let didCancel = false; | ||
| (async () => { | ||
| try { | ||
| const fetchedLocations = await getUspsLocations(); | ||
| const fetchedLocations = await getUspsLocations(prepToSend(foundAddress)); | ||
|
|
||
| if (mounted) { | ||
| if (!didCancel) { | ||
| const formattedLocations = formatLocation(fetchedLocations); | ||
| setLocationData(formattedLocations); | ||
| } | ||
| } finally { | ||
| if (mounted) { | ||
| if (!didCancel) { | ||
| setIsLoadingComplete(true); | ||
| } | ||
| } | ||
| })(); | ||
| return () => { | ||
| mounted = false; | ||
| didCancel = true; | ||
| }; | ||
| }, []); | ||
| }, [foundAddress]); | ||
|
allthesignals marked this conversation as resolved.
|
||
|
|
||
| let locationsContent: React.ReactNode; | ||
| if (!isLoadingComplete) { | ||
|
|
@@ -230,6 +201,7 @@ function InPersonLocationStep({ onChange, toPreviousStep }) { | |
| return ( | ||
| <> | ||
| <PageHeading>{t('in_person_proofing.headings.location')}</PageHeading> | ||
| {arcgisSearchEnabled && <AddressSearch onAddressFound={handleFoundAddress} />} | ||
|
allthesignals marked this conversation as resolved.
|
||
| <p>{t('in_person_proofing.body.location.location_step_about')}</p> | ||
| {locationsContent} | ||
| <BackButton onClick={toPreviousStep} /> | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.