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 component with heading component #3557

Closed

Conversation

JamesCGDS
Copy link
Contributor

@JamesCGDS JamesCGDS commented Feb 13, 2025

⚠️ 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.

One potential concern to note is that all the tests seem to pass despite there being instances of @content_item.heading_and_context[:title] in the code e.g. in call_for_evidence.html.erb, consultation.html.erb, detailed_guide.html.erb, and document_collection.html.erb. This is potentially concerning as the title property in the heading_and_context hash has been replaced with text (corresponding commit here), so I'm not too certain why this passes the tests. I may be missing something though and someone else might know more).

Why

Trello card

Visual changes

None - I tested every page listed in the README and a healthy amount of variations listed in the government-frontend root. I didn't notice any visual differences.

@govuk-ci govuk-ci temporarily deployed to government-frontend-pr-3557 February 13, 2025 02:22 Inactive
@JamesCGDS JamesCGDS force-pushed the replace_title_component_with_heading_component branch from 180e8be to 397852b Compare February 13, 2025 02:23
@govuk-ci govuk-ci temporarily deployed to government-frontend-pr-3557 February 13, 2025 02:23 Inactive
This will be used to introduce spacing above the component to maintain visual parity with the title component (which brings in its own top margin)
@JamesCGDS JamesCGDS force-pushed the replace_title_component_with_heading_component branch from 397852b to 58ed927 Compare February 13, 2025 02:29
@govuk-ci govuk-ci temporarily deployed to government-frontend-pr-3557 February 13, 2025 02:30 Inactive
@govuk-ci govuk-ci temporarily deployed to government-frontend-pr-3557 February 13, 2025 11:37 Inactive
@JamesCGDS JamesCGDS force-pushed the replace_title_component_with_heading_component branch from 15c1709 to 3b47a99 Compare February 13, 2025 12:09
@govuk-ci govuk-ci temporarily deployed to government-frontend-pr-3557 February 13, 2025 12:09 Inactive
@JamesCGDS
Copy link
Contributor Author

@KludgeKML I've added you to this PR as I've made a few changes to some presenters - hope that's alright.

@@ -6,7 +6,7 @@ class FieldOfOperationPresenter < ContentItemPresenter
def title_and_context
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the change to this file might want to be fixed up with the earlier commit where all the other presenter changes were.

@KludgeKML
Copy link
Contributor

I think you should definitely fixup those calls where [:title] rather than [:text] is still used, although it might be a better idea to just use the content_item title, if one is available?

The use points to here - the title is just passed to a GA4 component, so probably the tests don't fail because that component is happy with a nil title?

@KludgeKML
Copy link
Contributor

Ah, I see that sometimes the presenters override title. You should probably fix it up to use the [:text] field, then (you can't use the content item directly without a bit of refactoring, and we probably want to put that off until these routes are moved to frontend)

<div class="govuk-grid-column-two-thirds">
<%= render 'govuk_publishing_components/components/title',
title: @content_item.title %>
<div class="govuk-grid-column-two-thirds responsive-top-margin">
Copy link
Contributor

Choose a reason for hiding this comment

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

A lot of the problems with spacing introduced by switching to the heading component can be fixed by adding the govuk-main-wrapper class to the main element in the application layout. It's something we've not added for some reason but is supposed to be part of doing Design System layout. Apologies, I should have made you aware of this earlier.

You'll probably need to do something extra to override this class in some instances (for example on HTML publication pages, where the extra spacing isn't needed) but should save having to manually add spacing to each individual page type.

@govuk-ci govuk-ci temporarily deployed to government-frontend-pr-3557 February 14, 2025 13:41 Inactive
@JamesCGDS JamesCGDS force-pushed the replace_title_component_with_heading_component branch from 360605a to 22d123c Compare February 14, 2025 13:56
@govuk-ci govuk-ci temporarily deployed to government-frontend-pr-3557 February 14, 2025 13:57 Inactive
@govuk-ci govuk-ci temporarily deployed to government-frontend-pr-3557 February 18, 2025 01:51 Inactive
@govuk-ci govuk-ci temporarily deployed to government-frontend-pr-3557 February 18, 2025 13:30 Inactive
@JamesCGDS JamesCGDS force-pushed the replace_title_component_with_heading_component branch from ee1bc0f to b44a1ab Compare February 18, 2025 14:15
@govuk-ci govuk-ci temporarily deployed to government-frontend-pr-3557 February 18, 2025 14:15 Inactive
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 and fix tests
Use 'govuk-main-wrapper' class on the main element to introduce spacing
Due to using the govuk-main-wrapper class for spacing, this margin top override is no longer necessary
@JamesCGDS JamesCGDS force-pushed the replace_title_component_with_heading_component branch from b44a1ab to 7d57db2 Compare February 18, 2025 14:32
@govuk-ci govuk-ci temporarily deployed to government-frontend-pr-3557 February 18, 2025 14:32 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.

Looks good to me, just had some questions:

  • There's a small visual difference between the top spacing changes on some of the pages now, is this expected?
  • responsive-top-margin is added to the HTML and then removed in future commits - can this be squashed so it's never added to places it gets removed from? (I understand it being separated now makes the PR easier to follow, though I guess if it's never eventually used it should ideally be removed from the commit history)

@@ -118,6 +118,10 @@ def show_default_breadcrumbs?
true
end

def exclude_main_wrapper?
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it worth making this exclude_main_wrapper_class so it's more specific?

@JamesCGDS JamesCGDS force-pushed the replace_title_component_with_heading_component branch from 7d57db2 to 10efacc Compare February 18, 2025 15:39
@govuk-ci govuk-ci temporarily deployed to government-frontend-pr-3557 February 18, 2025 15:39 Inactive
@JamesCGDS
Copy link
Contributor Author

Thanks for your comments @andysellick @AshGDS @KludgeKML - I've made those changes but I got in a bit of a messy situation with squashing commits and I kept bumping into merge conflicts so I've made a new PR as it seemed simpler to start from scratch.

@JamesCGDS JamesCGDS closed this Feb 19, 2025
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.

5 participants