Skip to content
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

Upgrade to publishing components v19 #1448

Merged
merged 5 commits into from
Sep 5, 2019

Conversation

injms
Copy link
Contributor

@injms injms commented Aug 19, 2019

What

The upgrade to version 19 of the component gem will bring in the changes made to version 3 of GOV.UK Design System.

The legacy colours flag is turned on, and the legacy font size is set to be used. (There will be a separate pull request to remove this legacy code and to turn on the new colour palette.)

Why

The improvements include a new focus state, which will help this app to reach the WCAG 2.1 standard. It also has a tonne of other improvements that cascade from Design System, such as the new smaller font.

Visual regression results

https://government-frontend-pr-1448.surge.sh/gallery.html

Component guide for this PR:

https://government-frontend-pr-1448.herokuapp.com/component-guide

@bevanloon bevanloon temporarily deployed to government-frontend-pr-1448 August 19, 2019 10:14 Inactive
@bevanloon bevanloon temporarily deployed to government-frontend-pr-1448 August 20, 2019 07:03 Inactive
@bevanloon bevanloon had a problem deploying to government-frontend-pr-1448 August 21, 2019 09:36 Failure
@maxgds maxgds force-pushed the upgrade-to-publishing-components-v18 branch from 2b6fba6 to 85a9a7d Compare August 21, 2019 11:30
@bevanloon bevanloon had a problem deploying to government-frontend-pr-1448 August 21, 2019 11:30 Failure
@maxgds maxgds force-pushed the upgrade-to-publishing-components-v18 branch from 85a9a7d to 66ab617 Compare August 21, 2019 11:47
@maxgds maxgds force-pushed the upgrade-to-publishing-components-v18 branch 2 times, most recently from 58989d5 to d4055d8 Compare September 3, 2019 09:41
@maxgds maxgds changed the title Upgrade to publishing components v18 Upgrade to publishing components v19 Sep 3, 2019
@maxgds maxgds force-pushed the upgrade-to-publishing-components-v18 branch 2 times, most recently from 51f272b to 1df45d9 Compare September 3, 2019 10:44
@maxgds maxgds marked this pull request as ready for review September 3, 2019 11:04
@maxgds maxgds requested a review from chao-xian September 3, 2019 11:05
Copy link
Contributor

@chao-xian chao-xian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've only got one minor thing to change that I spotted, but from my point of view I'd rather an FE dev checked the commit that updates styles.

@maxgds maxgds force-pushed the upgrade-to-publishing-components-v18 branch from 1df45d9 to 73fad00 Compare September 4, 2019 09:47
@maxgds maxgds requested a review from vanitabarrett September 4, 2019 09:49
@maxgds maxgds force-pushed the upgrade-to-publishing-components-v18 branch 2 times, most recently from 0aab74c to 5a895ba Compare September 4, 2019 10:30
Gemfile Show resolved Hide resolved
app/assets/stylesheets/application.scss Outdated Show resolved Hide resolved
test/wraith/config.yaml Show resolved Hide resolved
@@ -36,7 +36,6 @@
}

.app-c-back-to-top {
color: $link-colour;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd suggest putting the govuk-link class on the elements in place of the stuff that's been removed here.

@@ -1,16 +1,26 @@
.app-c-published-dates {
@include core-16;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, you could try govuk-link on this element. I'm going to stop writing comments on this because there's a lot of them, the main benefit is that with the additional link class we're not duplicating the focus styles every time the mixin is called.

app/assets/stylesheets/application.scss Show resolved Hide resolved
app/assets/stylesheets/application.scss Show resolved Hide resolved
Copy link
Contributor

@vanitabarrett vanitabarrett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most of my comments have already been noted by @andysellick 🙂

Gemfile.lock Outdated Show resolved Hide resolved
injms and others added 2 commits September 4, 2019 14:35
 - Link redirected to live site.
Changes made:
- Explicitly adding the focus state multiple times
- Adding a class to hang the focus state off where necessary
- Removing "@include core-16" as it isn't needed and it introduces line spacing that makes the focus state overlap
- Removing an explicitly set link colour that overrode the focus state
- Adding breathing space to the print link
@maxgds maxgds force-pushed the upgrade-to-publishing-components-v18 branch from 5a895ba to a3091eb Compare September 4, 2019 13:45
@maxgds maxgds requested a review from vanitabarrett September 5, 2019 07:23
@maxgds maxgds dismissed vanitabarrett’s stale review September 5, 2019 07:24

Will address in a separate PR

@maxgds maxgds merged commit 843e07c into master Sep 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants