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

Feat: Login.gov help modal #1524

Merged
merged 7 commits into from Jul 13, 2023
Merged

Feat: Login.gov help modal #1524

merged 7 commits into from Jul 13, 2023

Conversation

angela-tran
Copy link
Member

Closes #1473

@angela-tran angela-tran added this to the Veterans milestone Jul 12, 2023
@angela-tran angela-tran self-assigned this Jul 12, 2023
@angela-tran
Copy link
Member Author

angela-tran commented Jul 12, 2023

There's some inconsistent behavior with closing the modal and what happens to the radio button.

See this GIF where I close the modal by:

  • clicking the X
  • pressing the Esc key
  • clicking off the modal

In the last case, the radio button gets selected.
benefits-login-gov-help-modal

I'm really close to implementing a fix to make it all consistent (as in, do not select the radio button), but I can't get the CSP nonce value into the script tag 😞

In selection-label--senior.html:

    <script nonce="{{ request.csp_nonce }}">
        const modal = document.getElementById("login-gov-help");
        modal.addEventListener('click', function (event) {
            event.preventDefault();
          });
    </script>

the nonce is not there.

If I put that code into base.html it works, but that's not an appropriate place for it.

Maybe we just call this out of scope for now.

@angela-tran angela-tran marked this pull request as ready for review July 12, 2023 21:44
@angela-tran angela-tran requested a review from a team as a code owner July 12, 2023 21:44
@angela-tran
Copy link
Member Author

Renamed the modal file

@thekaveman
Copy link
Member

I'm really close to implementing a fix to make it all consistent (as in, do not select the radio button), but I can't get the CSP nonce value into the script tag 😞

Hmm this is a bummer 🤔

I wonder if this is because you're inside an include and request.csp_nonce isn't available there?

Have you tried the context_processor version of {{ CSP_NONCE }}?

@angela-tran
Copy link
Member Author

Have you tried the context_processor version of {{ CSP_NONCE }}?

Yeah, it's the same situation (not available in the include).

@thekaveman
Copy link
Member

thekaveman commented Jul 12, 2023

An included template is rendered within the context of the template that includes it.

From https://docs.djangoproject.com/en/4.2/ref/templates/builtins/#include

Lame, seems like it should work??

EDIT: it does work in other includes, like analytics.html ?

@angela-tran
Copy link
Member Author

Hmm, it might have to do with this being a form widget template.

When rendering the form (which is an include), we still have the context:
image

We lose the context once we're rendering the widget.

image

@angela-tran
Copy link
Member Author

Someone made a helper library for this very purpose: https://github.com/dmptrluke/django-csp-helpers#form-widgets 👀

@thekaveman
Copy link
Member

Also this SO post from 2013! https://stackoverflow.com/a/16922285/453168

@angela-tran
Copy link
Member Author

angela-tran commented Jul 12, 2023

Also this SO post from 2013! https://stackoverflow.com/a/16922285/453168

Such profit!

I propagated the csp_nonce through to the widget in 67f7570. Let me know if that looks ok or not.

@thekaveman
Copy link
Member

Also this SO post from 2013! https://stackoverflow.com/a/16922285/453168

Such profit!

🤣

@@ -371,6 +375,10 @@ footer .footer-links li a:visited {
line-height: 1;
}

.eligibility-index #login .fallback-text {
display: inline-block;
Copy link
Member

Choose a reason for hiding this comment

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

This is so the ? icon stays on the same line as the Login.gov image in mobile.

Maybe look at refactoring this rule in the future (to not be inside a media query): https://github.com/cal-itp/benefits/blob/dev/benefits/static/css/styles.css#L385

@angela-tran angela-tran merged commit 83c716d into dev Jul 13, 2023
13 checks passed
@angela-tran angela-tran deleted the feat/login-gov-help-modal branch July 13, 2023 16:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Eligibility Index - Modal for Login.gov/Eligibility (Senior)
2 participants