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.

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

Fix/Refactor: Eligibility Start #2583

Merged

Conversation

machikoyasuda
Copy link
Member

@machikoyasuda machikoyasuda commented Dec 10, 2024

After grepping through the codebase for col-lg-8, I realized I had missed 2 Eligibility Start template includes in #2573: MST Courtesy Card and SBMTD were still on col-lg-8, not 6.

While fixing these templates up, and getting tired of repeating the same old code to center the button ( <div class="row justify-content-center"> <div class="col-12 col-lg-6">...), I realized that the main Eligibility Start template could instead inherit call-to-action-button, and use the button centering code from base.html instead. So I cleaned that up here.

Now, if the button style for Eligibility Start changes somehow, we only have to change it in one file (eligibility/start.html), not all the different flow templates.

Lastly, I renamed media-item to eligibility-item. Now the app is truly 1000% free of media-item HTML, CSS and filenames.

@machikoyasuda machikoyasuda self-assigned this Dec 10, 2024
@machikoyasuda machikoyasuda requested a review from a team as a code owner December 10, 2024 08:14
@github-actions github-actions bot added the front-end HTML/CSS/JavaScript and Django templates label Dec 10, 2024
Copy link

github-actions bot commented Dec 10, 2024

Coverage report

Click to see where and how coverage changed

FileStatementsMissingCoverageCoverage
(new stmts)
Lines missing
  benefits/eligibility
  forms.py
Project Total  

This report was generated by python-coverage-comment-action

angela-tran
angela-tran previously approved these changes Dec 10, 2024
Copy link
Member

@angela-tran angela-tran left a comment

Choose a reason for hiding this comment

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

I like it!

I left a thought about some potential consolidation and am fine with whatever you decide on it.

@@ -13,13 +13,13 @@
{% block inner-content %}
<div class="col-lg-6">
<p class="py-4">{% translate "You will need a few items to continue:" %}</p>
<ul class="d-flex flex-column gap-4 list-unstyled ps-0 pb-5 mb-3">
<ul class="d-flex flex-column gap-4 list-unstyled ps-0 mb-0">
Copy link
Member

@angela-tran angela-tran Dec 10, 2024

Choose a reason for hiding this comment

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

Got it, removing all this bottom padding because of how we have pt-8 from the <div> that surrounds the call-to-action-button block 👍

@@ -6,19 +6,15 @@
{% endblock page-title %}

{% block headline %}
<div class="col-lg-8">
<div class="col-lg-6">
Copy link
Member

Choose a reason for hiding this comment

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

👍

Copy link
Member

Choose a reason for hiding this comment

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

We probably could do something on the start.html template so that we don't have to repeat

<div class="col-lg-6">
   <h1>
...
   </h1>
</div>

on every one of these templates.

Something like

{% block headline %}
<div class="col-lg-6">
   <h1>
   {% block headline-text %}
   {% endblock headline-text %}
   </h1>
</div>
{% endblock headline %}

and then the start--<agency>--<flow>.html templates just override the headline-text block.

I had the thought so wrote it down, but I don't feel strongly about doing it or not. It's really not that much duplicate code, but then again we did have to remember to update the 8 to 6 in multiple places. 🤷‍♀️

Copy link
Member Author

Choose a reason for hiding this comment

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

I am thinking about doing this for all templates in base.html in #2540, but I like the idea of starting that effort here for all the start-agency-flow templates!

Copy link
Member Author

Choose a reason for hiding this comment

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

Done! cf11c04 Really like how the start-agency-flow templates are looking now, very clean and stream-lined.

Copy link
Member

@thekaveman thekaveman Dec 10, 2024

Choose a reason for hiding this comment

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

Ditto! I think this is inching us further towards copy that is easier to manage as well!

@@ -6,19 +6,15 @@
{% endblock page-title %}

{% block headline %}
<div class="col-lg-8">
<div class="col-lg-6">
Copy link
Member

Choose a reason for hiding this comment

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

👍

Copy link
Member

@angela-tran angela-tran left a comment

Choose a reason for hiding this comment

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

Nice!

@machikoyasuda machikoyasuda merged commit 7404a9d into refactor/recaptcha-copy Dec 10, 2024
8 checks passed
@machikoyasuda machikoyasuda deleted the fix/recaptcha-elig-start-buttons branch December 10, 2024 21:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
front-end HTML/CSS/JavaScript and Django templates
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants