Skip to content

LG-5115:Refactor the check/select box behavior across the IAL2 flow#5417

Merged
jmdembe merged 2 commits intomainfrom
jd-LG-5115-check-select-box-behavior
Sep 21, 2021
Merged

LG-5115:Refactor the check/select box behavior across the IAL2 flow#5417
jmdembe merged 2 commits intomainfrom
jd-LG-5115-check-select-box-behavior

Conversation

@jmdembe
Copy link
Contributor

@jmdembe jmdembe commented Sep 17, 2021

This PR adds a "please check this box to continue" warning to the doc auth agreement page.

Why: So that users are error messages are more consistent and easily identifiable to the user.

Before After
image image

@jmdembe jmdembe changed the title re-factor checkbox LG-5115:Refactor the check/select box behavior across the IAL2 flow Sep 17, 2021
@jmdembe jmdembe force-pushed the jd-LG-5115-check-select-box-behavior branch 3 times, most recently from ba1f01b to 925931d Compare September 17, 2021 16:26
@aduth
Copy link
Contributor

aduth commented Sep 17, 2021

Just a heads-up that I merged #5279 which updated the markup for the agreement checkbox to use the design system checkbox, and might introduce some conflicts for your branch. Let me know if I can help sort those out.

@jmdembe jmdembe force-pushed the jd-LG-5115-check-select-box-behavior branch 2 times, most recently from 3e2f179 to ab00f4f Compare September 17, 2021 19:21
@jmdembe jmdembe marked this pull request as ready for review September 17, 2021 19:43
@jmdembe jmdembe requested review from aduth and mdiarra3 September 17, 2021 20:31
@jmdembe jmdembe marked this pull request as draft September 17, 2021 20:44
@jmdembe jmdembe marked this pull request as ready for review September 17, 2021 22:05
@jmdembe jmdembe marked this pull request as draft September 17, 2021 22:05
Copy link
Contributor

@aduth aduth left a comment

Choose a reason for hiding this comment

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

For a similar error message in #4968, we implemented and used a "display-if-invalid" helper for the form validation, so that the message can be shown before the user hits "Continue". Could we use that here as well?

I think we'd just need to add display-if-invalid to the message and required: true to the input:

diff --git a/app/views/idv/doc_auth/agreement.html.erb b/app/views/idv/doc_auth/agreement.html.erb
index d801ffebe..1e29d93cd 100644
--- a/app/views/idv/doc_auth/agreement.html.erb
+++ b/app/views/idv/doc_auth/agreement.html.erb
@@ -25,14 +25,15 @@
           true,
           false,
           class: 'usa-checkbox__input',
+          required: true,
         ) %>
     <label class="usa-checkbox__label" for="ial2_consent_given">
       <%= t('doc_auth.instructions.consent') %>
       <%= new_window_link_to t('doc_auth.instructions.learn_more'), MarketingSite.security_and_privacy_practices_url %>
     </label>
-    <% if flow_session[:error_message] %>
-      <%= render 'shared/ial2-alert' %>
-    <% end %>
+    <div class="usa-error-message usa-error-message--with-icon display-if-invalid" role="alert">
+      <%= t('sign_up.agree_to_rules_of_use_and_continue') %>
+    </div>
   </div>
   <%= f.button :button, t('doc_auth.buttons.continue'), type: :submit,
                class: 'usa-button--disabled usa-button--big usa-button--wide' %>

Copy link
Contributor

Choose a reason for hiding this comment

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

The partial is quite small and not very reusable. Could we just render the markup inline here instead?

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 considered rendering inline, but the rails documentation deems this to be a bad practice. Also, it makes the code hard to maintain for future us.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I was unclear. By "inline", I just mean having the markup here, i.e.

Suggested change
<%= render 'shared/ial2-alert' %>
<div class="usa-error-message usa-error-message--with-icon" role="alert">
<%= t('sign_up.agree_to_rules_of_use_and_continue') %>
</div>

Copy link
Contributor

Choose a reason for hiding this comment

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

Since we're reusing this text outside of sign-up, maybe we should move / rename it to something more generic? In forms for example?

@jmdembe
Copy link
Contributor Author

jmdembe commented Sep 20, 2021

For a similar error message in #4968, we implemented and used a "display-if-invalid" helper for the form validation, so that the message can be shown before the user hits "Continue". Could we use that here as well?

I think we'd just need to add display-if-invalid to the message and required: true to the input:

diff --git a/app/views/idv/doc_auth/agreement.html.erb b/app/views/idv/doc_auth/agreement.html.erb
index d801ffebe..1e29d93cd 100644
--- a/app/views/idv/doc_auth/agreement.html.erb
+++ b/app/views/idv/doc_auth/agreement.html.erb
@@ -25,14 +25,15 @@
           true,
           false,
           class: 'usa-checkbox__input',
+          required: true,
         ) %>
     <label class="usa-checkbox__label" for="ial2_consent_given">
       <%= t('doc_auth.instructions.consent') %>
       <%= new_window_link_to t('doc_auth.instructions.learn_more'), MarketingSite.security_and_privacy_practices_url %>
     </label>
-    <% if flow_session[:error_message] %>
-      <%= render 'shared/ial2-alert' %>
-    <% end %>
+    <div class="usa-error-message usa-error-message--with-icon display-if-invalid" role="alert">
+      <%= t('sign_up.agree_to_rules_of_use_and_continue') %>
+    </div>
   </div>
   <%= f.button :button, t('doc_auth.buttons.continue'), type: :submit,
                class: 'usa-button--disabled usa-button--big usa-button--wide' %>

The message flips if aria-invalid="false. The reason why I went with this pattern instead of using display-if-invalid is because when I added the aria-invalid attribute to the check_box_tag, it would not respond to user interaction. I also tried changing this checkbox to use the simple form checkbox and for some reason, the checkbox would not toggle. If I am missing something, I would be more than happy to talk through this.

@jmdembe jmdembe force-pushed the jd-LG-5115-check-select-box-behavior branch 4 times, most recently from 541fa69 to 322b6d1 Compare September 20, 2021 16:43
@jmdembe jmdembe marked this pull request as ready for review September 20, 2021 17:20
Copy link
Contributor

Choose a reason for hiding this comment

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

I remember now we intentionally didn't apply usa-button--disabled until JavaScript has run, since otherwise the button always appears disabled in browsers where JavaScript is disabled, even after checking the checkbox (screenshot).

Was there a reason for adding it, or can we leave it out?

Suggested change
class: 'usa-button--disabled usa-button--big usa-button--wide' %>
class: 'usa-button--big usa-button--wide' %>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can leave it out. I think I was having a weird experience with the JavaScript and I forgot to take this out.

Copy link
Contributor

Choose a reason for hiding this comment

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

Per the previous comment, can we move / rename this string so it's not referencing "sign up" and "rules of use" if we intend to use it wherever there's a required checkbox to be checked? Maybe something like forms.validation.required_checkbox ?

@jmdembe jmdembe force-pushed the jd-LG-5115-check-select-box-behavior branch from 322b6d1 to c5f63ed Compare September 21, 2021 15:04
Copy link
Contributor

@aduth aduth 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! 👍

@jmdembe jmdembe merged commit 2b36af4 into main Sep 21, 2021
@jmdembe jmdembe deleted the jd-LG-5115-check-select-box-behavior branch September 21, 2021 22:08
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