Skip to content

LG-6089 Add location page to in-person flow#6624

Merged
tomas-nava merged 51 commits intomainfrom
shannon/lg-6089-select-location-page-retry
Jul 27, 2022
Merged

LG-6089 Add location page to in-person flow#6624
tomas-nava merged 51 commits intomainfrom
shannon/lg-6089-select-location-page-retry

Conversation

@tomas-nava
Copy link
Contributor

@tomas-nava tomas-nava commented Jul 25, 2022

This PR replaces #6607

Add the new "Select a location to verify your ID" page to the in-person proofing flow. Specifically, add as a page to the DocAuth React app so that the user can return to the image upload step without losing information.

screenshots

Desktop Location Page:

location-desktop

Location Page (error/no results):
Screen Shot 2022-07-26 at 1 13 45 PM

Location Page (loading):

localhost_3000_verify_doc_auth_document_capture (2)

Prepare Step:

localhost_3000_verify_doc_auth_document_capture (4)

Mobile

Location Page:

location-mobile

Location Page (loading):

localhost_3000_verify_doc_auth_document_capture(iPhone SE) (1)

Location Page (error/no results):

localhost_3000_verify_doc_auth_document_capture(iPhone SE)

Prepare Step:

localhost_3000_verify_doc_auth_document_capture(iPhone SE) (3)

@kellular kellular self-requested a review July 25, 2022 16:00
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.

The updated screenshots LGTM. Thanks!

@tomas-nava
Copy link
Contributor Author

tomas-nava commented Jul 26, 2022

I created LG-7026 to add tests for both the location and prepare step components.

@NavaTim NavaTim requested review from NavaTim and zachmargolis July 26, 2022 21:36
Copy link
Contributor

@NavaTim NavaTim left a comment

Choose a reason for hiding this comment

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

Looks good to me 😄

I did notice an issue with the back button, but that seems to be out of scope & was not caused by these changes. IIRC @svalexander agreed to create a story covering the back button issue. I am also under the impression that further tests for this page have been added to a separate story.

Comment on lines +28 to +38
@media screen and (max-width: 480px) {
.usa-button-mobile-hidden {
display: none;
}
}

@media screen and (min-width: 481px) {
.usa-button-desktop-hidden {
display: none;
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

(Non-blocking) Typically for this sort of thing I would encourage the use of display utility classes with responsive modifiers.

For example:

<!-- Hide below tablet breakpoint -->
<button class="usa-button display-none tablet:display-inline-block"></button>

<!-- Hide above tablet breakpoint -->
<button class="usa-button tablet:display-none"></button>

}
}

@media screen and (max-width: 480px) {
Copy link
Contributor

Choose a reason for hiding this comment

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

(Non-blocking) We try to standardize on the "tablet" breakpoint (640px) everywhere else in the application. Is that not possible?

Copy link
Contributor

Choose a reason for hiding this comment

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

this max-width is to make sure when we're on a mobile device the 'select' button to the right of the post office name is hidden. so the class usa-button-mobile-hidden signifies "hide the desktop placement of the button when on mobile". I wonder now if it should be renamed for better clarity?


module Idv
module InPerson
class UspsLocationsController < ApplicationController
Copy link
Contributor

Choose a reason for hiding this comment

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

(Non-blocking) We have some precedent for namespacing or naming routes as "API" when they talk exclusively JSON. Should we do that here?

Copy link
Contributor

Choose a reason for hiding this comment

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

For clarification - is the suggestion here that the controller should be renamed to "UspsLocationsAPIController"?

Copy link
Contributor

Choose a reason for hiding this comment

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

I was thinking in the module namespace, so e.g. Api::Verify::InPerson::UspsLocationsController. This is similar to what's done elsewhere.

module Api
  module Verify
    module InPerson
      class UspsLocationsController

And then we'd have the route be something like /api/verify/in_person/usps_locations.

Copy link
Contributor

Choose a reason for hiding this comment

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

oh ok , got it!

@tomas-nava tomas-nava merged commit fee0abe into main Jul 27, 2022
@tomas-nava tomas-nava deleted the shannon/lg-6089-select-location-page-retry branch July 27, 2022 16:59
@tomas-nava
Copy link
Contributor Author

My overall goal is that all requests/responses in and out of the IDP are snake_case, so it's up to the JS to convert to camel if needed?

I will fix this in a follow-on

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.

6 participants