Skip to content

LG-9200 show hint text for addresses in puerto rico#8299

Merged
svalexander merged 16 commits intomainfrom
shannon/lg-9200-pr-addresses
May 5, 2023
Merged

LG-9200 show hint text for addresses in puerto rico#8299
svalexander merged 16 commits intomainfrom
shannon/lg-9200-pr-addresses

Conversation

@svalexander
Copy link
Contributor

@svalexander svalexander commented Apr 27, 2023

🎫 Ticket

LG-9200

🛠 Summary of changes

This pr adds hint text for address line 1 and address line 2 on the state id page and address page when a user selects "Puerto Rico" from the state dropdown.
If a user changes their selection then the inputs for address line 1 and 2 remain but the hint text is removed.
When a user clicks update for the state id section and address section from the "verify your info" page and is routed to the update pages they see the hint text if they previously chose puerto rico as their state.

📜 Testing Plan

Provide a checklist of steps to confirm the changes.

  • Update in_person_capture_secondary_id_enabled to true
  • Walk through ipp flow and arrive at state id page
  • Select Puerto Rico from the state dropdown
  • Verify that hint text shows for address lines 1 and 2; enter text to both input boxes
  • Select another state
  • Verify that hint text is hidden and inputs remain
  • Re-select Puerto Rico
  • Repeat same selection steps described for state id page on the address page
  • Continue to "verify your info" page
  • Select update button for state id section
  • Verify that hint text shows for address lines 1 and 2
  • Click update button to go back to "verify your info" page
  • Repeat same update steps for address section

👀 Screenshots

If relevant, include a screenshot or screen capture of the changes.

Address page: 2023-04-24 at 12 56 25 PM 2023-04-24 at 12 50 39 PM
State id pg: 2023-04-27 at 12 22 33 PM 2023-04-27 at 12 22 41 PM

@svalexander svalexander requested review from a team, WheresHJ and eileen-nava and removed request for a team April 27, 2023 16:27
@carmenrosalop carmenrosalop requested review from carmenrosalop and removed request for WheresHJ April 27, 2023 16:30
@eileen-nava
Copy link
Contributor

Status update: I have started reviewing this PR and will wrap up the review tomorrow morning. Looking good so far! 💪🏻

@WheresHJ
Copy link

Generally looks good, just to cover all the bases can I see a screenshot of the non-Puerto Rico state of the Current Residential Address page? Thank you!

@eileen-nava
Copy link
Contributor

eileen-nava commented Apr 28, 2023

HI @svalexander, I noticed that your build is failing on the pinpoint-check. If you merge in the latest version of main, you'll pick up the work from PR #8279. Based on what I saw in slack discussions, that should get your build to pass the pinpoint-check step.

(Also, there is one failing test in spec/services/idv/steps/in_person/state_id_step_spec.rb, which looks like it will pass once you update the variables.)

Copy link

@carmenrosalop carmenrosalop 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. Thanks Shannon!

@eileen-nava
Copy link
Contributor

Status update 2: I think this looks really good, especially the feature test. I do agree with Andrew that it would be good to try and use class selectors, rather than id selectors.

<%= render ValidatedFieldComponent.new(
form: f,
hint: t('in_person_proofing.form.state_id.address1_hint'),
hint_class: 'puerto-rico-extras',
Copy link
Contributor

Choose a reason for hiding this comment

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

You might need to rebase or merge the latest main, since hint_class: '...' was removed in favor of hint_html: { class: '...' } in #8232.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i notice that the hint is flashing on the screen when the page is loaded. Not sure how to rectify this.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think you might have fixed it with 6f6263f ? Normally that might introduce other problems with supporting browsers where JavaScript is disabled, but since IdV requires JavaScript, it should be fine. An alternative might be to add the class server-side depending if the known address state is Puerto Rico.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes that commit fixed it, yay! I do need to test the remote flow before merging though

Copy link
Contributor

@eileen-nava eileen-nava 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. 👍🏻

form: f,
hint: t('in_person_proofing.form.state_id.address2_hint'),
hint_class: 'puerto-rico-extras',
hint_html: { class: 'puerto-rico-extras' },
Copy link
Contributor

Choose a reason for hiding this comment

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

👏🏻

@svalexander svalexander requested a review from NavaTim May 4, 2023 22:55
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.

Would consider approving, but a lint issue came up in CI & I'd like to see the tests run. Based on the type of issue, you may need to pull from origin/main.

Copy link
Contributor

@NavaTim NavaTim May 4, 2023

Choose a reason for hiding this comment

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

If we want this to be reusable functionality, then I recommend a few changes.

  • Use a new CSS class to specifically reference <select> dropdowns that should have this behavior instead of checking for jurisdiction in the element ID
  • Split out the showOrHidePuertoRicoExtras into a separate file included by this one
  • Consider moving onIdentityDocStateSelection into a separate file that doesn't immediately run the code nor setup an event listener

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i'm not sure i fully understand the last 2 bullet points

Copy link
Contributor

Choose a reason for hiding this comment

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

Those changes separate initialization on the web page from the function logic, which encourages more modular behavior and makes effective unit testing easier.

Also I am listing them separately because one pertains to showing/hiding the Puerto Rico text and the other pertains to the change event behavior.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

chatted w/ Micah about separating out the functions and came to the conclusion that could potentially be done in another ticket that generalizes this more to showOrHideHint() if we find there's a need for that.

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 at this point since the previous build passed. I think it's really good to have switched to the new CSS class too. Still recommend the remaining changes, but I think you're good to proceed if the next build passes.

@svalexander svalexander merged commit 123647e into main May 5, 2023
@svalexander svalexander deleted the shannon/lg-9200-pr-addresses branch May 5, 2023 16:23
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