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 Success: Reduce h1 parent container width for readability #2455

Merged
merged 1 commit into from
Oct 18, 2024

Conversation

machikoyasuda
Copy link
Member

@machikoyasuda machikoyasuda commented Oct 15, 2024

closes #2429

What this PR does

  • Reduces the H1 parent container width from col-8 to col-7, only on the Enrollment Success page, to get the headline to break into a second line at a semantic spot:
image image image
  • Alternatives considered: Use a manual <br> but that doesn't scale responsively well, and it's really hacky.

Note

A note to other developers - this is a bit of a heavy-handed custom override, and we (dev) don't want to be doing things like this too often. We want to be building an app that exists and works great in all browser-widths and all browser dimensions as possible. If the col-7 trick didn't just happen to conveniently work, I wouldn't pursue this fix. This is truly a one-off case.

@machikoyasuda machikoyasuda self-assigned this Oct 15, 2024
@machikoyasuda machikoyasuda requested a review from a team as a code owner October 15, 2024 19:58
@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.

I lean towards the <br> for this one.

I don't feel it is hacky, but actually more correct? Instead of a CSS rule that happens to work on this specific copy on this specific page (to make the visual reading of the sentence we want), <br> is explicit about this. Semantically this is one sentence, and <br> adds nothing either way, so we're not in trouble of e.g. a screenreader like if had we used <br> to split up multiple paragraphs.

We also use <br> on the enrollment:index page for a similar effect, to break two sentences that form the headline.

I think that is appropriate here too.

@machikoyasuda
Copy link
Member Author

I don't think the
is an applicable alternative though b/c it leads to breaking into 3 lines like this at around 1200px width:
image

and look like this on mobile:
image

To take care of the mobile case, you could add a class to add display: none to the <br> at a specific breakpoint. We could even use that on the desktop case too. But that probably wouldn't work with the Spanish. We'd then have to add a <br> in the Spanish at the same point, and hope that the Spanish doesn't break into 3 or 4 lines at different points.

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.

Gotcha, thanks for the explanation/screenshots 😭

@thekaveman
Copy link
Member

@machikoyasuda I'm going to merge this.

@thekaveman thekaveman merged commit c396d09 into main Oct 18, 2024
13 checks passed
@thekaveman thekaveman deleted the fix/2429-enrollment-success-h1 branch October 18, 2024 16:29
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 success: Header width
2 participants