Skip to content

LG-9345 Maintain material consistency in IPP pages #8104

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.

Sign up for GitHub

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

Merged
JackRyan1989 merged 4 commits into from
Apr 5, 2023
Merged

LG-9345 Maintain material consistency in IPP pages #8104

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e2bef7a
Change a few links that look like buttons into buttons add link role …
jack-ryan-nava-pbc Mar 30, 2023
982303c
changelog: User-Facing Improvements, In-person proofing, Make IPP but…
jack-ryan-nava-pbc Mar 30, 2023
d9a2ea9
Remove left over comment
jack-ryan-nava-pbc Mar 30, 2023
075dfe5
Fix JS errors, replace click_link with click_button
jack-ryan-nava-pbc Mar 31, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions app/javascript/packages/document-capture/components/in-person-call-to-action.spec.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,8 @@ describe('InPersonCallToAction', () => {
</AnalyticsContextProvider>,
);

const link = getByRole('link', { name: 'in_person_proofing.body.cta.button' });
await userEvent.click(link);
const button = getByRole('button', { name: 'in_person_proofing.body.cta.button' });
await userEvent.click(button);

expect(trackEvent).to.have.been.calledWith(
'IdV: verify in person troubleshooting option clicked',
Expand Down
10 changes: 6 additions & 4 deletions app/javascript/packages/document-capture/components/in-person-call-to-action.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import { useContext } from 'react';
import { Button } from '@18f/identity-components';
import { useInstanceId } from '@18f/identity-react-hooks';
import { t } from '@18f/identity-i18n';
import useHistoryParam from '@18f/identity-form-steps/use-history-param';
import AnalyticsContext from '../context/analytics';
import { InPersonContext } from '../context';

Expand All @@ -15,6 +16,7 @@ function InPersonCallToAction({ altHeading, altPrompt, altButtonText }: InPerson
const instanceId = useInstanceId();
const { trackEvent } = useContext(AnalyticsContext);
const { inPersonCtaVariantActive } = useContext(InPersonContext);
const [, setStepName] = useHistoryParam(undefined);

return (
<section
Expand All @@ -30,13 +32,13 @@ function InPersonCallToAction({ altHeading, altPrompt, altButtonText }: InPerson
isBig
isOutline
isWide
href="#location"
className="margin-top-3 margin-bottom-1"
onClick={() =>
onClick={() => {
setStepName('location');
Copy link
Copy Markdown
Contributor

@svalexander svalexander Mar 31, 2023

Choose a reason for hiding this comment

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

i feel like this change also makes the code easier to read

trackEvent('IdV: verify in person troubleshooting option clicked', {
in_person_cta_variant: inPersonCtaVariantActive,
})
}
});
}}
>
{altButtonText || t('in_person_proofing.body.cta.button')}
</Button>
Expand Down
2 changes: 1 addition & 1 deletion ...cript/packages/document-capture/components/in-person-location-post-office-search-step.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,7 @@ function InPersonLocationPostOfficeSearchStep({ onChange, toPreviousStep, regist
address={foundAddress?.address || ''}
/>
)}
<BackButton includeBorder onClick={toPreviousStep} />
<BackButton role="link" includeBorder onClick={toPreviousStep} />
Copy link
Copy Markdown
Contributor

@aduth aduth Mar 30, 2023

Choose a reason for hiding this comment

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

Are we planning to prevent the spacebar from activating these fake links, since a link wouldn't be expected to be activated when pressing Space?

Copy link
Copy Markdown
Contributor Author

@JackRyan1989 JackRyan1989 Mar 30, 2023

Choose a reason for hiding this comment

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

Ideally no, and I don't think this alters the functionality of the HTML element, rather just how it is presented to AT. And I could've sworn spacebar still triggered the back button, but maybe I need to test again and in another browser.

Copy link
Copy Markdown
Contributor Author

@JackRyan1989 JackRyan1989 Mar 31, 2023

Choose a reason for hiding this comment

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

Ok, so I've tested the functionality of the back buttons in both Safari and Firefox and they respond to both Space and Enter/mouse click events.

Copy link
Copy Markdown
Contributor

@aduth aduth Mar 31, 2023

Choose a reason for hiding this comment

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

My expectation is that it wouldn't be activated when hitting Spacebar. It's a small detail, and maybe it ends up being helpful to have both options to activate, but if the idea with this is to make a link behave strictly as a link ought to behave, then I'd expect part of that to restrict interaction to the supported keypresses.

In general, I'm become a bit wary of this proposed behavior, but I see the upstream uswds/uswds#4385 has recently been approved, so that train may have already left the station.

Copy link
Copy Markdown
Contributor Author

@JackRyan1989 JackRyan1989 Mar 31, 2023

Choose a reason for hiding this comment

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

I mean, your point is taken. What I'm doing here isn't really material honesty either, it's a button that is masquerading as a link, but still works like a button. At least now a screen reader will match what a sighted user might assume from the element's styling.

</>
);
}
Expand Down
8 changes: 4 additions & 4 deletions app/javascript/packages/document-capture/components/in-person-prepare-step.spec.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ describe('InPersonPrepareStep', () => {
{ wrapper },
);

await userEvent.click(getByRole('link', { name: 'forms.buttons.continue' }));
await userEvent.click(getByRole('button', { name: 'forms.buttons.continue' }));
await waitFor(() => window.location.hash === inPersonURL);

expect(trackEvent).to.have.been.calledWith('IdV: prepare submitted');
Expand All @@ -60,10 +60,10 @@ describe('InPersonPrepareStep', () => {
{ wrapper },
);

const link = getByRole('link', { name: 'forms.buttons.continue' });
const button = getByRole('button', { name: 'forms.buttons.continue' });

const didFollowLinkOnFirstClick = fireEvent.click(link);
const didFollowLinkOnSecondClick = fireEvent.click(link);
const didFollowLinkOnFirstClick = fireEvent.click(button);
const didFollowLinkOnSecondClick = fireEvent.click(button);

clock.tick(delay);

Expand Down
6 changes: 3 additions & 3 deletions app/javascript/packages/document-capture/components/in-person-prepare-step.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ function InPersonPrepareStep({ toPreviousStep, value }) {
setIsSubmitting(true);
removeUnloadProtection();
await trackEvent('IdV: prepare submitted');
window.location.href = (event.target as HTMLAnchorElement).href;
window.location.href = inPersonURL!;
Copy link
Copy Markdown
Contributor

@svalexander svalexander Mar 31, 2023

Choose a reason for hiding this comment

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

i like that this is more explicit/easier to read

}
};

Expand Down Expand Up @@ -59,7 +59,7 @@ function InPersonPrepareStep({ toPreviousStep, value }) {
{flowPath === 'hybrid' && <FormStepsButton.Continue />}
{inPersonURL && flowPath === 'standard' && (
<div className="margin-y-5">
<SpinnerButton href={inPersonURL} onClick={onContinue} isBig isWide>
<SpinnerButton onClick={onContinue} isBig isWide>
{t('forms.buttons.continue')}
</SpinnerButton>
</div>
Expand All @@ -78,7 +78,7 @@ function InPersonPrepareStep({ toPreviousStep, value }) {
)}
</p>
<InPersonTroubleshootingOptions />
<BackButton includeBorder onClick={toPreviousStep} />
<BackButton role="link" includeBorder onClick={toPreviousStep} />
</>
);
}
Expand Down
2 changes: 1 addition & 1 deletion spec/features/idv/in_person_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -342,7 +342,7 @@
mock_doc_auth_attention_with_barcode
attach_and_submit_images

click_link t('in_person_proofing.body.cta.button')
click_button t('in_person_proofing.body.cta.button')
search_for_post_office
within page.first('.location-collection-item') do
click_spinner_button_and_wait t('in_person_proofing.body.location.location_button')
Expand Down
2 changes: 1 addition & 1 deletion spec/support/features/in_person_helper.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ def begin_in_person_proofing(_user = nil)
complete_doc_auth_steps_before_document_capture_step
mock_doc_auth_attention_with_barcode
attach_and_submit_images
click_link t('in_person_proofing.body.cta.button')
click_button t('in_person_proofing.body.cta.button')
end

def search_for_post_office
Expand Down