LG-3427: Update doc auth ID support texts #4228

aduth · 2020-09-22T13:05:44Z

(Partially addresses LG-3427 and LG-3521)

Why: As a user, I want to be informed of recommendations and options for ID usage in IAL2 proofing, so that I have the most chance of success.

Of the task described in LG-3427, the changes proposed here currently only update the desktop "Upload" and mobile introductory steps at the beginning of the proofing process to use updated texts. The remainder of the work will be implemented as separate pull request(s).

Screenshots:

Mobile:

Before	After

Desktop:

Before	After

config/locales/doc_auth/en.yml

aduth · 2020-09-22T13:14:33Z

spec/javascripts/packages/document-capture/context/service-provider-spec.js

+
+describe('document-capture/context/service-provider', () => {
+  it('defaults to undefined', () => {
+    const { current } = renderHook(() => useContext(ServiceProviderContext));


I've been using @testing-library/react-hooks in some side-projects, and it seems like it's a nicer way to get at the context values. I think it could be used to replace a few ad hoc approaches we've used thus far:

identity-idp/spec/javascripts/packages/document-capture/context/device-spec.jsx

Lines 6 to 12 in 9b37653

const ContextValue = () => JSON.stringify(useContext(DeviceContext));

it('defaults to an object shape of device supports', () => {

const { container } = render(<ContextValue />);

expect(container.textContent).to.equal('{"isMobile":false}');

});

identity-idp/spec/javascripts/packages/document-capture/context/acuant-spec.jsx

Lines 81 to 93 in 9b37653

render(

<AcuantContextProvider sdkSrc="about:blank">

{createElement(() => {

const { isReady, isCameraSupported } = useContext(AcuantContext);

if (isReady) {

expect(isCameraSupported).to.be.true();

}

return null;

})}

</AcuantContextProvider>,

);

anniehirshman-gsa

Screenshots look good! 👍 I had ticketed this in LG-3521 in case it needed to be addressed separately - so we can either cancel or consider it completed.

Not following the technical side of this, but @amathews-fs previously worked on a bunch of logic for this screen and might be a good reviewer? Thanks!

config/locales/doc_auth/en.yml

app/javascript/packs/document-capture.jsx

zachmargolis · 2020-09-22T17:16:31Z

app/views/idv/doc_auth/upload.html.erb

-  <div class='mt2 mb2'>
+<p>
+  <%= t('doc_auth.info.id_worn_html') %>
+  <a href="/"><%= t('doc_auth.info.accepted_ids') %></a>


Can we use the link_to helper?

Should we drop a comment or something to call it out since we don't have this URL yet? Rubocop gets mad at TODO so maybe something else?

Can we use the link_to helper?

Should we drop a comment or something to call it out since we don't have this URL yet? Rubocop gets mad at TODO so maybe something else?

Whoops, I totally overlooked this. It was (is) a work-in-progress, pending a relevant Slack question about where exactly this link should point to.

Some more quick thoughts on this:

Can we add a method in our MarketingSite class for it?

Should we expose this URL as yet another data attribute to the React app to minimize duplication?

Some more quick thoughts on this:

Can we add a method in our MarketingSite class for it?

Should we expose this URL as yet another data attribute to the React app to minimize duplication?

Yeah, I'd been waiting to see how exactly we wanted to approach this before coming to any decision on implementation. But if we're going to direct users to the marketing site, I agree it makes sense to add a method there. I'd also agree that it makes sense to expose it to the React app as a data- attribute. I'm not quite as sure how best to actually make this data available to the component, aside from introducing yet another context provider, possibly mapping directly to this MarketingSite class. It doesn't really fit well into any of the existing context values.

It's also got me thinking a bit about future re-use. The idea of a React context which captures some or all of the values from the MarketingSite class seems clean enough and ripe for reuse, though unclear right now to what extent we want to go down this path (which methods? which package? or new package(s)? etc.)

I'm not quite as sure how best to actually make this data available to the component, aside from introducing yet another context provider, possibly mapping directly to this MarketingSite class. It doesn't really fit well into any of the existing context values.

Is it worth just collapsing all our existing contexts into "DocAuthContext" or something? We only have one React app/page today?

Is there any performance penalty to having too many small contexts?

Is it worth just collapsing all our existing contexts into "DocAuthContext" or something? We only have one React app/page today?

We've finally found our way to the eternal debate of multiple contexts vs. global store (Redux) 😄 We could certainly have opted to just shove everything into a single global state object. At our current position, I'd personally still lean toward pushing forward and continuing to use distinct contexts. There's a bit more overhead with managing each one, but it forces us to be somewhat more intentional, and potentially opens up some options for reuse in the future.

Short-term, I imagine this would look like: MarketingSiteContext in the same pattern as other contexts, likely only with a single property for the "Acceptable IDs" link.

Is there any performance penalty to having too many small contexts?

Not that I'm aware of, or at least not one I'd be particularly concerned about at our scale. Usually it's the opposite, where contexts have an advantage over global store approaches where global stores cause lots of unnecessary re-renders of components which ultimately aren't concerned with the change and won't have a changed rendered result (example).

aduth · 2020-09-22T19:35:06Z

Related to the current build errors, I think with these changes we could eliminate the "No ID" error screen altogether?

app/javascript/packages/document-capture/hooks/use-i18n.js

zachmargolis · 2020-09-22T20:21:47Z

app/javascript/packages/document-capture/components/mobile-intro-step.jsx

+        {formatHTML(t('doc_auth.info.id_worn_html'), {
+          strong: 'strong',
+        })}{' '}
+        <a href="/">{t('doc_auth.info.accepted_ids')}</a>


just wanted to highlight this as another pending URL we'll need to change

@aduth following up on the "Full list of state-issued IDs" link - since this URL/content doesn't currently exist, we can omit it for now, and add it to this screen as an improvement when it's ready. Thanks!

aduth · 2020-09-23T12:53:55Z

Related to the current build errors, I think with these changes we could eliminate the "No ID" error screen altogether?

Removed in bf68bee4f.

zachmargolis

LGTM! hopefully we can get those "list of acceptable IDs" links working before we merge

spec/javascripts/packages/document-capture/hooks/use-i18n-spec.jsx

See: https://github.com/18F/identity-idp/pull/4228/files#r492903440

**Why**: Improved reuse of strings between server-side and client-side

aduth · 2020-09-24T13:30:58Z

Pending the development of a "Full list of accepted IDs" resource, I've removed the link in be8dac4.

aduth requested a review from anniehirshman-gsa September 22, 2020 13:05

aduth commented Sep 22, 2020

View reviewed changes

config/locales/doc_auth/en.yml Outdated Show resolved Hide resolved

aduth commented Sep 22, 2020

View reviewed changes

anniehirshman-gsa reviewed Sep 22, 2020

View reviewed changes

zachmargolis reviewed Sep 22, 2020

View reviewed changes

app/javascript/packages/document-capture/hooks/use-i18n.js Show resolved Hide resolved

zachmargolis reviewed Sep 22, 2020

View reviewed changes

zachmargolis approved these changes Sep 23, 2020

View reviewed changes

spec/javascripts/packages/document-capture/hooks/use-i18n-spec.jsx Show resolved Hide resolved

aduth mentioned this pull request Sep 23, 2020

Use react-hooks-testing-library to test React context, hooks #4234

Merged

aduth force-pushed the aduth-id-worn-help branch from 139f078 to af70499 Compare September 23, 2020 18:54

aduth added 10 commits September 24, 2020 09:22

Support string tagName for formatHTML handler

ef652da

Support variables substitution in i18n strings

1a64600

Update doc auth ID support texts

3307aac

Remove unused string idv.messages.jurisdiction.no_id

6322528

Remove unused "No ID" error screen

3355460

Remove unnecessary HTML suffix for plaintext use_cac string

2d63003

Extract getServiceProvider to conditionally create SP object

05fb290

See: https://github.com/18F/identity-idp/pull/4228/files#r492903440

Support including attributes in match in formatHTML

f0d997b

**Why**: Improved reuse of strings between server-side and client-side

Substitute no_other_id_help_bold with no_other_id_help_bold_html

56a814b

Remove link "Full list of accepted IDs"

be8dac4

aduth force-pushed the aduth-id-worn-help branch from af70499 to be8dac4 Compare September 24, 2020 13:29

aduth merged commit 7d5d77a into master Sep 24, 2020

aduth deleted the aduth-id-worn-help branch September 24, 2020 15:39

This was referenced Sep 30, 2020

LG-3427: Add ial2Strict to ServiceProviderContext to conditionally hide selfie in "Review Issues" step #4250

Merged

LG-3427: Show field errors at field, unknown errors before step title #4256

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

LG-3427: Update doc auth ID support texts #4228

LG-3427: Update doc auth ID support texts #4228

aduth commented Sep 22, 2020 •

edited

Loading

aduth Sep 22, 2020

anniehirshman-gsa left a comment

zachmargolis Sep 22, 2020

aduth Sep 22, 2020

zachmargolis Sep 23, 2020

aduth Sep 23, 2020

zachmargolis Sep 23, 2020

aduth Sep 23, 2020

aduth commented Sep 22, 2020

zachmargolis Sep 22, 2020

anniehirshman-gsa Sep 24, 2020

aduth commented Sep 23, 2020

zachmargolis left a comment

aduth commented Sep 24, 2020

	const ContextValue = () => JSON.stringify(useContext(DeviceContext));

	it('defaults to an object shape of device supports', () => {
	const { container } = render(<ContextValue />);

	expect(container.textContent).to.equal('{"isMobile":false}');
	});

	render(
	<AcuantContextProvider sdkSrc="about:blank">
	{createElement(() => {
	const { isReady, isCameraSupported } = useContext(AcuantContext);

	if (isReady) {
	expect(isCameraSupported).to.be.true();
	}

	return null;
	})}
	</AcuantContextProvider>,
	);

LG-3427: Update doc auth ID support texts #4228

LG-3427: Update doc auth ID support texts #4228

Conversation

aduth commented Sep 22, 2020 • edited Loading

Choose a reason for hiding this comment

anniehirshman-gsa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aduth commented Sep 22, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aduth commented Sep 23, 2020

zachmargolis left a comment

Choose a reason for hiding this comment

aduth commented Sep 24, 2020

aduth commented Sep 22, 2020 •

edited

Loading