Skip to content

Fix placement of disclaimers in new reCAPTCHA screen#9258

Merged
jmdembe merged 8 commits intomainfrom
LG-10730-fix-disclaimer-placement
Sep 26, 2023
Merged

Fix placement of disclaimers in new reCAPTCHA screen#9258
jmdembe merged 8 commits intomainfrom
LG-10730-fix-disclaimer-placement

Conversation

@jmdembe
Copy link
Contributor

@jmdembe jmdembe commented Sep 25, 2023

🎫 Ticket

LG-10730
Link to Figma

🛠 Summary of changes

This PR moves the reCAPTCHA disclosure to be in a more intuitive location which is above the "Choose another method" link

👀 Screenshots

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

Before:

Screenshot of current add a phone page

After:

Screenshot of updated add a phone page

@jmdembe jmdembe marked this pull request as ready for review September 25, 2023 16:51
@jmdembe jmdembe requested a review from a team September 25, 2023 16:51
Copy link
Contributor

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

@jmdembe jmdembe changed the title Implementation: Fix placement of disclaimers in new reCAPTCHA screen Fix placement of disclaimers in new reCAPTCHA screen Sep 25, 2023
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.

Minor comment about partial margins, but LGTM otherwise 👍

<%= render 'shared/cancel_or_back_to_options' %>

<% if FeatureManagement.phone_recaptcha_enabled? %>
<%= render 'shared/recaptcha_disclosure' %>
Copy link
Contributor

Choose a reason for hiding this comment

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

With this no longer being at the bottom of the page, I think we could revisit the margins on the partial:

<p class="margin-top-4 margin-bottom-0">

Specifically wondering if we could remove those utility classes, and rely on margins from the submit button (2.5rem bottom) and footer section (2rem top) instead.

I'm also not sure it makes a lot of sense to be a partial anymore as well, now that we have a single page for phone setups. We could inline it here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Definitely agree on removing the partial and moving the disclosure to be inline.

We can remove the margin-bottom-0 for the class with no adverse effect. The send code button does not naturally have margin on it, so we would still have to keep the margin-top-4

image

Copy link
Contributor

Choose a reason for hiding this comment

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

The send code button does not naturally have margin on it, so we would still have to keep the margin-top-4

Good point. I'd wonder if we'd want the margin on that instead. Doesn't make much of a difference either way. I'd expect the unit should probably change from 4 to 5 at least, for the standard vertical margins on primary buttons.

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.

LGTM 👍

@jmdembe jmdembe merged commit 8746fd2 into main Sep 26, 2023
@jmdembe jmdembe deleted the LG-10730-fix-disclaimer-placement branch September 26, 2023 13:46
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.

3 participants