Skip to content

LG-9663 Move prepare page before location page in IPP flow - functional changes only#8308

Merged
JackRyan1989 merged 18 commits intomainfrom
jryan/lg-8948-move-prepare-page-before-location
May 5, 2023
Merged

LG-9663 Move prepare page before location page in IPP flow - functional changes only#8308
JackRyan1989 merged 18 commits intomainfrom
jryan/lg-8948-move-prepare-page-before-location

Conversation

@JackRyan1989
Copy link
Contributor

@JackRyan1989 JackRyan1989 commented Apr 28, 2023

🎫 Ticket

🛠 Summary of changes

The prepare page now appears before the location page in the IPP flow. Changes made to React application and RSpec feature tests where necessary.

📜 Testing Plan

  • Confirm that the Prepare Page is visited before the Location page in the IPP flow
  • Confirm that you can navigate back to the Prepare Page from the Location Page
  • Confirm that the Location Page navigates to the State ID step on submit

👀 Screenshots - see PR 8312

@JackRyan1989 JackRyan1989 requested review from a team April 28, 2023 21:09
@JackRyan1989 JackRyan1989 self-assigned this Apr 28, 2023
@JackRyan1989 JackRyan1989 requested review from NavaTim and rutvigupta-design and removed request for a team April 28, 2023 21:09
@kellular kellular requested review from kellular and removed request for rutvigupta-design April 28, 2023 21:22
@JackRyan1989 JackRyan1989 changed the title LG 8948 Move prepare page before location page in IPP flow LG-9663 Move prepare page before location page in IPP flow - functional changes only May 2, 2023
@carmenrosalop carmenrosalop requested review from carmenrosalop and removed request for kellular May 2, 2023 18:10
@carmenrosalop
Copy link

@JackRyan1989 could you please update the screen shots? it seems like the version in Spanish for the PO search doesn't show the updates

@JackRyan1989
Copy link
Contributor Author

@JackRyan1989 could you please update the screen shots? it seems like the version in Spanish for the PO search doesn't show the updates

@carmenrosalop can you please check in the content changes only Pull Request? I'm sorry for the confusion!

@carmenrosalop carmenrosalop requested review from kellular and removed request for carmenrosalop May 2, 2023 18:54
Copy link

@kellular kellular left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Jack walked me through the updated flow. LGTM!

Comment on lines 53 to 58
{flowPath === 'hybrid' && <FormStepsButton.Continue />}
{inPersonURL && flowPath === 'standard' && (
<div className="margin-y-5">
<SpinnerButton onClick={onContinue} isBig isWide>
<SpinnerButton type="submit" onClick={onContinue} isBig isWide>
{t('forms.buttons.continue')}
</SpinnerButton>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we able to simplify this logic and use FormStepsButton.Continue for both cases, esp. now that this page will not direct the user outside the flow?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think so. I have some changes that reduce the complexity here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated @NavaTim

Comment on lines +77 to +81
trackEvent('IdV: location submitted', {
selected_location: selectedLocationAddress,
in_person_cta_variant: inPersonCtaVariantActive,
});
forceRedirect(inPersonURL!);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the behavior of these trackEvent and forceRedirect calls during hybrid flow?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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

Comment on lines +46 to +48
if (flowPath !== 'hybrid') {
e.preventDefault();
}
Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor

@allthesignals allthesignals left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering what was the source of our testing woes with the hybrid flow? I see a new check for the hybrid flowPath... seems related.

Thanks for shepherding this through! Looks great.

selected_location: selectedLocationAddress,
in_person_cta_variant: inPersonCtaVariantActive,
});
forceRedirect(inPersonURL!);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, was this the helper you mentioned earlier?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Um, I don't think so as Tomas was the one who added this bit in. I think I was referring to const [, setStepName] = useHistoryParam(undefined);

Comment on lines +44 to +47
{inPersonURL && flowPath === 'standard' ? (
<FormStepsButton.Continue className="margin-y-5" />
) : (
<FormStepsButton.Continue />
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

@JackRyan1989 JackRyan1989 merged commit 572b64a into main May 5, 2023
@JackRyan1989 JackRyan1989 deleted the jryan/lg-8948-move-prepare-page-before-location branch May 5, 2023 18:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants