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

Eligibility Index: Veterans - Use Older Adult copy #1706

Merged
merged 10 commits into from
Sep 11, 2023

Conversation

machikoyasuda
Copy link
Member

@machikoyasuda machikoyasuda commented Sep 6, 2023

This PR depends on #1705 to be merged first.

What this PR does

  • Copy: On Elig Index, for the Veteran pathway, change the last sentence to read: You will need to verify your identity with Login.gov., with a link to the Login.gov modal. Both links from the Veteran and the Older Adult radio input open the same exact modal.
  • Templates: Like in Eligibility Start: Veterans - Use Older Adult copy #1705, rename senior modal to login-gov modal.

Includes, CSS changes

  • CSS: Convert the #login id to a class. This is necessary b/c now, for the first time, a page has more than one Login.gov button on it. This requires changing all calls to the #login id to the class. This affects the Eligibility Start page for Veterans, Eligibility Start page for Older Adults, and the Enrollment Success page.
  • Modal trigger element: Previously the Modal trigger element needed an id field to pass in the id of login, to show the color Login.gov trigger image. Now, though, there is no id necessary to pass in. So I removed the field entirely from the includes template. It was actually causing an interesting bug on the radio button field page, where the id of the radio button field was being used in the modal trigger instead - which was resulting in 2 links with the same id. And that was coming up in a11y testing.

How to test this PR

  • Test the regular app flow
  • Pay attention to all buttons with the Login.gov icon: Elig Start, Log out on Enrollment Success, Login.gov modal triggers. Ensure no visual regressions.
image image image image

@machikoyasuda machikoyasuda self-assigned this Sep 6, 2023
@github-actions github-actions bot added deployment-dev [auto] Changes that will trigger a deploy if merged to dev i18n Copy: Language files or Django i18n framework front-end HTML/CSS/JavaScript and Django templates and removed deployment-dev [auto] Changes that will trigger a deploy if merged to dev labels Sep 6, 2023
@machikoyasuda machikoyasuda linked an issue Sep 6, 2023 that may be closed by this pull request
1 task
@machikoyasuda machikoyasuda changed the title Elig Index: Veterans - Change copy to Older Adult copy Eligibility Index: Veterans - Change copy to Older Adult copy Sep 6, 2023
@machikoyasuda machikoyasuda changed the title Eligibility Index: Veterans - Change copy to Older Adult copy Eligibility Index: Veterans - Use Older Adult copy Sep 6, 2023
@@ -375,6 +375,8 @@ footer .footer-links li a.footer-link:visited {

.btn.btn-lg.btn-primary:focus,
.btn.btn-lg.btn-primary:focus-visible,
.btn.btn-lg:focus,
.btn.btn-lg:focus-visible,
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 for the Login.gov Sign Out button on the Enrollment Success page for the Login.gov flows.

{% endblocktranslate %}

{% include "core/includes/modal-trigger.html" with classes="border-0 bg-transparent p-0 login" modal="modal--login-gov" login=True period=True %}
Copy link
Member Author

Choose a reason for hiding this comment

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

@thekaveman @angela-tran This modal trigger link opens the modal--login-gov modal, which is included into the page from the selection-label--senior.html file. That means that technically, under this design, if a transit agency ONLY offers Veterans benefits, but doesn't offer an Older Adult one, this template will have to be modified to also include the modal code. In other words, the Veteran selection label would require the Older Adult one to also be there.

Alternatively, I could also add this line here:

{% include "eligibility/includes/modal--login-gov-help.html" with id="modal--login-gov" size="modal-lg" header="p-md-2 p-3" body="pb-md-3 mb-md-3 mx-md-3 py-0 pt-0 absolute-top" %}

but then this HTML modal code would be duplicated on this page, and since there would be 2 modals with the same id (modal--login-gov), we'd have to have 2 different IDs pointing to duplicated HTML. Not sure which approach is better.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, that is tricky. With your second idea - the one about including the login-gov-help modal in the veteran selection-label - maybe the selection-label include templates can have a variable that says whether to include the modal or not?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hrmm - yeah, but that variable would have to live in 0002_data.py because that's where the radio button selector templates get required for each agency:
image

And that seems like a little too much for this situation?

Might just be better to go with adding the modal all the time just to make each selection template independent.

Copy link
Member Author

Choose a reason for hiding this comment

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

Update: ff4ee18

The page now has 2 modals with 2 different IDs. The Veteran selector will have a modal with the ID "modal--login-gov-veterans" and the trigger will open that one.

Copy link
Member

Choose a reason for hiding this comment

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

And that seems like a little too much for this situation?

Might just be better to go with adding the modal all the time just to make each selection template independent.

Oh I see. Yeah, I didn't look too deeply at how the variable would get passed in, and what you're saying makes sense. Seems like having 2 modals is our best option here

@machikoyasuda machikoyasuda marked this pull request as ready for review September 6, 2023 23:05
@machikoyasuda machikoyasuda requested a review from a team as a code owner September 6, 2023 23:05
@machikoyasuda
Copy link
Member Author

Ready for review @thekaveman @angela-tran

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.

👍

@machikoyasuda machikoyasuda merged commit 22e7def into dev Sep 11, 2023
8 checks passed
@machikoyasuda machikoyasuda deleted the feat/1692-veterans-elig-index branch September 11, 2023 19:22
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 i18n Copy: Language files or Django i18n framework
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Revise supporting copy for US Veteran option in Eligibility view.
3 participants