LG-3427: Size desktop selfie capture aligned to ID inputs in review step #4260

aduth · 2020-10-02T13:43:11Z

Why: Per design, for consistent width of inputs on Review Issues step.

Implementation Notes:

Inline styles on a wrapper element is not an ideal implementation here, though it seemed more appealing than one of the obvious alternatives:

BEM modifier class selfie-capture--size-as-id-input, applied either via prop className or isSizedAsIdInput prop on SelfieCapture.
SelfieCapture supports style prop, applying inline style instead of wrapper element.

Problem in both of these is that this requirement feels very use-case specific to bake into SelfieCapture.

Before	After

anniehirshman-gsa

Not sure about the implementation notes, but screenshot looks good 👍

Two favors: 1) check that mobile is unaffected (should span full width minus L/R margin), and 2) check that the live camera capture is still larger when user clicks retake photo? Thanks!

zachmargolis

LGTM

zachmargolis · 2020-10-02T13:58:17Z

app/javascript/packages/document-capture/components/review-issues-step.jsx

-              onChange={(nextSelfie) => onChange({ selfie: nextSelfie })}
-              errorMessage={selfieError ? <FormErrorMessage error={selfieError} /> : undefined}
-            />
+            <div style={{ maxWidth: '375px' }}>


Inline styles on a wrapper element is not an ideal implementation here, though it seemed more appealing than one of the obvious alternatives:

BEM modifier class selfie-capture--size-as-id-input, applied either via prop className or isSizedAsIdInput prop on SelfieCapture.

SelfieCapture supports style prop, applying inline style instead of wrapper element.

Instead of BEM style, could we just define something like .image-input? and apply that to all 3 input fields? Then it would have the max-width bit?

I don't think the style attribute will be allowed by the CSP. This would need to be nonced in order to work.

I don't think the style attribute will be allowed by the CSP. This would need to be nonced in order to work.

Hm, I was a bit confused by this, since we have inline styles elsewhere already, notably in the AcuantCaptureCanvas component, which appear to work just fine.

I dug into this a bit more, and it appears that it's possibly fine because of how React is actually assigning these styles using the style property.

Related: https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Security-Policy/style-src

However, styles properties that are set directly on the element's style property will not be blocked, allowing users to safely manipulate styles via JavaScript:

document.querySelector('div').style.display = 'none';

That being said, I like @zachmargolis's suggestion of a class here anyways, so I'll plan to move away from the inline styles here. It might be worth considering for AcuantCaptureCanvas as well, just to avoid any potential future conflicts with CSP (including possible changes in the spec and/or browser behavior).

aduth · 2020-10-02T16:48:19Z

Pulling out the discussion from #4260 (comment) :

Part of the reason I've leaned on inline styles is that it's not entirely obvious the best place to put stylesheets for some of these domain-specific components (e.g. components under the umbrella of "document capture").

In bc93ce4d01c6e403e744bcee997c69289793fd95, I consider allowing stylesheets to be defined adjacent to the component JavaScript, based partly on some guidance from Webpacker documentation.

I think this could work well as a pattern for defining styles for individual components, but there's a few considerations:

It does fragment the stylesheets to an extent, where otherwise one could assume to find all stylesheets within app/assets/stylesheets. On the other hand, it fits well into the model of Yarn workspaces and considering these as independent packages.
- Aside: On a related note, I've been struggling with how tests for components are so far in the filesystem from where the component is implemented, and would find some convenience in similarly locating them together.
We might want to have some clear guidance around naming conventions for component styles, to reduce the likelihood of conflicts. I've previously had fairly good scalable success with a [package-name]-[component-name][__element][--modifier] convention, which I've adapted here.

I'm not set on this approach, so any concerns are welcomed!

aduth · 2020-10-02T19:42:32Z

Not sure about the implementation notes, but screenshot looks good 👍

Thanks for looking!

Two favors: 1) check that mobile is unaffected (should span full width minus L/R margin),

The width constraint is applied as a max-width, so it should show to at least 375px, which is the default behavior for other fields as well. We could also consider changing this value if we want to show 100% width up to a predefined viewport size.

iPhone X Screenshot Example: localhost_3000_verify_doc_auth_document_capture(iPhone X)

and 2) check that the live camera capture is still larger when user clicks retake photo? Thanks!

This should be updated in 24d8596.

Photo Selected	Retaking Photo

anniehirshman-gsa · 2020-10-02T19:55:58Z

Great thanks for confirming both that it's max-width and the larger width of the live photo capture. Sounds good!

zachmargolis

LGTM

zachmargolis · 2020-10-05T18:25:42Z

app/javascript/packages/document-capture/components/review-issues-step.jsx

@@ -6,6 +6,7 @@ import AcuantCapture from './acuant-capture';
 import SelfieCapture from './selfie-capture';
 import FormErrorMessage from './form-error-message';
 import ServiceProviderContext from '../context/service-provider';
+import './review-issues-step.scss';


I didn't realize JS could import CSS files? 🤯

I didn't realize JS could import CSS files? 🤯

Not JS, but Webpack. It can feel like a bit of an ugly hack, but it's nice to keep things self-contained, and leaves open some options for omitting unused styles.

The main alternative here could be to have something like a manifest file or SASS entrypoint which then loads all of the individual stylesheets for the package.

As you noticed below, this is a feature Webpacker provides for free by providing an option to extract CSS imported this way.

Related:

https://github.com/rails/webpacker/blob/master/docs/css.md#link-styles-from-your-rails-views

https://github.com/rails/webpacker/blob/master/docs/css.md#importing-css-from-js

config/webpacker.yml

spec/javascripts/support/mocha.js

**Why**: Per design, for consistent width of inputs on Review Issues step.

**Why**: Errors if plugins provided, warns if plugins not provided. Not intending to use any plugins. Wasted overhead to run loader when unused.

See: 18F/identity-idp#5359 **Why:** So that we can use a common standard ruleset across all repositories where JavaScript is used. Notes: - Doesn't yet incorporate Prettier, since that will be a significant number of changes that can be safely be applied separately. - Removes PostCSS configuration. This follows a [similar removal in IdP](18F/identity-idp#4260), and we don't appear to be using [import inlining](https://github.com/postcss/postcss-import). Flexbugs and env settings seem most applicable to legacy browsers like Internet Explorer, but we appear to already be targeting newer browsers with e.g. [unpolyfilled usage of modern JavaScript like String#includes](https://github.com/18F/identity-dashboard/blob/abb55d8efa906f3db62ffa93f02b2d52e1bfacad/app/javascript/app/service-provider-form.js#L76).A quick check in Internet Explorer does not reveal any obvious breakage where we depend on these transforms.

aduth requested a review from anniehirshman-gsa October 2, 2020 13:43

anniehirshman-gsa reviewed Oct 2, 2020

View reviewed changes

zachmargolis approved these changes Oct 2, 2020

View reviewed changes

zachmargolis approved these changes Oct 5, 2020

View reviewed changes

aduth added 6 commits October 5, 2020 15:08

LG-3427: Size desktop selfie capture aligned to ID inputs in review step

c31d364

**Why**: Per design, for consistent width of inputs on Review Issues step.

Add stylesheet for ReviewIssuesStep component

f0523ac

Disable PostCSS loader

c1fca3d

**Why**: Errors if plugins provided, warns if plugins not provided. Not intending to use any plugins. Wasted overhead to run loader when unused.

Ignore SCSS imports in test runner

1b59c2e

Accept optional class on SelfieCapture component

9c16043

Remove width constraint on selfie field if taking photo

41148af

aduth force-pushed the aduth-size-desktop-selfie-capture branch from 24d8596 to 41148af Compare October 5, 2020 19:12

aduth merged commit b5740b5 into master Oct 5, 2020

aduth deleted the aduth-size-desktop-selfie-capture branch October 5, 2020 19:30

aduth mentioned this pull request Oct 21, 2020

Wrap document capture error message at bounds of file input #4324

Merged

aduth mentioned this pull request Sep 9, 2021

Use login.gov shared ESLint configuration 18F/identity-dashboard#476

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

LG-3427: Size desktop selfie capture aligned to ID inputs in review step #4260

LG-3427: Size desktop selfie capture aligned to ID inputs in review step #4260

aduth commented Oct 2, 2020

anniehirshman-gsa left a comment •

edited

Loading

zachmargolis left a comment

zachmargolis Oct 2, 2020

jmhooper Oct 2, 2020 •

edited

Loading

aduth Oct 2, 2020 •

edited

Loading

aduth commented Oct 2, 2020

aduth commented Oct 2, 2020

anniehirshman-gsa commented Oct 2, 2020

zachmargolis left a comment

zachmargolis Oct 5, 2020

aduth Oct 5, 2020

LG-3427: Size desktop selfie capture aligned to ID inputs in review step #4260

LG-3427: Size desktop selfie capture aligned to ID inputs in review step #4260

Conversation

aduth commented Oct 2, 2020

anniehirshman-gsa left a comment • edited Loading

Choose a reason for hiding this comment

zachmargolis left a comment

Choose a reason for hiding this comment

zachmargolis Oct 2, 2020

Choose a reason for hiding this comment

jmhooper Oct 2, 2020 • edited Loading

Choose a reason for hiding this comment

aduth Oct 2, 2020 • edited Loading

Choose a reason for hiding this comment

aduth commented Oct 2, 2020

aduth commented Oct 2, 2020

anniehirshman-gsa commented Oct 2, 2020

zachmargolis left a comment

Choose a reason for hiding this comment

zachmargolis Oct 5, 2020

Choose a reason for hiding this comment

aduth Oct 5, 2020

Choose a reason for hiding this comment

anniehirshman-gsa left a comment •

edited

Loading

jmhooper Oct 2, 2020 •

edited

Loading

aduth Oct 2, 2020 •

edited

Loading