Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

reCAPTCHA: Enrollment Success, Logged Out #2579

Merged
merged 5 commits into from
Dec 10, 2024
Merged

reCAPTCHA: Enrollment Success, Logged Out #2579

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
2b50e4b
feat(enrollment-success): widen header to 9, to match content
machikoyasuda Dec 6, 2024
31722ca
feat(logged-out): update logged out page with new spacing
machikoyasuda Dec 6, 2024
45351bd
feat(enrollment-success): don't forget cta log out sentence
machikoyasuda Dec 6, 2024
9059962
refactor: reduce divitis on enrollment-success
machikoyasuda Dec 6, 2024
279fc00
fix(logged-out): fix padding from top
machikoyasuda Dec 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 3 additions & 5 deletions benefits/core/templates/core/logged-out.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,10 @@

{% block main-content %}
<div class="container">
<div class="row justify-content-lg-center">
<div class="row justify-content-center">
<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>
Copy link
Member Author

@machikoyasuda machikoyasuda Dec 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Putting a span with a d-block to wrap an img...... so weird! Changed to a div.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, weird also that the image/icon was inside the h1. This makes way more sense 👍

{% translate "You have successfully logged out. Thank you for using Cal-ITP Benefits!" %}
</h1>
<div class="py-4 mt-5 text-center">{% include "core/includes/icon.html" with name="happybus" %}</div>
<h1 class="h2 pt-0">{% translate "You have successfully logged out. Thank you for using Cal-ITP Benefits!" %}</h1>
</div>
</div>
</div>
Expand Down
21 changes: 8 additions & 13 deletions benefits/enrollment/templates/enrollment/success.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
{% endblock page-title %}

{% block headline %}
<div class="col-lg-7">
<div class="col-lg-9">
<h1 class="pb-lg-4 mb-lg-3 pb-4">
{% block headline-message %}
{% blocktranslate trimmed %}
Expand All @@ -24,7 +24,7 @@ <h1 class="pb-lg-4 mb-lg-3 pb-4">
{% endblock headline %}

{% block inner-content %}
<div class="col-12 col-sm-12 col-lg-9">
<div class="col-12 col-lg-9">
<div class="row flex-column-reverse flex-lg-row">
<div class="col-12 col-lg-7 pe-1">
{# djlint:off #}
Expand Down Expand Up @@ -52,18 +52,13 @@ <h2 class="h3 mt-lg-3 mb-1">{% translate "Your benefit will expire on" %} {{ enr
alt="" />
</div>
</div>
</div>
{% endblock inner-content %}

{% block call-to-action %}
{% if authentication and authentication.logged_in and authentication.sign_out_button_template %}
<div class="row d-flex justify-content-start justify-content-lg-center">
<div class="col-12 col-lg-8 pt-lg-5 mt-lg-0 pt-4 mt-2">
<p>
{% if authentication and authentication.logged_in and authentication.sign_out_button_template %}
<div class="row justify-content-start justify-content-lg-center">
<p class="pt-lg-5 mt-lg-0 pt-4 mt-2">
{% 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 %}
Copy link
Member Author

Choose a reason for hiding this comment

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

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).

Copy link
Member

Choose a reason for hiding this comment

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

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
image image

Could you explain more what you mean by:

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).

? I'm not fully following and am curious what it means to be more specific with the call-to-action block.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

{% endif %}
</div>
{% endblock inner-content %}
Loading