Skip to content

LG-11577: selfie UI content update#9746

Merged
dawei-nava merged 17 commits intomainfrom
dwang/LG-11577-selfie-ui-content
Dec 18, 2023
Merged

LG-11577: selfie UI content update#9746
dawei-nava merged 17 commits intomainfrom
dwang/LG-11577-selfie-ui-content

Conversation

@dawei-nava
Copy link
Contributor

@dawei-nava dawei-nava commented Dec 12, 2023

🎫 Ticket

LG-11577

🛠 Summary of changes

Update UI content when selfie is enabled. Screen divided into two sections with numbered section headers.
Updated the page header also.

📜 Testing Plan

Provide a checklist of steps to confirm the changes.
With selfie disabled

  • Step 1: Start doc auth flow
  • Step 2: On initial document capture page
  • Step 3: Check the new page header, subheader(not numbered) and tip list header.

With selfie enabled

  • Step 1: Start doc auth flow
  • Step 2: On initial document capture page
  • Step 3: Check the new page header, numbered subheaders(for doc and selfie) and tip list header for doc and selfie.

👀 Screenshots

English with liveness

After:

Mobile top section:

top


Mobile bottom section:

mobile-livness-bottom-en


Mobile top with error:

top-error


Desktop:

desktop-liveness-en-update

Spanish with liveness

After:

Mobile top

top-es-liveness


Mobile bottom

mobile-bottom-es-update


Mobile top error

top-liveness-error-es


Desktop:

desktop-liveness-es-update

French with liveness

After:

Mobile top

top-liveness-fr


Mobile bottom

mobile-fr-update


Mobile top error

top-liveness-error-fr


Desktop:

desktop-liveness-fr-update


English without liveness

After:

mobile

mobile-top-noliveness-en

desktop

desktop-noliveness-en

Spanish without liveness

After:

mobile

mobile-top-noliveness-es

desktop

desktop-noliveness-es

French without liveness

After:

mobile

mobile-top-noliveness-fr

desktop

desktop-noliveness-fr

@dawei-nava
Copy link
Contributor Author

@kellular please take a look at the screenshots.

@dawei-nava dawei-nava marked this pull request as ready for review December 14, 2023 15:19
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.

Do you know what's happening with the spacing between the "Take photo" button and the word "or"? The space should be the same distance as "or" and "Upload photo"... should I create a separate eng ticket to address this?

image

@dawei-nava
Copy link
Contributor Author

Do you know what's happening with the spacing between the "Take photo" button and the word "or"? The space should be the same distance as "or" and "Upload photo"... should I create a separate eng ticket to address this?

image

@kellular , not sure, did not touch that part. May be some style changes affected this.

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.

Just had a couple translations that need updating!

Copy link
Contributor

@charleyf charleyf left a comment

Choose a reason for hiding this comment

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

Looks like it's getting close! I have some suggestions about restructuring the component tree.

* @type {DocumentSide[]}
*/
const documentSides = selfieCaptureEnabled ? ['front', 'back', 'selfie'] : ['front', 'back'];
const documentSides = ['front', 'back'];
Copy link
Contributor

Choose a reason for hiding this comment

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

I see why you went this direction. I agree that we should be splitting things out into components. Instead of doing it this way, I'd recommend you return a component structure like this from this file. Some of my reasoning:

  • With the approach I'm suggesting all the headers are in the same file.
  • With this approach we avoid the need for another component that mostly passes props through.
  • I think naming the various instances of is a good idea.
return (
    <>
      {flowpath === ...}
      <PageHeading>...
      <DocumentCaptureSubheader> // returns the h2 starting with "1."
      <TipList>...
      <DocumentFront> // a new component, wraps DocumentSideAcuantCapture
      <DocumentBack> // a new component, wraps DocumentSideAcuantCapture
      <SelfieCaptureSubheader> // returns the h2 starting with "2."
      <Selfie> // a new component, wraps DocumentSideAcuantCapture
      {isLastStep ...}
      {notReadySectionEnabled ...}
      {exisQuestionSectionEnabled ...}
      <Cancel>
    </>
)

And for the <DocumentFront>, <DocumentBack>, and <Selfie> components, something like this in this same file (probably don't need a new file for each of these components):

const DocumentFront = ({registerField, onChange, onError}: {...new type...}) = {
  const side: DocumentSide = 'front'
  return (
    <DocumentSideAcuantCapture
          key={side}
          side={side}
          registerField={registerField}
          value={value[side]}
          onChange={onChange}
          errors={errors}
          onError={onError}
    />
  )
}

Copy link
Contributor Author

@dawei-nava dawei-nava Dec 15, 2023

Choose a reason for hiding this comment

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

@charleyf , refactored with HOC for document sides. Also I kept selfie part as a separate component, since it contains multiple parts, it feels easier to test as a whole piece.

Copy link
Contributor

@charleyf charleyf Dec 15, 2023

Choose a reason for hiding this comment

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

it feels easier to test as a whole piece

That makes sense to me. I do still think that everything in document-capture-selfie-capture.tsx should move into documents-step.jsx. I think that if you move everything up into documents-step.jsx your tests will continue to work (with minor setup changes) in documents-step-spec.jsx?

A few pieces of evidence:

  • The two h2s ( 1. ... and 2. ...) should appear in the same file since they're at the same level on the same page.
  • The props you're sending to DocumentCaptureSelfieCapture are identical to the props received by DocumentsStep.
  • The two tip lists have identical props: SelfieTipList and DocumentTipList
  • The props for SelfieSection (in the existing section) are identical to the props for Selfie (in the new file)

Another way I'm thinking about this: I can't describe in words what the purpose of document-capture-selfie-capture is, or how it's different from what documents-step accomplishes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Revert how front, back sides rendered, otherwise it cause some document active element issues during testing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@charleyf agree most of the bullet points, and seems backtracked from previously components etc.

Also, in general it may be callsed document-capture-selfi-section? Anyway I can pull into document-steps, separate it because later we will have it with review-issue-step page.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@charleyf , pulled all stuff in document step.

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice! Having all the TipList, <h2> and <DocumentSideAcuantCapture> in the same file makes it much clearer to me.

and seems backtracked from previously components etc

I'd be interested to hear more about this. The pseudocode for my suggested component structure with an example component is earlier in this thread. I'm happy to help you implement that, but I think the way the code is currently is close enough.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@charleyf , yes basically following is the same with the HOC withProps, where the element focus seems not behaves the same with rendering in places, I too was baffled by it since the change has nothing to do with functionality, but that feels quite elusive and can consume quite some time.

const DocumentFront = ({registerField, onChange, onError}: {...new type...}) = {
  const side: DocumentSide = 'front'
  return (
    <DocumentSideAcuantCapture
          key={side}
          side={side}
          registerField={registerField}
          value={value[side]}
          onChange={onChange}
          errors={errors}
          onError={onError}
    />
  )
}

Copy link
Contributor

Choose a reason for hiding this comment

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

✅ just approved, since I agree it doesn't necessarily make much sense to keep polishing this.

Agreed, I'm also quite surprised that extracting those components would change anything. Can you tell me what you mean about the element focus changing? Do you mean the outline or some other aspect? A screenshot would help too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@charleyf in the document-capture-spec test, checks for document.activeElement

@dawei-nava
Copy link
Contributor Author

Just had a couple translations that need updating!

@kellular All updated.

Comment on lines 69 to 74
const DocumentFront = withProps({
key: 'front',
side: 'front',
value: value.front,
...defaultSideProps,
})(DocumentSideAcuantCapture);
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice! Can you move these outside the DocumentsStep component? When they're inline like this it's hard to trace exactly which props are required by each component.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This caused some weird test issues on focused element. Revert to the old way.

…e issues on active element on page during testing.
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.

LGTM!

Copy link
Contributor

@charleyf charleyf left a comment

Choose a reason for hiding this comment

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

I think this is ready to merge.

I manually verified:

  • Turning off the selfie featureflag hides all of this.
  • The doc auth flow continues to work with the selfie featureflag on.
  • The translations match (I didn't read every line, but did look through the app and translations file together).
  • The updated tests look good.

@dawei-nava dawei-nava merged commit a34549f into main Dec 18, 2023
@dawei-nava dawei-nava deleted the dwang/LG-11577-selfie-ui-content branch December 18, 2023 01:14
user.document_capture_sessions.last.store_result_from_response(response)
end

def expect_doc_capture_page_header(text)
Copy link
Contributor

Choose a reason for hiding this comment

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

👏🏻 I know you already merged this, but I just wanted to chime in and say that these are helpful helper methods!

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.

5 participants