-
Notifications
You must be signed in to change notification settings - Fork 328
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
Add button groups for use in cookie banner #2114
Conversation
8308c54
to
fcc84d2
Compare
This comment has been minimized.
This comment has been minimized.
fcc84d2
to
c98bd68
Compare
Looks good! The things that stood out to me are:
Those are pretty minor issues tbh and I don't think they'd impact usability much. Are they easily fixable or not? |
Alignment of links on iOS 7 (Simulated iPhone 5S)In theory this could be an easy fix, but I don't know if we'd want to do it… Flexbox is only supported on iOS 7 through 8.4 with a If we changed the browserslist to include For reference, according to https://iosref.com/ios the last iPhone that did not support iOS 9 or above was the iPhone 4 which Apple stopped selling in September 2013. Stats from visits from iOS to GOV.UK in December:
If this is something we want to look into further I think we might want to create a separate issue to track this as it affects more than this component. Diff of changes to package by changing
|
With different spacing for links and buttons | With consistent spacing for links and buttons |
---|---|
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.
@36degrees This is looking great 🎉 Just left a couple of small comments.
I'll take a look at your comment about iOS7/IE8-9 support.
.govuk-link { | ||
@include govuk-font($size: 19, $line-height: 19px); | ||
display: inline-block; | ||
max-width: 100%; |
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.
What does setting the max-width do here?
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.
IE10 and IE11 have a bug where column flex items set to align-items: center
overflow their container – see c98bd68. Setting max-width: 100%
stops this from happening.
I split it out as a separate commit so that it would show in a git blame, but could add a comment if we think it warrants it.
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.
Ahh my bad, I missed that. A separate comment might be good so that down the line we could consider removing the rule when we drop support for certain browser combinations.
@@ -0,0 +1,111 @@ | |||
{% extends "layout.njk" %} |
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.
We could also update could /full-page-examples/confirm-delete
to use the new button group.
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.
Ah great call, thanks 👍
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.
Done in f85eb47
// Reduce the bottom margin to the size of the vertical gap (accommodating | ||
// the button shadow) – the 'lost' margin is moved to the button-group. | ||
.govuk-button { | ||
margin-bottom: $vertical-gap + $button-shadow-size; |
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.
It might be good to make the bottom margin of links and buttons inside the button group to be consistent with each other, at the moment there's a small difference between them.
We could also consider making the combined bottom margin of the button group to be consistent with govuk-form-group
- but that's just a suggestion.
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 this is probably one for @CharlotteDowns to check, but I think the difference is intentional, based on how we want them to appear on mobile and when they wrap onto multiple lines. On tablet and above, everything should be aligned along the baseline so in theory the vertical margins of links are irrelevant unless they appear on a line by themselves.
The intent is that the combined bottom margin of the group matches the bottom margin on a button – so if there's only a single row of buttons then it should have the exact same spacing as if there was a single button not in a group.
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.
The main reason I mentioned it was that on mobile the button group ends up having a slightly different bottom margin depending on whether the last item is a link or a button.
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.
Hey both, we want the link to look equally spaced between the 'visual' bottom of the 'reject' button and the bottom of the banner on mobile view. To follow the 5px baseline grid, I had originally tried to aim for 20px above and 20px below (as I think that was the padding on the banner container, this did mean some odd number margin values to account for the 3px box-shadow). I guess the aim is that the vertical spacing can all be divisible by 5px on all size viewports and that the text is on the same vertical line on sizes greater than mobile.
@36degrees Re: alignment of links on iOS 7, I would be tempted to leave it as it is since it's outside our support matrix and the user experience is still functional. If there was a blocker to user completing an action then we should consider fixing it. Interesting investigation into the potential fix though 👍 Similarly with IE8/9, the user experience is still functional. |
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 for addressing my comments @36degrees!
Add a new button groups object which can be used to group buttons and links together. Within a button group: - links are styled to line up visually with the buttons, including being centre-aligned on mobile - spacing between the buttons and links is handled automatically, including when they wrap across multiple lines The button group object will then be used to group the 'Accept cookies', 'Reject cookies' and 'View and set cookie preferences' buttons and links in the cookie banner component. The button group object uses flexbox and align-items to align the buttons and links along the 'cross axis' – centre-aligning on mobile and aligning rows along the baseline on tablet and above. align-items is in theory supported in IE11 and above, but our use of autoprefixer means that IE10-compatible flexbox properties are also included, and in testing it works as well in IE10 as it does in IE11. In browsers that do not support flexbox [1] or align-items [2] the spacing between buttons and links is different (as whitespace between inline blocks is not handled) and they are not aligned along the cross-axis, but the general layout is still applied. [1]: https://caniuse.com/flexbox [2]: https://caniuse.com/mdn-css_properties_align-items_flex_context
Set max-width to fix links overflowing in IE10/11 in the mobile viewport. https://github.com/philipwalton/flexbugs#2-column-flex-items-set-to-align-itemscenter-overflow-their-container
f85eb47
to
b650d2d
Compare
Just to reply to Ollie's comment – agree with what you proposed:
|
Add a new button groups object which can be used to group buttons and links together.
Within a button group:
The button group object will then be used to group the 'Accept cookies', 'Reject cookies' and 'View cookies' buttons and links in the cookie banner component.
The button group object uses flexbox and align-items to align the buttons and links along the 'cross axis' – centre-aligning on mobile and aligning rows along the baseline on tablet and above.
align-items
is in theory supported in IE11 and above, but our use of autoprefixer means that IE10-compatible flexbox properties are also included, and in testing it works as well in IE10 as it does in IE11.In browsers that do not support flexbox or align-items the spacing between buttons and links is different (as whitespace between inline blocks is not handled) and they are not aligned along the cross-axis, but the general layout is still applied (see screenshots for IE9, IE8 and iOS 7 for examples).
There's a corresponding PR to add guidance to the Design System here: alphagov/govuk-design-system#1472
Closes #2107
Screenshots
Firefox 84 (macOS)
Safari 14 (macOS)
Chrome 88 (macOS)
Android 11
Edge 87
IE11
IE10
IE10 ('mobile')
IE9
IE8
iOS 7 (Simulated iPhone 5S)
Android 4.4 (Simulated Galaxy S4)
Example of use in the cookie banner
Screen.Recording.2021-01-26.at.09.36.50.mov