-
Notifications
You must be signed in to change notification settings - Fork 20
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
Update to govuk-frontend 3.0 #1010
Conversation
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.
Since we seem to be using govuk-focusable
so much would it be worth making our own mixin for the code that you've written to replace it?
Also I've not talked to anyone about this yet but do you know the reason for removing this mixin? It was kind of useful...
app/assets/javascripts/govuk_publishing_components/lib/header-navigation.js
Show resolved
Hide resolved
app/assets/stylesheets/govuk_publishing_components/components/_attachment.scss
Outdated
Show resolved
Hide resolved
app/assets/stylesheets/govuk_publishing_components/components/_highlight-boxes.scss
Outdated
Show resolved
Hide resolved
app/assets/stylesheets/govuk_publishing_components/components/_input.scss
Show resolved
Hide resolved
app/assets/stylesheets/govuk_publishing_components/components/_feedback.scss
Outdated
Show resolved
Hide resolved
ae06f54
to
69759b5
Compare
69759b5
to
f9afdcd
Compare
f9afdcd
to
aa492f8
Compare
2d4c56d
to
92d2725
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.
Initial review, I've only up to the commit Replace govuk-focusable mixins
.
app/assets/stylesheets/govuk_publishing_components/components/_button.scss
Outdated
Show resolved
Hide resolved
app/assets/stylesheets/govuk_publishing_components/components/_checkboxes.scss
Outdated
Show resolved
Hide resolved
app/assets/stylesheets/govuk_publishing_components/components/_feedback.scss
Outdated
Show resolved
Hide resolved
app/assets/stylesheets/govuk_publishing_components/components/_feedback.scss
Outdated
Show resolved
Hide resolved
app/assets/stylesheets/govuk_publishing_components/components/_modal-dialogue.scss
Show resolved
Hide resolved
app/assets/stylesheets/govuk_publishing_components/components/_search.scss
Show resolved
Hide resolved
app/assets/stylesheets/govuk_publishing_components/components/_step-by-step-nav.scss
Show resolved
Hide resolved
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.
Another partial review, I've got up to (but not including) Update start button icon
.
I've noticed that a few of my comments are already out of date, so it's possible you've already addressed them in later commits. I'll review my review and mark/update as appropriate.
app/views/govuk_publishing_components/components/feedback/_survey_signup_form.html.erb
Show resolved
Hide resolved
app/views/govuk_publishing_components/components/_layout_header.html.erb
Show resolved
Hide resolved
app/assets/stylesheets/govuk_publishing_components/components/helpers/_variables.scss
Outdated
Show resolved
Hide resolved
app/assets/stylesheets/govuk_publishing_components/components/_table.scss
Outdated
Show resolved
Hide resolved
To be addressed after this PR is merged, logged here: #1028 and here: #1032 Some more thoughts...
Here's an only semi-related but interesting example from the contents list component, where the second line is indented, leaving a trailing bit of the first line's underline visible (I thought it was a quote mark when I first spotted it) |
Also fairly obvious but worth pointing out that apps will break with this update - I just tried it in finder-frontend and it failed because apparently we're using the UPDATE: actually might just be finder-frontend that has the problem. Shouldn't be too hard to fix then 😁 |
92d2725
to
ab95dcd
Compare
To be addressed after this PR is merged, logged here: #1028 The new focus style takes up a bit more vertical space than it used to. The worst example of this so far that I've found is the image card. I don't know if this is actually a problem though. You're most likely to notice this if you're using a keyboard, in which case moving the focus won't be a problem. Also the one you're currently focussed on is fine. Might be worth looking at some of these issues after this PR is merged, just noting it here for reference. Other components where this is a problem:
|
77cf625
to
6dda899
Compare
Unrelated to this PR, logged separately here: #1029 This is unrelated but worth adding to the 'to check later' list... the layout header with links doesn't seem to collapse early enough, plus when the links do collapse the dropdown menu doesn't seem to work? |
Unrelated to this PR, logged separately here: #1030 Page title (with context link) lacks a focus state for the link. This was a problem prior to this PR, so arguably out of scope, but we should fix it. It's even worse for the inverse option... |
app/assets/stylesheets/govuk_publishing_components/components/govspeak/_charts.scss
Outdated
Show resolved
Hide resolved
Do we need to update the govspeak 'highlight answer' to use the green instead of the turquoise? |
6dda899
to
817f8af
Compare
@vanitabarrett Thanks for the review. Tried to address the comments. Do let me know if the review is still in progress.
Updated to use the new colour @andysellick Thanks for the review. Tried to address all the comments (aside from the ones with an associated issue - 🙌 for that). For those comments without a thread I tried to reply below.
Updated to preserve the focus state when hovering
This is how the component is coming from
Fixed the focus/hover states on image card component |
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.
🎉 ❤️ 🚀 🎉 ❤️ 🚀 🎉 ❤️ 🚀 🎉 ❤️ 🚀 🎉 ❤️ 🚀 🎉 ❤️ 🚀 🎉 ❤️ 🚀 🎉 ❤️ 🚀 🎉 ❤️ 🚀 🎉 ❤️ 🚀 🎉 ❤️ 🚀 🎉 ❤️ 🚀 🎉 ❤️ 🚀 🎉 ❤️ 🚀 🎉 ❤️ 🚀 🎉 ❤️ 🚀 🎉 ❤️ 🚀 🎉 ❤️ 🚀 🎉 ❤️ 🚀 🎉 ❤️ 🚀
…-next-navigation and title
Use govuk-frontend variable instead of colour mixin.
Updating the colours and font require dropping compatibility with legacy projects
Align the styles of example and information callout with the inset text component
817f8af
to
b8bb01b
Compare
Rebased again before merging. Thanks everyone who commented on this, that helped a lot! |
What
Update
govuk_publishing_components
to usegovuk-frontend
3.0.Fix focus states on a few components to align them with the colour contrast requirements
Why
Notes
raised for visibility and be used as a base for discussionsready for reviewgovuk-frontend
3.0 changelogCompatibility checks
These changes were tested with applications in the following states using the appropriate compatibility flags.
govuk_elements_rails
,govuk_frontend_toolkit
and an indirect dependency ongovuk_template
(e.g. Service manual frontend)govuk_frontend_toolkit
and an indirect dependency ongovuk_template
(e.g. Frontend)govuk_template
- dependent on static (e.g. Calendars)Visual Changes
There are visual changes for almost all components, especially related to focus states (which are harder to get with a basic visual comparison). I would recommend using the preview app when reviewing as images don't capture all the states.
View Changes
https://govuk-publishing-compo-pr-1010.herokuapp.com/component-guide
Trello card