Skip to content
This repository has been archived by the owner on Mar 6, 2023. It is now read-only.

Revert and fix upgrade to govuk frontend v3 #542

Merged
merged 7 commits into from
Sep 23, 2019

Conversation

@injms injms marked this pull request as ready for review September 18, 2019 13:51
@bevanloon bevanloon temporarily deployed to govuk-service-manual-fr-pr-542 September 18, 2019 13:58 Inactive
@bevanloon bevanloon temporarily deployed to govuk-service-manual-fr-pr-542 September 18, 2019 14:16 Inactive
@36degrees
Copy link
Contributor

Really minor thing, and it could be fixed in a future pull request, but the show/hide all updates links do not use the new focus style.

Screen Shot 2019-09-18 at 15 36 05

Otherwise, this is looking good to me.

@36degrees
Copy link
Contributor

In the same category of nice-to-fix-but-not-essential, the 'is there anything wrong with this page' form seems to have a double gutter on mobile:

Screen Shot 2019-09-18 at 15 37 51

Copy link
Contributor

@36degrees 36degrees left a comment

Choose a reason for hiding this comment

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

I'm happy that the code was previously reviewed as part of the original PR (before it was reverted), so I've not reviewed the code – just looked at the example pages linked from this PR.

@bevanloon bevanloon temporarily deployed to govuk-service-manual-fr-pr-542 September 18, 2019 15:45 Inactive
@injms
Copy link
Contributor Author

injms commented Sep 18, 2019

I'm happy that the code was previously reviewed as part of the original PR (before it was reverted), so I've not reviewed the code – just looked at the example pages linked from this PR.

There are commits since the revert that weren't part of the code review - these fix the problem that caused the reversion:

@injms injms requested a review from 36degrees September 18, 2019 15:48
@vanitabarrett
Copy link
Contributor

The spacing between paragraphs and headings seems a bit small compared to how it used to be? It also doesn't quite match what the govspeak component should be doing (p tags should have 20px top and bottom margin, these seem to have 15px), so i'm wondering if something is interfering?
Screen Shot 2019-09-19 at 10 17 41

I can also see some CSS errors but I can't work out where they're coming from.
Screen Shot 2019-09-19 at 10 18 53

This is on https://govuk-service-manual-fr-pr-542.herokuapp.com/service-manual/service-standard/point-1-understand-user-needs

@36degrees
Copy link
Contributor

There are commits since the revert that weren't part of the code review

Aside from @vanitabarrett's comments, those commits make sense to me 👍

@chao-xian
Copy link
Contributor

Thanks @vanitabarrett @36degrees

@bevanloon bevanloon temporarily deployed to govuk-service-manual-fr-pr-542 September 20, 2019 09:59 Inactive
@injms
Copy link
Contributor Author

injms commented Sep 20, 2019

Good catch on the CSS errors @vanitabarrett!

The CSS errors caused by em(4) come from a GOV.UK Elements, which is being included via Frontend Toolkit. There is a helper function in GOV.UK Elements that should output valid CSS, but it doesn't appear to be working. Checking against the live site, the errors made it into production 😳.

Since Elements and Frontend Toolkit are deprecated, are due to be removed soon 🤞, and this error will be ignored by browsers, I think that this can be safely ignored.

@injms
Copy link
Contributor Author

injms commented Sep 20, 2019

The paragraph tags are set to have a lower spacing of 15px because ¯\_(ツ)_/¯ that's what the CSS in this app originally had. I missed this thanks to the loveliness of the way margins collapse.

I originally added an increase in line height so the focus state didn't overlap on the next line. Since it's just-about-okay, I'll add that as an issue for discussion on the component itself.

Anyways, this is a longwinded way of me saying I've removed the app specific Govspeak overrides, so the spacing should be more consistent.

@injms injms merged commit 84c165f into master Sep 23, 2019
@injms injms deleted the revert-and-fix-upgrade-to-govuk-frontend-v3 branch September 23, 2019 12:54
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants