reCAPTCHA: Enrollment Success, Logged Out #2579
Conversation
Coverage report
This PR does not seem to contain any modification to coverable code. |
| <div class="col-md-6"> | ||
| <h1 class="h2 text-center"> | ||
| <span class="d-block pb-lg-8 pb-5">{% include "core/includes/icon.html" with name="happybus" %}</span> |
There was a problem hiding this comment.
Putting a span with a d-block to wrap an img...... so weird! Changed to a div.
There was a problem hiding this comment.
Yeah, weird also that the image/icon was inside the h1. This makes way more sense 👍
| {% translate "If you are on a public or shared computer, don’t forget to sign out of " %} | ||
| {% include authentication.sign_out_button_template %} | ||
| </p> | ||
| </div> | ||
| </div> | ||
| {% endif %} | ||
| {% endblock call-to-action %} |
There was a problem hiding this comment.
The current file has the If you are on a public or shared computer.. line in the call-to-action area. But I moved it to the parent block container, inner-content instead. In the future, this will allow us to be more specific with what the call-to-action block has in it (if for example we ONLY use it for the majority case where there's just 1 big button).
There was a problem hiding this comment.
Ok so I think the idea here with moving it up to inner-content was so it can be inside the <div> defined on line 27 (<div class="col-12 col-sm-12 col-lg-9">) and therefore not have to duplicate the col- classes.
Ultimately the spacing / alignment looks correct ✅ :
| Before | This branch |
|---|---|
 |
 |
Could you explain more what you mean by:
In the future, this will allow us to be more specific with what the
call-to-actionblock has in it (if for example we ONLY use it for the majority case where there's just 1 big button).
? I'm not fully following and am curious what it means to be more specific with the call-to-action block.
There was a problem hiding this comment.
Sorry that line was confusing 😆 , as I'm wondering to myself what it means 😆
I'm gonna retract that statement entirely. #2583 (comment) achieved what I meant in that confusing sentence.
Instead I'm going to say, although we can put the If you are on a public computer sentence in either inner-content or call-to-action, I think we should keep it in inner-content b/c it's not really the same design/intent as the call-to-action block, which is really meant to be the container of the big CTA buttons. So it's more of a semantics decision rather than a code change.
Just in case in the future, there's some other new re-design to the call-to-action button / parent area, and we want to apply it to all the big buttons - it would be easier to do if we're only using this block only had the big buttons.
machikoyasuda
force-pushed
the
feat/2544-recaptcha-enrollment-success
branch
from
c892e58 to
9059962
Compare
|
@angela-tran This PR is ready for review. (Goes a little out of order from what I had listed! It's a small PR though, compared with the others). |
|
@machikoyasuda I see that this PR contains some changes that do make the code cleaner, namely the I'm not sure how to proceed with my code review of this. |
|
@angela-tran Hrm.. I'm just going by what alignment changes were made on Figma: un-centering the H1s, making sure the H1 is 72px from the top, 24px between grafs. The most visibly noticeable change is the un-centering the H1s. |
Ah yeah, the headline becoming un-centered and the 24px spacing change are all from the reCAPTCHA design changes 👍 I was getting caught up in the Figma design not following the grid and calling for the content to be wider for some reason, but I guess it's fine. Thanks for clarifying and sending me the link to the Figma diff -- it was really helpful! |
| {% translate "If you are on a public or shared computer, don’t forget to sign out of " %} | ||
| {% include authentication.sign_out_button_template %} | ||
| </p> | ||
| </div> | ||
| </div> | ||
| {% endif %} | ||
| {% endblock call-to-action %} |
There was a problem hiding this comment.
Ok so I think the idea here with moving it up to inner-content was so it can be inside the <div> defined on line 27 (<div class="col-12 col-sm-12 col-lg-9">) and therefore not have to duplicate the col- classes.
Ultimately the spacing / alignment looks correct ✅ :
| Before | This branch |
|---|---|
|
|
Could you explain more what you mean by:
In the future, this will allow us to be more specific with what the
call-to-actionblock has in it (if for example we ONLY use it for the majority case where there's just 1 big button).
? I'm not fully following and am curious what it means to be more specific with the call-to-action block.
closes #2544
What this PR does
Enrollment Success
This is now the only page in the flow that isn't
col-lg-6. Though, as a reminder, the home page and the Help page aren't either. The Help page will remaincol-lg-8. This page doesn't by any columns on Figma, both the headline text and the image/text combo. On the app, we are using acol-lg-9as our closest approximate.Logged out
A
col-lg-6page with the now standard, 72px between header and headline, and 24px between icon and graf.