-
Notifications
You must be signed in to change notification settings - Fork 167
LG-9663 Move prepare page before location page in IPP flow - functional changes only #8308
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 7 commits
fbce6d0
6debd20
7b1d9d8
427f765
5419e34
599cf3a
ae30a8a
6fa61a3
d8e65fa
607e114
e76720e
6eddae3
7ed29d3
d08e591
fc9c1ca
a6754e7
9de64b1
11394b6
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 |
|---|---|---|
|
|
@@ -2,6 +2,7 @@ import { useState, useEffect, useCallback, useRef, useContext } from 'react'; | |
| import { useI18n } from '@18f/identity-react-i18n'; | ||
| import { Alert, PageHeading } from '@18f/identity-components'; | ||
| import { request } from '@18f/identity-request'; | ||
| import { forceRedirect } from '@18f/identity-url'; | ||
| import BackButton from './back-button'; | ||
| import AnalyticsContext from '../context/analytics'; | ||
| import AddressSearch, { | ||
|
|
@@ -12,20 +13,22 @@ import AddressSearch, { | |
| } from './address-search'; | ||
| import InPersonLocations, { FormattedLocation } from './in-person-locations'; | ||
| import { InPersonContext } from '../context'; | ||
| import UploadContext from '../context/upload'; | ||
|
|
||
| function InPersonLocationPostOfficeSearchStep({ onChange, toPreviousStep, registerField }) { | ||
| const { inPersonCtaVariantActive } = useContext(InPersonContext); | ||
| const { inPersonCtaVariantActive, inPersonURL } = useContext(InPersonContext); | ||
| const { t } = useI18n(); | ||
| const [inProgress, setInProgress] = useState<boolean>(false); | ||
| const [isLoadingLocations, setLoadingLocations] = useState<boolean>(false); | ||
| const [autoSubmit, setAutoSubmit] = useState<boolean>(false); | ||
| const { setSubmitEventMetadata } = useContext(AnalyticsContext); | ||
| const { trackEvent } = useContext(AnalyticsContext); | ||
| const [locationResults, setLocationResults] = useState<FormattedLocation[] | null | undefined>( | ||
| null, | ||
| ); | ||
| const [foundAddress, setFoundAddress] = useState<LocationQuery | null>(null); | ||
| const [apiError, setApiError] = useState<Error | null>(null); | ||
| const [disabledAddressSearch, setDisabledAddressSearch] = useState<boolean>(false); | ||
| const { flowPath } = useContext(UploadContext); | ||
|
|
||
| // ref allows us to avoid a memory leak | ||
| const mountedRef = useRef(false); | ||
|
|
@@ -40,13 +43,12 @@ function InPersonLocationPostOfficeSearchStep({ onChange, toPreviousStep, regist | |
| // useCallBack here prevents unnecessary rerenders due to changing function identity | ||
| const handleLocationSelect = useCallback( | ||
| async (e: any, id: number) => { | ||
| if (flowPath !== 'hybrid') { | ||
| e.preventDefault(); | ||
| } | ||
| const selectedLocation = locationResults![id]!; | ||
| const { streetAddress, formattedCityStateZip } = selectedLocation; | ||
| const selectedLocationAddress = `${streetAddress}, ${formattedCityStateZip}`; | ||
| setSubmitEventMetadata({ | ||
| selected_location: selectedLocationAddress, | ||
| in_person_cta_variant: inPersonCtaVariantActive, | ||
| }); | ||
| onChange({ selectedLocationAddress }); | ||
| if (autoSubmit) { | ||
| setDisabledAddressSearch(true); | ||
|
|
@@ -57,8 +59,6 @@ function InPersonLocationPostOfficeSearchStep({ onChange, toPreviousStep, regist | |
| }, 250); | ||
| return; | ||
| } | ||
| // prevent navigation from continuing | ||
| e.preventDefault(); | ||
| if (inProgress) { | ||
| return; | ||
| } | ||
|
|
@@ -69,16 +69,22 @@ function InPersonLocationPostOfficeSearchStep({ onChange, toPreviousStep, regist | |
| json: selected, | ||
| method: 'PUT', | ||
| }); | ||
| // In try block set success of request. If the request is successful, fire remaining code? | ||
| if (mountedRef.current) { | ||
| setAutoSubmit(true); | ||
| setImmediate(() => { | ||
| // continue with navigation | ||
| e.target.disabled = false; | ||
| e.target.click(); | ||
| trackEvent('IdV: location submitted', { | ||
| selected_location: selectedLocationAddress, | ||
| in_person_cta_variant: inPersonCtaVariantActive, | ||
| }); | ||
| forceRedirect(inPersonURL!); | ||
|
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 behavior of these
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. Good q. So trackEvent could get called twice, once on the typical submission event and then here at line 77. And then the forceRedirect behavior is probably being overridden.
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. Options: I could wrap the trackEvent and forceRedirect in an if check analogous to what I do for the event.preventDefault; or I could try to take advantage of the e.click() behavior that pre-existed my changes.
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 opted for wrapping the forceRedirect trackEvent calls in an if check @NavaTim |
||
| // allow process to be re-triggered in case submission did not work as expected | ||
| setAutoSubmit(false); | ||
| }); | ||
| } | ||
| } catch { | ||
| setAutoSubmit(false); | ||
| } finally { | ||
| if (mountedRef.current) { | ||
| setInProgress(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.
Is this related to what we were seeing in the hybrid test failures? Unsure what this is needed for.
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.
Yeah, so as far as I can tell this is the one change that was breaking that test. I need to do more digging, but my understanding of the React FSM is that it's listening for submit events specifically. We were preventing that and then doing a click, so I think that was it.