-
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
Fix breadcrumbs WCAG problem in the covid page #3763
Fix breadcrumbs WCAG problem in the covid page #3763
Conversation
30ade10
to
2c46495
Compare
2c46495
to
fd9b5bf
Compare
color: govuk-colour("white"); | ||
|
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.
Doing it in a similar way to how it's done for /browse pages.
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, just left a couple of small comments.
@@ -15,27 +15,40 @@ $c19-landing-page-header-background: govuk-colour("blue"); | |||
|
|||
.global-bar-present { | |||
@include govuk-media-query($from: desktop) { | |||
.covid__page-header { | |||
.covid__breadcrumb-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.
Do we think that these three renamed classes are still being used? Their parent is something called .global-bar-present
but there doesn't seem to be
.global-bar-present
on the page or in the app.
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 didn't change anything just to stay safe, but I also don't think .global-bar-present
is still used, so I'm happy to remove this part. It's also used in browse pages, so I can also remove it from there. Please, let me know, if that's ok?
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 dug into this and realised this for the global banner coming from static: https://docs.publishing.service.gov.uk/manual/global-banner.html#activate-the-global-banner It might be good to double check that these styles work if the global banner is there although I think it's been a bit waffy to test with the global banner in the past since it's in static 🤔
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'll test it with a global banner enabled in static.
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've just had a look at integration/coronavirus and it looks good to me with the global banner enabled 👍 I'm going to approve the PR but let me know if there was something else about the global banner I should be checking.
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 all I think.
fd9b5bf
to
97385e7
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.
Thanks for addressing my comments, LGTM 👍
Follow these steps if you are doing a Rails upgrade.
What
collections places breadcrumbs inside the
container in app/views/coronavirus_landing_page/components/landing_page/_page_header.html.erb, creating a WCAG fail on the covid landing page.Why
Having the breadcrumbs inside the main container is a WCAG fail.
Trello ticket