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

Refactor: verifier radio template #1489

Merged
merged 7 commits into from Jul 7, 2023
Merged

Conversation

angela-tran
Copy link
Member

@angela-tran angela-tran commented Jul 6, 2023

Part of #1473

Refactors the VerifierRadioSelect to expect a selection_label_template to be given for each verifier, and uses that to render the label and description.

This is solely a code refactor and does not change the front-end result.

Before (dev-benefits.calitp.org) After (localhost)

@angela-tran angela-tran self-assigned this Jul 6, 2023
@angela-tran angela-tran added this to the Veterans milestone Jul 6, 2023
@github-actions github-actions bot added front-end HTML/CSS/JavaScript and Django templates migrations [auto] Review for potential model changes/needed data migrations updates back-end Django views, sessions, middleware, models, migrations etc. deployment-dev [auto] Changes that will trigger a deploy if merged to dev labels Jul 6, 2023
@angela-tran angela-tran force-pushed the refactor/verifier-radio-template branch from 60231ca to ba1e82e Compare July 6, 2023 19:27
make other selection-label-related fields optional
@angela-tran angela-tran force-pushed the refactor/verifier-radio-template branch from ba1e82e to 5941880 Compare July 6, 2023 19:31
@angela-tran angela-tran force-pushed the refactor/verifier-radio-template branch from 2abcf0d to 4ded806 Compare July 6, 2023 23:25
@angela-tran angela-tran marked this pull request as ready for review July 6, 2023 23:28
@angela-tran angela-tran requested a review from a team as a code owner July 6, 2023 23:28
@thekaveman thekaveman mentioned this pull request Jul 7, 2023
4 tasks
Copy link
Member

@machikoyasuda machikoyasuda left a comment

Choose a reason for hiding this comment

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

LGTM!

I tested out changing the includes locally:
image

@thekaveman Maybe on Monday we can discuss naming/folder conventions for all these includes from this PR and #1498. There's both hyphen, underscore going on now. Not necessary to change in these PRs, but something to think about in a future refactor ticket.

image image

Copy link
Member

@thekaveman thekaveman left a comment

Choose a reason for hiding this comment

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

Confirming the rendered HTML for a single radio looks the same as well:

Dev

<div class="radio-input-group d-flex">
  <input class="radio-input rounded-circle flex-grow-0 flex-shrink-0"
         type="radio"
         name="verifier"
         value="4"
         required=""
         id="id_verifier_0">
  <label for="id_verifier_0" class="radio-label">
    <span class="d-block h3">I am 65 years of age or older</span>
    <span class="d-block pt-1">
         You must be 65 years or older. This benefit does not expire,
         but you may need to renew. Using this benefit means your
         new transit fare is half of the standard fare.
      </span>
  </label>
</div>

This PR

<div class="radio-input-group d-flex">
<input class="radio-input rounded-circle flex-grow-0 flex-shrink-0"
       type="radio"
       name="verifier"
       value="4"
       required=""
       id="id_verifier_0">
  <label for="id_verifier_0" class="radio-label">
    <span class="d-block h3">I am 65 years of age or older</span>
    <span class="d-block pt-1">    
        You must be 65 years or older. This benefit does not expire,
        but you may need to renew. Using this benefit means your
        new transit fare is half of the standard fare.
    </span>
  </label>
</div>

with spacing adjusted in each

@angela-tran angela-tran merged commit b4e25bf into dev Jul 7, 2023
13 checks passed
@angela-tran angela-tran deleted the refactor/verifier-radio-template branch July 7, 2023 17:57
@thekaveman thekaveman mentioned this pull request Jul 11, 2023
20 tasks
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 migrations [auto] Review for potential model changes/needed data migrations updates
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants