Remove title component margin_top option #4508
Merged
+7
−23
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
andysellick
force-pushed
the
title-margin
branch
from
8ec358a to
e2b7968
Compare
andysellick
added a commit
to alphagov/collections
that referenced
this pull request
Dec 19, 2024
- moving towards a world where the title component has no margin_top option: alphagov/govuk_publishing_components#4508 - this removes instances of the use of the margin_top option for the title component - in some cases it was possible to replace the title component with the heading component for no visual change (where the margin_top: 0 option was applied) - in other cases there is a visual change, but this will be minimal
andysellick
force-pushed
the
title-margin
branch
from
e2b7968 to
62b059e
Compare
andysellick
force-pushed
the
title-margin
branch
2 times, most recently
from
f25d501 to
033b759
Compare
- our spacing model is to apply margin bottom, so this is an anomaly - replace margin_top with default padding on the component, so no visual changes - margin_top option isn't widely used and can be replaced with alternatives, separate PRs will be raised
andysellick
force-pushed
the
title-margin
branch
from
033b759 to
a6ea4c2
Compare
andysellick
changed the title
AshGDS
approved these changes
Jan 28, 2025
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What
Applications that use this component, and their use of the
margin_topoption are:collections(changes covered in Update usage of the title component collections#3918) (done)datagovuk_find(no use ofmargin_topso no change required)email-alert-frontend(no use ofmargin_topso no change required)feedback(changes covered in Use heading component instead of title feedback#2042) (done)finder-frontend(no use ofmargin_topso no change required)frontend(changes covered in Page title component changes frontend#4550) (done)government-frontendneed to deploy this so we can switch from the title to heading component: Add context option to heading component #4510) (done) and now need to switch titles with zero margin top to headings (Remove margin tops and replace titles with headings government-frontend#3539)manuals-publisher(changes covered in Replace title component with heading manuals-publisher#2595) (done)release(no use ofmargin_topso no change required)search-admin(no use ofmargin_topso no change required)smart-answers(no use ofmargin_topso no change required)whitehall(changes covered in Replace title component with heading whitehall#9835) (done)govuk_publishing_components(changes covered inhttps://github.com/Replace title component with heading #4591) (done)Note that this PR will have to wait for the ones mentioned above and further work to check it is okay before merging
Why
This is part of a wider piece of work to move the margin options from the shared helper into the component wrapper helper and standardise our component spacing model. This is a step towards being able to remove the
margin_topoption from the shared helper.This is a bit of a stop gap because we intend to replace all instances of the title component with the heading component eventually, but that's a larger piece of work.
Visual Changes
Percy is showing a visual diff on the inverse header, which is expected. But this change has been accounted for by a corresponding change in the inverse header to allow custom padding top and bottom. That PR is linked to several PRs in applications where those changes are accounted for to produce no visual difference. Once this code is and that code is in a new gem release we'll merge those other application PRs and everything (should) be fine.
Trello card: https://trello.com/c/Y0pDWbHw/390-move-some-shared-helper-options-into-component-wrapper