-
Notifications
You must be signed in to change notification settings - Fork 327
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
Fix footer alignment with grid classes and add width
macro option
#2462
Conversation
39832b3
to
06f398d
Compare
06f398d
to
4106b54
Compare
span
macro option
span
macro optionspan
macro option
9983d5f
to
951e023
Compare
951e023
to
727b949
Compare
727b949
to
93d0f37
Compare
span
macro optionwidth
macro option
93d0f37
to
2f9668f
Compare
2f9668f
to
8d8f12d
Compare
8d8f12d
to
92f67c0
Compare
92f67c0
to
43bc830
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.
Content looks nice and clear! Just added a few comments.
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.
Generally think the approach is sound and the changes are looking good – a few minor comments.
Something of an edge case, but long words can overflow across other columns – but this is also true of the grid elsewhere. Not sure if it's worth worrying about as I couldn't think of any realistic examples, other than a long email address which seems an like unlikely thing to put in a section.
it('renders two-column section full width by default', () => { | ||
const $ = render('footer', examples['with default width navigation (two columns)']) | ||
|
||
const $component = $('.govuk-footer') | ||
const $section = $component.find('.govuk-footer__section') | ||
expect($section.hasClass('govuk-grid-column-full')).toBeTruthy() | ||
}) |
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'm not 100% sure what this is testing. I think the only difference between this and the previous test is that this one has columns
set?
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.
Yep, you're right, that is the only difference. I think I added it just to double check that columns being set doesn't have any impact on the width class being set because originally they were linked. But maybe that's not really worth testing now?
const $component = $('.govuk-footer') | ||
const $section = $component.find('.govuk-footer__section') |
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 know this is consistent with some of the other tests, but not sure we're getting much from doing a query to find the 'component' and then a second query to find the section within it.
What do you think about just looking for the section in the first place?
const $component = $('.govuk-footer') | |
const $section = $component.find('.govuk-footer__section') | |
const $section = $('.govuk-footer__section') |
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.
Thanks, good point! I've added a new commit to remove these in the footer tests.
Allow users to set the width of a footer section explicitly. For example: set `width: 'two-thirds'` for a two-third section.
Each list in the footer section as a bottom margin of 30px, and a top margin (which is actually set by a bottom margin on the heading) of 40px on desktop and 30px on mobile. This means that on desktop the spacing above and below the list is unequal. Having spoken to Chris, we've decided to make this spacing and drop the heading margin down to 30px on all breakpoints as it felt too large.
Update the footer YAML examples to use the new `width` macro option Update the full GOV.UK yaml example to reflect the current layout and use the new `width` macro option. This commit also renames the example. When the footer example has 'GOV.UK' in the name, it gets flagged by Chrome as a possible spam/phishing site which means you can't open that example in a new window. This commit works around that issue by renaming the example - it seems to be the word "GOV.UK" which triggers it
This commit updates the announcement example to fix the footer layout to a two-thirds/one-third layout as it was before, using the new `width` macro option. This commit updates the campaign page example. It builds a footer based on the example screenshot from #1726
3e5f48c
to
ed08815
Compare
ed08815
to
8a439c9
Compare
Some of the examples in the footer.yaml file that we use for tests were not marked as `hidden`. This commit adds those missing `hidden` attributes so they're not shown in the review app
The footer tests included a query to look up the `.govuk-footer` component, and then a second query to look up the specific element within that component to test. We can remove the first query and go straight to the second for most tests.
8a439c9
to
e745aa6
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 great! 👍🏻
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.
Ship it! 🚢
* This is in response to bumping our version of `govuk-frontend`: https://github.com/alphagov/govuk-frontend/blob/main/CHANGELOG.md#check-your-footer-displays-as-expected * The original PR which forces the change: alphagov/govuk-frontend#2462
* This is in response to bumping our version of `govuk-frontend`: https://github.com/alphagov/govuk-frontend/blob/main/CHANGELOG.md#check-your-footer-displays-as-expected * The original PR which forces the change: alphagov/govuk-frontend#2462
* This is in response to bumping our version of `govuk-frontend`: https://github.com/alphagov/govuk-frontend/blob/main/CHANGELOG.md#check-your-footer-displays-as-expected * The original PR which forces the change: alphagov/govuk-frontend#2462
Closes #1539 and #1726
Why
This PR attempts to rework what was originally spiked in #2446 and #2428 to have the same outcome, but without using flexbox. When reviewing the previous PRs, we realised that due to the fallback provided for older browsers, we were 1) duplicating a lot of what our existing grid does, and 2) flexbox was only really being used for 'dynamic' half/half layouts.
One positive of using flexbox was that we could support more 'dynamic' layouts where the footer sections could shrink/grow accordingly. However, the downsides are that it makes the code difficult to follow and it was potentially also a bit of "magic" for users.
What
This PR makes the following changes:
width
macro option to explicitly set the width of the section if needed, using our grid widthsLinks for testing
https://govuk-frontend-pr-2462.herokuapp.com/components/footer
https://govuk-frontend-pr-2462.herokuapp.com/examples/footer-alignment
https://govuk-frontend-pr-2462.herokuapp.com/full-page-examples/campaign-page
Screenshots
IE8