Skip to content

Add FullScreen React component#3938

Merged
aduth merged 1 commit intomasterfrom
aduth-react-full-screen
Jul 23, 2020
Merged

Add FullScreen React component#3938
aduth merged 1 commit intomasterfrom
aduth-react-full-screen

Conversation

@aduth
Copy link
Contributor

@aduth aduth commented Jul 17, 2020

Relates to: LG-3094, LG-3023
Currently configured to merge to (blocked by) #3931 in order to leverage assets behavior

Why: As a user trying to proof, I want the capture mode to be more intuitive in landscape and portrait orientation, so that I can easily capture images of my IDs and selfie.

The changes in this pull request introduce a new React component <FullScreen />, implementing the behavior of a dismissible full-screen modal dialog. This is currently integrated in the work-in-progress single-page document authorization upload flow, shown in response to clicking the "Take photo" button in the verification step.

Screenshot:

Before Activating After Activating
before activating after activating

Notes:

  • The desktop experience doesn't currently support camera capture, so the content is expected to be empty. A prompt will appear to upload a photo.
  • Changes were needed to the testing setup, since I encountered that the existing focus-trap dependency has side effects in its imports which depend on DOM globals. These changes are included here, though could make sense to break off into a separate pull request. Overall it should simplify authoring tests in many cases.

@aduth aduth force-pushed the aduth-document-tips branch from 1747971 to 6bdf517 Compare July 20, 2020 17:34
@aduth aduth force-pushed the aduth-react-full-screen branch from 32b0631 to 79a3949 Compare July 20, 2020 17:39
Base automatically changed from aduth-document-tips to master July 21, 2020 15:55
Copy link
Contributor

@zachmargolis zachmargolis left a comment

Choose a reason for hiding this comment

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

LGTM

@aduth
Copy link
Contributor Author

aduth commented Jul 23, 2020

Summarizing the discussion at #3938 (comment) :

  • focus-trap is quite good at what it seeks to achieve, and despite the specific integration here in React being clunky, it doesn't appear the alternatives explored provide much benefit, and often have their own disadvantages.
  • There are potential use-cases for a focus trap beyond how we're using it here, and it may make sense to create some abstraction of useFocusTrap or a FocusTrap component.
  • However, it's yet to be seen what those specific requirements may be, and since this could be a driver for how the interface ought best to be defined†, I'm inclined to keep this implementation as-is, and wait for those requirements to develop.

† For example, should it behave like focus-trap-react and accept an object of all focusTrapOptions? Will we need all of those? Or does an onRequestClose suffice? How best should we handle changed values for each of these options?

@aduth aduth merged commit 969673b into master Jul 23, 2020
@aduth aduth deleted the aduth-react-full-screen branch July 23, 2020 12:57
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.

2 participants