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

Replace title components with heading components #3564

Merged
merged 3 commits into from
Feb 19, 2025

Conversation

JamesCGDS
Copy link
Contributor

⚠️ This repo is Continuously Deployed: make sure you follow the guidance ⚠️

Follow these steps if you are doing a Rails upgrade.

What

This PR replaces instances of the title component with the heading component.

Why

Trello card

Visual changes

There is a very small decrease in spacing above the heading on larger screen sizes due to introducing the govuk-main-wrapper class on the main element to handle spacing. On smaller screen sizes, spacing has been introduced above the heading where it didn't exist before (screenshots below).

Due to the very small difference on larger screens and the benefit of additional spacing on smaller screens, I'd argue these changes aren't a blocker. Happy to discuss though.

Before (desktop) After (desktop)
image image
Before (mobile) After (mobile)
image image

Update templates to call the heading component; update presenters (rename instances of TitleAndContext/title_and_context to HeadingAndContext/heading_and_context; replace t.delete(:average_title_length) with t[:font_size] = 'xl' (average_title_length is used by the title component to set either a large or extra large font size of the heading however the heading component does not use this option. Therefore, we directly set the heading size to large in heading_and_context.rb and use the override seen here when we want an extra large font size); update/fix tests)
This reinstates spacing that was introduced by the title component (and also the main element should have this class anyway). The class is excluded from html_publication, worldwide_corporate_information, worldwide_office, and worldwide_organisation templates as it introduces too much spacing. Additionally, using the 'govuk-main-wrapper' class makes the instances of 'responsive-top-margin' redundant as the latter now introduces excess spacing. Therefore they have been removed.
During testing, I noticed this margin top override was now introducing unnecessary spacing after implementing the 'govuk-main-wrapper' class in the previous commit, and therefore I've removed it.
@govuk-ci govuk-ci temporarily deployed to government-frontend-pr-3564 February 19, 2025 13:20 Inactive
Copy link
Contributor

@AshGDS AshGDS left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Copy link
Contributor

@KludgeKML KludgeKML left a comment

Choose a reason for hiding this comment

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

LGTM (I feel like the main-wrapper business is going to bite us when doing consolidation work, but it's not like it's our biggest problem there, so better to do it this way here and then solve as we move routes over.)

@JamesCGDS JamesCGDS merged commit 3af25ad into main Feb 19, 2025
12 checks passed
@JamesCGDS JamesCGDS deleted the replace_title_components_with_heading_components branch February 19, 2025 13:51
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.

4 participants