reCAPTCHA: Elig Index, Elig Confirm - Add reCAPTCHA text, hide flag #2586

machikoyasuda · 2024-12-11T20:21:36Z

What this PR does

This ticket actually adds the reCAPTCHA text to 2 forms (Eligibility Confirm, Eligibility Index) and hides the badge.

Creates a recaptcha-text includes with the English and Spanish text
Includes recaptcha-text in form.html within the if request.recaptcha block, so that it shows up automatically on where recaptcha is used
Adds CSS to: hide the badge via visibility hidden, updates form-text class to fix the form field helper letter-spacing, and use that same class for the recaptcha text, adds styling so that the external link icon is smaller for the smaller text
Minor div/spacing refactors on form and template pages.

Testing notes

Use Fix: reCAPTCHA + HTML5 form validation #1071 instructions to test in Dev. The reCAPTCHA test environment key/secret are in LastPass, under Benefits reCAPTCHA configuration.
Post-merge test plan: When this PR is merged into dev, we can first test that the reCAPTCHA message does not show up. It should not show up b/c reCAPTCHA is not enabled. Once we do turn reCAPTCHA on, then we can test that the reCAPTCHA text turns on and the feature is enabled.

Screenshots

Desktop	Mobile

github-actions · 2024-12-11T20:22:56Z

Coverage report

Click to see where and how coverage changed

File	Statements	Missing	Coverage	Coverage (new stmts)	Lines missing
benefits/eligibility
forms.py
Project Total

_{This report was generated by python-coverage-comment-action}

machikoyasuda · 2024-12-11T20:24:37Z

benefits/eligibility/templates/eligibility/confirm.html

+
+{% block call-to-action %}
+{% endblock call-to-action %}


Why is this here?

It's here to ensure that more spacing is not created. If this is not here, then the base.html will put this HTML on the page, which includes pt-8 -- which we do not want:

<div class="row d-flex justify-content-center pt-8"> <div class="col-12 col-lg-6"> {% block call-to-action-button %} {% endblock call-to-action-button %} </div> </div>

There's an alternate way to achieve this. I could have put this code on both eligibility/confirm.html and eligibility/index.html:

{% block call-to-action-button %} {% include "core/includes/recaptcha-text.html" %} {% endblock call-to-action-button %}

But it felt semantically incorrect to put text into code that was meant to be for a button only. And I didn't want to come up with a more generic name to rename call-to-action-button or rename all those on the other templates... So I went with this method instead.

This sounds reasonable 👍 I think a comment above the empty block in this file just indicating that it is there for a reason (maybe a link to this thread here?) would be helpful for the future.

Added comments in both templates to clarify what is going on there

machikoyasuda · 2024-12-11T20:25:37Z

benefits/static/css/variables.css

-  --bs-font-sans-serif: Roboto, system-ui, -apple-system, "Segoe UI", sans-serif,
-    "Apple Color Emoji", "Segoe UI Emoji", "Segoe UI Symbol", "Noto Color Emoji";
+  --bs-font-sans-serif: Roboto, system-ui, -apple-system, "Segoe UI",
+    sans-serif, "Apple Color Emoji", "Segoe UI Emoji", "Segoe UI Symbol",
+    "Noto Color Emoji";


Wait why does pre-commit do this to me T_T

machikoyasuda · 2024-12-11T23:09:43Z

benefits/static/css/styles.css

@@ -556,7 +564,7 @@ footer .footer-links li a.footer-link:visited {
 .form-text {
  font-size: var(--font-size-14px);
  line-height: var(--form-field-text-line-height);
-  letter-spacing: var(--letter-spacing-5);
+  letter-spacing: calc(var(--font-size-14px) * var(--letter-spacing-5));


This letter-spacing update affects not just reCAPTCHA text, but the form field helper text as well. It was not updated on Production.
The way this is calculated: In Figma, click on the text. In the Dev resources sidebar, ensure Properties is enabled (instead of Code). Look for the letter spacing percentage:

Multiply the percentage with the font size.

machikoyasuda · 2024-12-11T23:11:55Z

benefits/static/css/styles.css

@@ -517,26 +533,18 @@ footer .footer-links li a.footer-link:visited {
 }

 /* Forms: Text Inputs */
-/* Form Container: Must use .form-container parent to use these styles */
+/* Form Field Container: Must use .form-container parent to use these styles */


Renamed this for clarity. Each .form-field-container wraps one of these as a unit

.form-container made it seem like it was wrapping the entire form, which is kinda true for Elig Index but not on Elig Confirm.

machikoyasuda · 2024-12-11T23:13:14Z

benefits/core/templates/core/includes/form.html

-    <div class="row form-container justify-content-center">
-      <div class="{{ form.classes }}">
-        {% for field in form %}
-          <div class="row form-group mb-0">
-            <div class="col-12">
-              {# djlint:off #}
-              {% if field.label %}
-              <label for="{{ field.id_for_label }}" class="form-control-label">{{ field.label }}{% if field.field.required %}<span class="required-label text-body">*</span>{% endif %}
-              </label>
-              {% endif %}
-              {# djlint:on #}
-
-              {{ field }}
-
-              {% if field.help_text %}<small class="d-block mt-2 pt-1 form-text text-body">{{ field.help_text }}</small>{% endif %}
-            </div>
-          </div>
-        {% endfor %}
-      </div>


All of these row justify-content-center, row, col-12 are unnecessary.

machikoyasuda · 2024-12-11T23:13:36Z

benefits/core/templates/core/includes/form.html

-      <div class="row d-flex justify-content-center pt-8">
-        <div class="col-lg-6">


Now longer needed.

machikoyasuda · 2024-12-11T23:37:33Z

benefits/static/css/styles.css

+.eligibility-confirm footer {
+  margin-top: 24px;
+}
+


The Elig Index page's reCAPTCHA text has 64px padding both above and below the text, but the Elig Confirm's has 64px above and 24px below. So this class allows us to get that. This is the only page on the whole app, aside from the Index page, to not have that 64px buffer above the footer.

I narrowed down the issue I mentioned in #2586 (comment) for confirm.html to this style... somehow this is causing the footer to have spacing on the bottom?

machikoyasuda · 2024-12-11T23:38:10Z

benefits/static/css/styles.css

@@ -517,26 +533,18 @@ footer .footer-links li a.footer-link:visited {
 }

 /* Forms: Text Inputs */
-/* Form Container: Must use .form-container parent to use these styles */
+/* Form Field Container: Must use .form-field-container parent to use these styles */


Renamed this b/c this div is a container around one form field, rather than the entire form.

machikoyasuda · 2024-12-11T23:38:36Z

benefits/static/css/styles.css

-:root {
-  --form-input-gap: calc(24rem / 16);
-}
-
-.form-container .form-group:not(:first-child) {
-  padding-top: var(--form-input-gap);
-}


Use the d-flex flex-column gap-4 CSS class to achieve this instead.

machikoyasuda · 2024-12-12T01:25:49Z

This is ready for review.

benefits/static/css/styles.css

thekaveman

Looking really good. Just a couple of questions.

machikoyasuda · 2024-12-16T19:35:03Z

How to test reCAPTCHA locally: #1071

I added the test reCAPTCHA keys to the Benefits reCAPTCHA file in the Compiler Engineering LastPass shared doc.

…er text for elig-confirm

benefits/core/templates/core/includes/form.html

angela-tran

Changes are looking good. I noticed an odd CSS issue though with how we're trying to override the 64px top spacing for the footer with 24px.

benefits/eligibility/templates/eligibility/confirm.html

angela-tran · 2024-12-17T19:54:34Z

benefits/static/css/styles.css

+.eligibility-confirm footer {
+  margin-top: 24px;
+}
+


I narrowed down the issue I mentioned in #2586 (comment) for confirm.html to this style... somehow this is causing the footer to have spacing on the bottom?

angela-tran · 2024-12-17T22:16:42Z

@machikoyasuda can you check on this alignment issue on Enrollment Index? I see it only in this branch.

edit: maybe you've fixed it in #2588? I haven't checked yet

machikoyasuda · 2024-12-18T01:07:05Z

@angela-tran Oh wow I don't know how those non-fixes on in there. For now, I just added those good ole divs on divs back again. But they'll all be removed in #2588

angela-tran · 2024-12-20T22:31:59Z

benefits/core/templates/core/includes/form.html

    {% csrf_token %}

-    <div class="row form-container justify-content-center">
-      <div class="{{ form.classes }}">


I think there's a regression in Benefits Admin coming from this change.

We have some styles in admin/styles.css so that descendant checkboxes will be on the left side:

benefits/benefits/static/css/admin/styles.css

Lines 88 to 103 in dce769c

/* Eligibility Page */

.checkbox-parent .form-group:last-of-type .col-12 {

display: flex;

flex-direction: row-reverse;

justify-content: start;

column-gap: 0.5rem;

margin-top: 2rem;

}

.checkbox-parent,

.checkbox-parent .form-group .col-12,

.checkbox-parent .form-group .col-12 #id_flow {

display: flex;

flex-direction: column;

row-gap: 1rem;

}

InPersonEligibilityForm has checkbox-parent in its classes, which would end up on this line, but now it's on the <form> element.

main this branch (with #2555 merged in locally... *)

Not sure if we should put this <div> back or try to tweak the CSS?

(*When we're back in the new year, it'd be good to get all this reCAPTCHA work caught up with the fixes that are in main.)

machikoyasuda self-assigned this Dec 11, 2024

github-actions bot added i18n Copy: Language files or Django i18n framework front-end HTML/CSS/JavaScript and Django templates labels Dec 11, 2024

machikoyasuda commented Dec 11, 2024

View reviewed changes

machikoyasuda force-pushed the feat/2541-recaptcha-add-link-hide-flag branch from dee6b3f to 303a5b1 Compare December 11, 2024 23:01

machikoyasuda commented Dec 11, 2024

View reviewed changes

machikoyasuda force-pushed the feat/2541-recaptcha-add-link-hide-flag branch from f19ba58 to 36b9a3b Compare December 11, 2024 23:32

machikoyasuda commented Dec 11, 2024

View reviewed changes

machikoyasuda marked this pull request as ready for review December 11, 2024 23:43

machikoyasuda requested a review from a team as a code owner December 11, 2024 23:43

machikoyasuda changed the title ~~reCAPTCHA: Add text, hide flag~~ reCAPTCHA: Elig Index, Elig Confirm: Add text, hide flag Dec 12, 2024

machikoyasuda changed the title ~~reCAPTCHA: Elig Index, Elig Confirm: Add text, hide flag~~ reCAPTCHA: Elig Index, Elig Confirm - Add text, hide flag Dec 12, 2024

machikoyasuda marked this pull request as draft December 12, 2024 00:15

machikoyasuda marked this pull request as ready for review December 12, 2024 01:25

thekaveman added this to the Benefits app frontend enhancements and optimization milestone Dec 13, 2024

thekaveman reviewed Dec 13, 2024

View reviewed changes

benefits/static/css/styles.css Outdated Show resolved Hide resolved

thekaveman reviewed Dec 13, 2024

View reviewed changes

benefits/static/css/styles.css Outdated Show resolved Hide resolved

thekaveman reviewed Dec 13, 2024

View reviewed changes

machikoyasuda changed the title ~~reCAPTCHA: Elig Index, Elig Confirm - Add text, hide flag~~ reCAPTCHA: Elig Index, Elig Confirm - Add reCAPTCHA text, hide flag Dec 16, 2024

machikoyasuda added 2 commits December 16, 2024 11:35

feat(elig-index): create recaptcha-text, add spanish, add to elig index

d81de3e

refactor(form): put recaptcha link in form instead

8a5d4cd

machikoyasuda added 9 commits December 16, 2024 11:35

fix(form): fix alignment of recaptcha, was slightly too wide

32c9297

fix(elig-confirm): form does not need to be wrapped in container/row

7a81aab

feat(recaptcha-text): add text styling, add icon size styling

d331ca6

fix(recaptcha-text): don't link the Google part, for En and Es

129d108

feat(recaptcha): hide the badge with CSS

128d28a

fix(recaptcha-text): add mt-0 to reset margin-top from bootstrap

29dad70

refactor: refactor form to allow for a mb-fix that shortens space und…

7479af3

…er text for elig-confirm

fix: update comment to proper classname

5d34bb4

refactor(css): use a footer style rather than negative margin hack

9cfd2c6

machikoyasuda force-pushed the feat/2541-recaptcha-add-link-hide-flag branch from 9cefd75 to 9cfd2c6 Compare December 16, 2024 19:35

machikoyasuda added 2 commits December 16, 2024 21:19

refactor(css): variable-ize external link icon size

a4e9b6f

fix(css): contain icon size

1f2ee3e

machikoyasuda requested a review from thekaveman December 16, 2024 21:29

docs: add clarifying comments

87f40f7

machikoyasuda commented Dec 17, 2024

View reviewed changes

benefits/core/templates/core/includes/form.html Show resolved Hide resolved

machikoyasuda mentioned this pull request Dec 17, 2024

reCAPTCHA: Spacing and template refactor #2588

Open

fix(recaptcha): use display none on badge

b21f806

angela-tran requested changes Dec 17, 2024

View reviewed changes

machikoyasuda added 2 commits December 17, 2024 21:25

fix(css): remove 24px footer margin from elig-confirm

986fc6f

fix: remove unnecessary class

ec05326

fix(form): undo form refactoring for now

dce769c

machikoyasuda requested a review from angela-tran December 18, 2024 01:15

machikoyasuda linked an issue Dec 19, 2024 that may be closed by this pull request

reCAPTCHA: Add reCAPTCHA text and hide button #2541

Open

angela-tran reviewed Dec 20, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

reCAPTCHA: Elig Index, Elig Confirm - Add reCAPTCHA text, hide flag #2586

reCAPTCHA: Elig Index, Elig Confirm - Add reCAPTCHA text, hide flag #2586

machikoyasuda commented Dec 11, 2024 •

edited

Loading

github-actions bot commented Dec 11, 2024 •

edited

Loading

machikoyasuda Dec 11, 2024

thekaveman Dec 13, 2024

machikoyasuda Dec 17, 2024

machikoyasuda Dec 11, 2024

machikoyasuda Dec 11, 2024

machikoyasuda Dec 11, 2024

machikoyasuda Dec 11, 2024

machikoyasuda Dec 11, 2024

machikoyasuda Dec 11, 2024

angela-tran Dec 17, 2024 •

edited

Loading

machikoyasuda Dec 11, 2024

machikoyasuda Dec 11, 2024

machikoyasuda commented Dec 12, 2024

thekaveman left a comment

machikoyasuda commented Dec 16, 2024 •

edited

Loading

angela-tran left a comment

angela-tran Dec 17, 2024 •

edited

Loading

angela-tran commented Dec 17, 2024 •

edited

Loading

machikoyasuda commented Dec 18, 2024

angela-tran Dec 20, 2024

		<div class="row d-flex justify-content-center pt-8">
		<div class="col-lg-6">

	/* Eligibility Page */
	.checkbox-parent .form-group:last-of-type .col-12 {
	display: flex;
	flex-direction: row-reverse;
	justify-content: start;
	column-gap: 0.5rem;
	margin-top: 2rem;
	}

	.checkbox-parent,
	.checkbox-parent .form-group .col-12,
	.checkbox-parent .form-group .col-12 #id_flow {
	display: flex;
	flex-direction: column;
	row-gap: 1rem;
	}

reCAPTCHA: Elig Index, Elig Confirm - Add reCAPTCHA text, hide flag #2586

Are you sure you want to change the base?

reCAPTCHA: Elig Index, Elig Confirm - Add reCAPTCHA text, hide flag #2586

Conversation

machikoyasuda commented Dec 11, 2024 • edited Loading

What this PR does

Testing notes

Screenshots

github-actions bot commented Dec 11, 2024 • edited Loading

Coverage report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

angela-tran Dec 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

machikoyasuda commented Dec 12, 2024

thekaveman left a comment

Choose a reason for hiding this comment

machikoyasuda commented Dec 16, 2024 • edited Loading

angela-tran left a comment

Choose a reason for hiding this comment

angela-tran Dec 17, 2024 • edited Loading

Choose a reason for hiding this comment

angela-tran commented Dec 17, 2024 • edited Loading

machikoyasuda commented Dec 18, 2024

Choose a reason for hiding this comment

machikoyasuda commented Dec 11, 2024 •

edited

Loading

github-actions bot commented Dec 11, 2024 •

edited

Loading

angela-tran Dec 17, 2024 •

edited

Loading

machikoyasuda commented Dec 16, 2024 •

edited

Loading

angela-tran Dec 17, 2024 •

edited

Loading

angela-tran commented Dec 17, 2024 •

edited

Loading