-
Notifications
You must be signed in to change notification settings - Fork 323
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 footer component #569
Add footer component #569
Conversation
Nice! This is looking really good so far 💯 👍 |
@dashouse this component uses non-standard greys, is this an exception? Should we iterate this so it doesnt have it's own custom colours later? |
Mainly the colour of text? The background is an existing colour? Agreed, probably something to iterate later as it's the same as the Design System and GOV.UK |
af38a5a
to
4f481f7
Compare
0622ea3
to
a4c9a6f
Compare
src/footer/_footer.scss
Outdated
|
||
.govuk-c-footer__copyright-logo { | ||
display: block; | ||
min-width: 125px; // Based on the govuk-crest-2x.png image width: 250px/2 |
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.
Given that these numbers appear in a number of places with comments, it might be a good idea to break them out into variables:
.govuk-c-footer__copyright-logo {
// Dimensions of the crest image at retina resolution
$crest-image-width: 125px;
$crest-image-height: 102px;
display: block;
min-width: $crest-image-width;
// ...
}
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.
Good shout
a4c9a6f
to
3f3e522
Compare
{% endfor %} | ||
</ul> | ||
{% endif %} | ||
<svg |
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.
Worth trying to see if this can be optimised – a quick play with http://petercollingridge.appspot.com/svg-optimiser suggests we can shave a few hundred bytes off this.
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.
Yeah I've already optimised it, those bytes could be from putting it on a single line which I figured is whitespace that'll get GZIPed but I'll double check.
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.
👍
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.
Confirmed the extra savings come from the whitespace
3f3e522
to
f9d0e9f
Compare
f9d0e9f
to
a8c21f2
Compare
a8c21f2
to
6052f4f
Compare
6052f4f
to
88322d9
Compare
88322d9
to
0ee1e67
Compare
0ee1e67
to
6750869
Compare
6750869
to
b1dc784
Compare
b1dc784
to
9d63f9c
Compare
This seems to have different breakpoints to whats currently on GOV.UK, is that ok? GOV.UK goes to a single column quite quickly as you make the browser narrower. |
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 to me @NickColley! 💯
Happy to approve it anytime, but I'll wait for the initial reviewers to take a look too.
I have only one question: shall we try and put in more accessibility on the SVG (I think it's the first inline SVG in the library)? I was thinking of having a <title>
and maybe even a <description>
for more complex SVGs; and maybe bind them to the <svg>
with aria-labelledby
to extend the number of ATs that are using them. It doesn't seem to bring much value for our OGL, which is already explained in the next element, but more of a thought about inline SVGs in general.
@joelanman that's intentional, flexbox allows for more granular reflow. |
@alex-ju the SVG is presentational so any additional properties would not be exposed to users of assistive technologies. Definitely agree if we use SVGs inline that are not presentational we should take extra care to do as you've described. |
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.
This is looking great, thanks @NickColley.
Something we noticed earlier today was the poor contrast between the link colour and the hover state link colour. I didn't actually realise the colour changed the first time I tried it! Might be worth @dashouse having a look.
None of my comments should be considered blocking though (the hover colour issue mainly because the same issue exists on the current GOV.UK footer).
@include mq ($until: tablet) { | ||
padding-bottom: $govuk-spacing-scale-2; | ||
} | ||
border-bottom: 1px solid $govuk-footer-border; |
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.
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 figured that was a mistake so wanted to make the new version more consistent.
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.
That's cool. It does look better now compared to the current slightly bunched up mobile view 👍
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.
Thank @dashouse's patented spacing scale 🌈
src/footer/footer.yaml
Outdated
Note: This decision has been made given that most uses of this role tend to be used directly on a `<footer>` element. | ||
Assumption made is that is most predictable for users of assistive technologies. | ||
The spec indicates that `contentinfo` is useful for "Examples of information included in this region of the page are copyrights and links to privacy statements.", which may indicate that it might be better placed around the 'meta' section of this component. | ||
Should be challenged if user research indicates this is an issue. |
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 sure there's a good explanation for this but I'm not clear why the accessibility criteria are in the YAML file?
👍 for adding them.
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 imagined eventually we'd want to expose these somewhere, either in the README, Design System or Review application.
For now it's in the yaml.
@joelanman I'll see if I can make it 1:1 at tablet. |
- uses columns CSS3 feature to avoid multiple lists
Used when you want to make the footer full width
Adds a break between navigation and the rest of the footer
39b7264
to
c92588f
Compare
@joelanman updated based on your feedback, looks more balanced and less chaotic now 👍 |
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.
Need to add a changelog entry before merging.
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 from a design point of view - needs a code review
Spoke to @amyhupe, I've made some updates based on an initial conversation. We're going to noodle on the content for the accessibility acceptance criteria, but can be updated later since this guidance is not exposed anywhere to users yet. |
Includes decisions made that can be challenged
https://www.w3.org/TR/wai-aria-1.1/#contentinfo) Note: This decision has been made given that most uses of this role tend to be used directly on a `<footer>` element. Assumption made is that is most predictable for users of assistive technologies. The spec indicates that `contentinfo` is useful for "Examples of information included in this region of the page are copyrights and links to privacy statements.", which may indicate that it might be better placed around the 'meta' section of this component. Should be challenged if user research indicates this is an issue.
c92588f
to
c56780c
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.
Code reviewed.
👉link: https://govuk-frontend-review.herokuapp.com/components/footer
New version of the GOV.UK Footer based off the work done in the GOV.UK Design System.
Notable changes
Testing
Inverse colours ✅
Cross browser testing ✅
Known issues:
IE8
Fixed (html5 shiv):
IE9
IE11 (IE10 looks similar)
Edge
Windows, Chrome
Windows, Firefox
Mac, Safari
Mac, Chrome
Mac, Firefox
iPhone 6s
After fix:
iPhone X
Android, Chrome
Android, Firefox
Windows phone
After fix:
Assistive technology testing ✅(see notes below)
Accessibility questions
Accessibility notes
When tabbing through on IE11, 'All content is avaliable text' span is tabbable, this happens with and without Jaws 18. Does not read out as a link, but just text so does not feel like a barrier but might be worth investigating.
As part of https://trello.com/c/hSF9zpJP/241-3-fe-footer-component