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

reCAPTCHA: Eligibility index page - Align page and refactor #2570

Merged

Conversation

machikoyasuda
Copy link
Member

@machikoyasuda machikoyasuda commented Dec 5, 2024

closes #2539

What this PR does

This PR primarily makes changes on the Eligibility page, but, in doing this, because of the way we are re-using code or CSS styles in various places, it touches on changes made on other parts of the flow, detailed below.

Desktop Mobile
Screenshot 2024-12-05 at 12-38-59 Choose benefit option Cal-ITP Benefits Screenshot 2024-12-05 at 12-39-38 Choose benefit option Cal-ITP Benefits

Eligibility page

  • Desktop: Align page to col-lg-6, remove the padding start on the form radio container, refactor some CSS
  • Mobile: No regressions
  • Make sure to check every Eligibility page by every agency

All forms in the flow

  • The submit buttons are now 100% wide, which means the agency card form button is also now widened:
image

All pages in the flow

  • <h1> are now left-aligned on desktop and mobile by default, as declared in the CSS, so this will affect all pages:
image image image

Error page, Help page are unaffected by this h1 change:
image

@github-actions github-actions bot added back-end Django views, sessions, middleware, models, migrations etc. deployment-dev [auto] Changes that will trigger a deploy if merged to dev front-end HTML/CSS/JavaScript and Django templates labels Dec 5, 2024
Copy link

github-actions bot commented Dec 5, 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

Comment on lines -561 to -576
--radio-input-gap: calc(24rem / 16);
}

@media (min-width: 992px) {
:root {
--radio-input-gap: calc(15rem / 16);
}
}

.radio-label {
cursor: pointer;
}

.radio-input-group {
gap: var(--radio-input-gap);
}
Copy link
Member Author

Choose a reason for hiding this comment

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

This is a refactor that I made by:

  • Bumping up the 15px gap on desktop to 16px,
  • Allows us to get to place where we are only using 8pt values for both desktop (16px) and mobile (24px)
  • Which then allows us to use gap-4 (4 is 24px) and gap-md-3 (3 is 16px) in the template code.

Now all the radio button CSS styles are around who the element looks, and all the positioning (margin, padding) are done with classes on the page the elements are on.

Copy link
Member

Choose a reason for hiding this comment

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

Bullet points make sense 👍

Not sure I'm following this part:

Now all the radio button CSS styles are around who the element looks

Copy link
Member Author

Choose a reason for hiding this comment

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

how the element looks, rather than how the elements as a whole are positioned onto the page. sorry typo!

Copy link
Member

Choose a reason for hiding this comment

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

Ah, got it


<div {% if id %}id="{{ id }}"{% endif %} class="ps-3 ps-md-0 ps-lg-0 offset-md-1 offset-lg-1 radio-container">
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 didn't see the radio-container class name used in any CSS or JS, so I deleted it. I can't remember why I made this class name, perhaps purely for naming/reading purposes(!?!)

@machikoyasuda machikoyasuda changed the title Feat/2539 recaptcha eligibility reCAPTCHA: Eligibility index page - Align page and refactor Dec 5, 2024
@machikoyasuda machikoyasuda self-assigned this Dec 5, 2024
@machikoyasuda machikoyasuda marked this pull request as ready for review December 5, 2024 20:45
@machikoyasuda machikoyasuda requested a review from a team as a code owner December 5, 2024 20:45
@machikoyasuda machikoyasuda changed the base branch from main to refactor/recaptcha-copy December 5, 2024 20:45
@machikoyasuda machikoyasuda force-pushed the feat/2539-recaptcha-eligibility branch from 8d215cd to 22950f6 Compare December 5, 2024 21:12
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.

Glad to see the spacing implementation is simpler. Noticed a class that can probably be removed?

@machikoyasuda machikoyasuda force-pushed the feat/2539-recaptcha-eligibility branch from 22950f6 to 72b312d Compare December 6, 2024 00:14
@machikoyasuda machikoyasuda merged commit 2ed9222 into refactor/recaptcha-copy Dec 6, 2024
11 checks passed
@machikoyasuda machikoyasuda deleted the feat/2539-recaptcha-eligibility branch December 6, 2024 19:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
back-end Django views, sessions, middleware, models, migrations etc. deployment-dev [auto] Changes that will trigger a deploy if merged to dev front-end HTML/CSS/JavaScript and Django templates
Projects
None yet
Development

Successfully merging this pull request may close these issues.

reCAPTCHA: Eligibility - Realign form, text, button
2 participants