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

Enrollment Index: Remove space after Littlepay #2456

Merged
merged 1 commit into from
Oct 15, 2024

Conversation

machikoyasuda
Copy link
Member

closes #2428

What this PR does

  • Make a fix to the Enrollment Index page that shows up on the Agency Card, Veterans, Senior, Medicare flows (all but CalFresh).
  • Remove the space that appears after the Littlepay link by not using the includes and using the a href link code directly, allowing the link code to be right next to the copy, which allows the HTML to be rendered w/o any extra whitespaces.
  • The only drawback to this is that if there is ever a style change to modal-trigger, we need to make sure the change is made here too. I found this drawback to be acceptable, b/c I think most changes if any that happen to the modal-trigger will be done with CSS classes, and it wouldn't be hard to add the CSS classes in both places. I'm open to the alternatives considered though.
image image image

Alternatives considered

  • Tried removing line breaks from the modal-trigger.html template, from the media-item template, but didn't fix the bug.
  • I considered trying to add conditional code to the modal-trigger.html includes, something like If no_space=True, then render {% translate ", to enter your contactless card details." %} - but it felt a little over the top, doesn't scale if this ever happened on another page. Would this be preferred though? I can do that.

@machikoyasuda machikoyasuda self-assigned this Oct 15, 2024
@machikoyasuda machikoyasuda requested a review from a team as a code owner October 15, 2024 20:38
@github-actions github-actions bot added deployment-dev [auto] Changes that will trigger a deploy if merged to dev front-end HTML/CSS/JavaScript and Django templates labels Oct 15, 2024
Copy link

Coverage report

Click to see where and how coverage changed

FileStatementsMissingCoverageCoverage
(new stmts)
Lines missing
  benefits
  routes.py
  sentry.py
  benefits/core
  models.py
  views.py
  benefits/eligibility
  views.py
  benefits/enrollment
  enrollment.py
  views.py
  benefits/oauth
  views.py
Project Total  

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

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.

Yep, I agree your approach is simpler and fine to maintain compared to the alternatives.

Modal link works ✅

English ✅
dev local
image image
Spanish ✅
dev local
image image

@machikoyasuda machikoyasuda merged commit 717d232 into main Oct 15, 2024
13 checks passed
@machikoyasuda machikoyasuda deleted the fix/2428-littlepay-space branch October 15, 2024 22:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

Enrollment page: Space after Littlepay
2 participants