-
Notifications
You must be signed in to change notification settings - Fork 17
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
Conversation
180e8be
to
397852b
Compare
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)
397852b
to
58ed927
Compare
15c1709
to
3b47a99
Compare
@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 |
There was a problem hiding this comment.
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.
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?
|
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"> |
There was a problem hiding this comment.
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.
360605a
to
22d123c
Compare
ee1bc0f
to
b44a1ab
Compare
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
b44a1ab
to
7d57db2
Compare
There was a problem hiding this 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? |
There was a problem hiding this comment.
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?
…duces too much extra spacing
7d57db2
to
10efacc
Compare
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. |
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 thetitle
property in theheading_and_context
hash has been replaced withtext
(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.