#3190: Heading refactor and other CSS fixes - [RJM] #3358

rachidatecs · 2025-01-16T22:22:06Z

Ticket

Resolves #3190

This was a collaboration with @gabydisarli. We primarily set out to fix headers across the app, but ended up fixing some minor issues along the way. Also, the header refactor touched some components a bit more than others, like summary items.

Changes

Refactor headers
Fix icons in buttons and links
Summary box color consolidation

Context for reviewers

Summary items code was heavily affected by the refactors

New CSS writing rules

Not sure whether this goes into documentation somewhere, but this is something anyone working on templates and CSS should know:

We only override margins (and in some instances padding) on headers using USWDS utility classes. We do not override colors, font size, or line height. We do not create custom classes that do so. We do not use CSS specificity to do so.
On headers, we now have classes .h1, .h2 .h3 and .h4 in case you semantically need a, eg, h3 but you want it to look like an h4.

Setup

Code Review Verification Steps

As the original developer, I have

Satisfied acceptance criteria and met development standards

Met the acceptance criteria, or will meet them in a subsequent PR
Created/modified automated tests
Update documentation in READMEs and/or onboarding guide

Ensured code standards are met (Original Developer)

If any updated dependencies on Pipfile, also update dependencies in requirements.txt.
Interactions with external systems are wrapped in try/except
Error handling exists for unusual or missing values

Validated user-facing changes (if applicable)

Tag @dotgov-designers in this PR's Reviewers for design review. If code is not user-facing, delete design reviewer checklist
Verify new pages have been added to .pa11yci file so that they will be tested with our automated accessibility testing
Checked keyboard navigability
Tested general usability, landmarks, page header structure, and links with a screen reader (such as Voiceover or ANDI)

As a code reviewer, I have

Reviewed, tested, and left feedback about the changes

Pulled this branch locally and tested it
Verified code meets all checks above. Address any checks that are not satisfied
Reviewed this code and left comments. Indicate if comments must be addressed before code is merged
Checked that all code is adequately covered by tests
Verify migrations are valid and do not conflict with existing migrations

Validated user-facing changes as a developer

Note: Multiple code reviewers can share the checklists above, a second reviewer should not make a duplicate checklist. All checks should be checked before approving, even those labeled N/A.

New pages have been added to .pa11yci file so that they will be tested with our automated accessibility testing
Checked keyboard navigability
Meets all designs and user flows provided by design/product
Tested general usability, landmarks, page header structure, and links with a screen reader (such as Voiceover or ANDI)
(Rarely needed) Tested as both an analyst and applicant user

As a designer reviewer, I have

Verified that the changes match the design intention

Checked that the design translated visually
Checked behavior. Comment any found issues or broken flows.
Checked different states (empty, one, some, error)
Checked for landmarks, page heading structure, and links

Validated user-facing changes as a designer

Checked keyboard navigability
Tested general usability, landmarks, page header structure, and links with a screen reader (such as Voiceover or ANDI)
Tested with multiple browsers (check off which ones were used)
- Chrome
- Microsoft Edge
- FireFox
- Safari
(Rarely needed) Tested as both an analyst and applicant user

References

Code review best practices

Screenshots

This is not a comprehensive list of changes:

Icons before:

Icons after:

Manage request before:

Manage request after (matches manage Domain):

github-actions · 2025-01-16T22:41:43Z

🥳 Successfully deployed to developer sandbox rjm.

gabydisarli · 2025-01-16T23:01:02Z

Hey @rachidatecs I actually had someone change all the icons to the smaller size a while ago (I Can't remember what PR it was). I'd like to keep them small. Is that possible?

CocoByte · 2025-01-17T06:14:47Z

src/registrar/templates/includes/portfolio_request_review_steps.html

@@ -46,7 +46,7 @@
        {% endwith %}

        {% if domain_request.alternative_domains.all %}
-            <h3 class="header--body text-primary-dark margin-bottom-0">Alternative domains</h3>
+            <h3 class="h4 margin-bottom-0">Alternative domains</h3>


This seems really counter-intuitive. If I didn't read through the css, I would want to "correct" this so we are using h4 for the element instead of as a class for an h3 element....

This is a pattern I learned from Bootstrap. Just remember that the DOM element is driven by semantics while the CSS class is presentational.

So any dev's thought process should be:
1- What should this header be? It's a new section and the parent section starts with a h2, so h3 makes the most sense
2- Now that I have chosen h3, I want it to look like an h4. Instead of messing with custom class names like we used to, font size and font weight and line height styles or utility classes, simply use class h4 - again, CSS classes are presentational

For more, read through the first part of this Bootstrap documentation page

@rachidatecs I know we talked about this but, this could be a good example to use for that new documentation/guidance we'd like to share on how semantic use of headings vs visual.

CocoByte · 2025-01-17T06:16:27Z

src/registrar/templates/includes/request_review_steps.html

@@ -132,8 +132,8 @@ <h3 class="header--body text-primary-dark margin-bottom-0">Alternative domains</
        {% with title=form_titles|get_item:step %}
            {% if domain_request.has_additional_details %}
                {% include "includes/summary_item.html" with title="Additional Details" value=" " heading_level=heading_level editable=is_editable edit_link=domain_request_url %}
-                <h3 class="header--body text-primary-dark margin-bottom-0">CISA Regional Representative</h3>
-                <ul class="usa-list usa-list--unstyled margin-top-0">
+                <h4 class="margin-bottom-0">CISA Regional Representative</h4>


^ This is the change I REALLY want to do for the <h3 class="h4...> elements...

CocoByte

I see what is happening and OVERALL, I like it alot.....except for one area that I am sure you have your reasons for, but I left a couple comments anyway.

github-actions · 2025-01-17T20:24:12Z

🥳 Successfully deployed to developer sandbox rjm.

github-actions · 2025-01-17T20:53:11Z

🥳 Successfully deployed to developer sandbox rjm.

CocoByte

Good huddle today. Great demo presentation. I see the changes. LGTM

gabydisarli · 2025-01-22T00:35:36Z

@rachidatecs H1 - seeing wrong font-size (2.13rem). We normalized to 2.125rem so that the px number is 34px. Can we update h1 font-size to 2.125 rem?

gabydisarli

Conditionally approve with requested changes. Will you let me know what you end up doing with the summary box heading? I think the heading should look more like the Preamble than the DNSSEC version (bold).

gabydisarli · 2025-01-22T01:09:54Z

I think there's something funky still going on with the summary box component. I understand we have limitations here because of what USWDS does. But when comparing the headings of the summary box on the DNSSEC enabled page to the summary box on the Preamble, they seem very different. I thought we wanted to make them appear the same even if semantically we need to use another level. (See screenshots)

rachidatecs · 2025-01-22T19:29:02Z

@rachidatecs H1 - seeing wrong font-size (2.13rem). We normalized to 2.125rem so that the px number is 34px. Can we update h1 font-size to 2.125 rem?

I traced this to browser rounding: "Browsers prioritize rendering performance over extremely precise decimal calculation"

github-actions · 2025-01-22T19:37:47Z

🥳 Successfully deployed to developer sandbox rjm.

rachidatecs · 2025-01-22T19:55:13Z

I think there's something funky still going on with the summary box component. I understand we have limitations here because of what USWDS does. But when comparing the headings of the summary box on the DNSSEC enabled page to the summary box on the Preamble, they seem very different. I thought we wanted to make them appear the same even if semantically we need to use another level. (See screenshots) ...

Fixed. USWDS summary box header styles excluded font-weight, I normalized it to bold.

github-actions · 2025-01-22T20:00:48Z

🥳 Successfully deployed to developer sandbox rjm.

gabydisarli

Looks great!

github-actions · 2025-01-22T22:47:20Z

🥳 Successfully deployed to developer sandbox rjm.

rachidatecs added 7 commits January 10, 2025 15:32

normalize summary box

1bc83a1

use typeset mixin instead of hardcoded values in summary-box

7dbb9de

clean up summary boxes

0e9d69a

header revision wip

20e8ad5

Header refactor wip

3337e5d

cleanup

52ba794

More header typography, fix ison buttons and links

52734fb

rachidatecs requested a review from abroddrick as a code owner January 16, 2025 22:22

cleanup

631288d

rachidatecs requested review from lizpearl, therealslimhsiehdy, Matt-Spence, CocoByte, dave-kennedy-ecs, erinysong, zandercymatics and a team January 16, 2025 22:25

merge main

53e3149

CocoByte reviewed Jan 17, 2025

View reviewed changes

make icons small again

1b635e7

fix an h3 that should have been an h4

1388b5b

CocoByte self-assigned this Jan 17, 2025

CocoByte approved these changes Jan 17, 2025

View reviewed changes

gabydisarli approved these changes Jan 22, 2025

View reviewed changes

rachidatecs changed the title ~~#3190: Heading refactor and other CSS fixes~~ #3190: Heading refactor and other CSS fixes - [RJM] Jan 22, 2025

Fix font weight on summary header box

7c633c0

merge main

6daf328

gabydisarli approved these changes Jan 22, 2025

View reviewed changes

Merge branch 'main' into rjm/3190-heading-refactor

c3d7172

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#3190: Heading refactor and other CSS fixes - [RJM] #3358

#3190: Heading refactor and other CSS fixes - [RJM] #3358

rachidatecs commented Jan 16, 2025 •

edited

Loading

github-actions bot commented Jan 16, 2025

gabydisarli commented Jan 16, 2025

CocoByte Jan 17, 2025

rachidatecs Jan 17, 2025

rachidatecs Jan 17, 2025

gabydisarli Jan 17, 2025

CocoByte Jan 17, 2025 •

edited

Loading

CocoByte left a comment

github-actions bot commented Jan 17, 2025

github-actions bot commented Jan 17, 2025

CocoByte left a comment

gabydisarli commented Jan 22, 2025 •

edited

Loading

gabydisarli left a comment

gabydisarli commented Jan 22, 2025

rachidatecs commented Jan 22, 2025

github-actions bot commented Jan 22, 2025

rachidatecs commented Jan 22, 2025

github-actions bot commented Jan 22, 2025

gabydisarli left a comment

github-actions bot commented Jan 22, 2025

#3190: Heading refactor and other CSS fixes - [RJM] #3358

Are you sure you want to change the base?

#3190: Heading refactor and other CSS fixes - [RJM] #3358

Conversation

rachidatecs commented Jan 16, 2025 • edited Loading

Ticket

Changes

Context for reviewers

New CSS writing rules

Setup

Code Review Verification Steps

As the original developer, I have

Satisfied acceptance criteria and met development standards

Ensured code standards are met (Original Developer)

Validated user-facing changes (if applicable)

As a code reviewer, I have

Reviewed, tested, and left feedback about the changes

Validated user-facing changes as a developer

As a designer reviewer, I have

Verified that the changes match the design intention

Validated user-facing changes as a designer

References

Screenshots

github-actions bot commented Jan 16, 2025

gabydisarli commented Jan 16, 2025

CocoByte Jan 17, 2025

Choose a reason for hiding this comment

rachidatecs Jan 17, 2025

Choose a reason for hiding this comment

rachidatecs Jan 17, 2025

Choose a reason for hiding this comment

gabydisarli Jan 17, 2025

Choose a reason for hiding this comment

CocoByte Jan 17, 2025 • edited Loading

Choose a reason for hiding this comment

CocoByte left a comment

Choose a reason for hiding this comment

github-actions bot commented Jan 17, 2025

github-actions bot commented Jan 17, 2025

CocoByte left a comment

Choose a reason for hiding this comment

gabydisarli commented Jan 22, 2025 • edited Loading

gabydisarli left a comment

Choose a reason for hiding this comment

gabydisarli commented Jan 22, 2025

rachidatecs commented Jan 22, 2025

github-actions bot commented Jan 22, 2025

rachidatecs commented Jan 22, 2025

github-actions bot commented Jan 22, 2025

gabydisarli left a comment

Choose a reason for hiding this comment

github-actions bot commented Jan 22, 2025

rachidatecs commented Jan 16, 2025 •

edited

Loading

CocoByte Jan 17, 2025 •

edited

Loading

gabydisarli commented Jan 22, 2025 •

edited

Loading