Update error pages to use public layout template #2440

injms · 2021-03-19T11:53:30Z

What

The base error page template has been updated to use the public layout component
Application stylesheet has been updated to include only the styles needed for the public layout component, global bar, and emergency banner
Application JavaScript has been updated to only include the modules needed for the public layout component
Error page stylesheet has only the extra imports needed - in this case, only the the title component was needed
Minor tweak to make the apostrophe in the "We're" on some of the error pages look nicer
Fixes a minor bug where the heading styles weren't being included, so the h1 on the error page was using the default browser font size for h1s.

Why

Adding a template to static that uses the public layout component meant updating these error pages first, since they use the same default application.scss and application.js that the public layout component uses.

The default stylesheet and JavaScript file now contains only the things needed for the public layout component.

Visual differences

Note - the page footer will look different as it the public layout component uses the footer component. These changes are outside the scope of this pull request and are best raised as issues on the govuk_publishing_components repo

Page	Before	After
400
401
403
404
405
406
410
422
429
500
501
502
503
504

app/assets/stylesheets/error-page.scss

andysellick

I haven't done a full review, just writing down some of the thoughts we had when we talked about this.

andysellick · 2021-03-22T17:05:23Z

app/assets/javascripts/application.js

+//= require govuk_publishing_components/dependencies
+
+//= require govuk_publishing_components/lib
+//= require govuk_publishing_components/lib/track-click


Confusingly, lib is both a directory and a file, lib.js. I'd thought that this would include the whole directory, but that would need require_tree, so I think this is including lib.js. However, lib.js then includes everything in the lib directory, plus the polyfills.

Since lib the directory contains track-click, line 6 isn't needed?

andysellick · 2021-03-22T17:07:10Z

app/assets/javascripts/application.js

+
+//= require govuk_publishing_components/dependencies
+
+//= require govuk_publishing_components/lib


Is everything in lib needed for the error pages? Based on a quick glance, you might only need track-click and cookie-functions?

From a bit of trial and error, I think that the only required bits are:

cookie-functions

header-navigation

track-click

📉 👍

app/assets/javascripts/application.js

andysellick · 2021-03-22T17:13:23Z

app/assets/javascripts/application.js

+//= require modules/global-bar
+//= require modules/cross-domain-tracking
+
+//= require global-bar-init


Interestingly global-bar-init includes the cookie functions code from the gem, which is already pulled in by analytics.js in the gem, which is included here on line 8. Again, possibly out scope for this PR, but I wonder if there's an optimisation to made here, or whether rails is smart enough to not duplicate this code?

I don't know if there's any de-duping or tree-shaking going on in Rails's asset pipeline. This is the kind of thing that Webpack would help with - we could include dependencies as many times as we want and Webpack would ensure that they're only in the compiled code once.

owenatgov

Some finicky comments. Happy with this otherwise 👍

README.md

owenatgov · 2021-03-26T09:44:09Z

app/views/root/400.html.erb

@@ -1,5 +1,5 @@
 <%= render partial: "error_page", locals: {
    heading: "Sorry, there is a problem",
    intro: '<p class="govuk-body">Go back and change the information you entered.</p>',
-    status_code: 400
+    status_code: 400,


I was wondering why there were so many changed files in this PR 😂 Nice one 👍

I do love me a dangling comma. Better diffs ftw!

owenatgov · 2021-03-26T09:49:25Z

app/views/root/400.html.erb

@@ -1,5 +1,5 @@
 <%= render partial: "error_page", locals: {
    heading: "Sorry, there is a problem",


Maybe out of scope for this PR but there's a lot of hard coded content in these templates. Could we translate it? I don't know if this content would ever actually be translated, I'm picturing a situation where we implement something to set locale based on user info or whatever. It's more for good practice. At the very least it'll handle a few instances of repeated content. I don't even know if static supports translations.

I agree, there's a lot of content that could be translated here. I can't see rails-i18n or any localisation files in Static, so I think it'd need to be set up from scratch. With that in mind, setting up translations is outside of the scope of this pull request / should be a thing that is worthy of its own pull request.

I've raised this an an issue so we don't forget: #2448

That's perfectly reasonable. Is there a way we can at least make some of this content non-repeatable, maybe in a little partial or something? It's not a huge deal but just in case it has to change suddenly we can update it once.

Not a blocker for merging. Just blasting opinions.

I've had a go at this and I think it ends up convoluting things - the template ends up calling a partial, which calls another partial - and then the middle partial only sometimes overwrites the variables inside. It feels like premature optimisation that'd need to be undone when translations are used.

I think your first solution of using translations would be the best, simplest, and most future proof method to solve this - and that's outside the scope of this pull request.

* The base error page template updated to use the public layout component * Application stylesheet has been updated to include only the styles needed for the public layout component, global bar, and emergency banner. * Application JavaScript has been updated to only include the modules needed for the public layout component * Error page stylesheet has only the extra imports needed - in this case, only the the title component was needed * Minor tweak to make the apostrophe in the "We're" on some of the error pages look nicer * Updated tests to match content and layout changes

owenatgov

I am content with this. One final comment but otherwise I reckon this can go out (once it's ready to go out)

owenatgov · 2021-03-29T17:11:12Z

app/views/root/400.html.erb

@@ -1,5 +1,5 @@
 <%= render partial: "error_page", locals: {
    heading: "Sorry, there is a problem",


That's perfectly reasonable. Is there a way we can at least make some of this content non-repeatable, maybe in a little partial or something? It's not a huge deal but just in case it has to change suddenly we can update it once.

Not a blocker for merging. Just blasting opinions.

bevanloon temporarily deployed to govuk-static-update-err-inlukx March 19, 2021 11:53 Inactive

injms changed the title ~~Update error pages to use public layout template~~ 🚧 Update error pages to use public layout template Mar 19, 2021

injms added the Do not merge label Mar 19, 2021

injms force-pushed the update-error-page-layout branch from 42ee664 to 5f423d2 Compare March 19, 2021 19:24

bevanloon temporarily deployed to govuk-static-update-err-inlukx March 19, 2021 19:24 Inactive

maxgds reviewed Mar 22, 2021

View reviewed changes

app/assets/stylesheets/error-page.scss Outdated Show resolved Hide resolved

injms force-pushed the update-error-page-layout branch from 5f423d2 to c0c4d89 Compare March 22, 2021 09:44

bevanloon temporarily deployed to govuk-static-update-err-inlukx March 22, 2021 09:44 Inactive

injms force-pushed the update-error-page-layout branch from c0c4d89 to 06e6e28 Compare March 22, 2021 12:32

bevanloon requested a deployment to govuk-static-update-err-inlukx March 22, 2021 12:35 Abandoned

injms changed the title ~~🚧 Update error pages to use public layout template~~ Update error pages to use public layout template Mar 22, 2021

injms marked this pull request as ready for review March 22, 2021 13:10

injms removed the Do not merge label Mar 22, 2021

bevanloon temporarily deployed to govuk-static-update-err-inlukx March 22, 2021 16:22 Inactive

injms force-pushed the update-error-page-layout branch from 67c0bb6 to 12d8119 Compare March 22, 2021 17:13

bevanloon deployed to govuk-static-update-err-inlukx March 22, 2021 17:13 View deployment

andysellick reviewed Mar 22, 2021

View reviewed changes

bevanloon had a problem deploying to govuk-static-update-err-inlukx March 23, 2021 13:20 Failure

injms added the Do not merge label Mar 23, 2021

injms force-pushed the update-error-page-layout branch from d9a8395 to 7693dae Compare March 23, 2021 14:53

bevanloon temporarily deployed to govuk-static-update-err-inlukx March 23, 2021 14:53 Inactive

owenatgov reviewed Mar 26, 2021

View reviewed changes

injms force-pushed the update-error-page-layout branch from 7693dae to 3c9faec Compare March 29, 2021 12:54

injms added 2 commits March 29, 2021 15:06

Update JavaScript tests README to include Docker

374d0f7

injms force-pushed the update-error-page-layout branch from 3c9faec to 374d0f7 Compare March 29, 2021 14:08

injms removed the Do not merge label Mar 29, 2021

injms requested review from owenatgov and andysellick March 29, 2021 15:28

owenatgov approved these changes Mar 29, 2021

View reviewed changes

injms merged commit 49eb12c into master Mar 30, 2021

injms deleted the update-error-page-layout branch March 30, 2021 10:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update error pages to use public layout template #2440

Update error pages to use public layout template #2440

injms commented Mar 19, 2021 •

edited

Loading

andysellick left a comment

andysellick Mar 22, 2021

andysellick Mar 22, 2021

injms Mar 23, 2021

andysellick Mar 22, 2021

injms Mar 23, 2021

owenatgov left a comment

owenatgov Mar 26, 2021

injms Mar 26, 2021

owenatgov Mar 26, 2021

injms Mar 26, 2021

owenatgov Mar 29, 2021

injms Mar 30, 2021

owenatgov left a comment

owenatgov Mar 29, 2021


		//= require govuk_publishing_components/dependencies

		//= require govuk_publishing_components/lib

		@@ -1,5 +1,5 @@
		<%= render partial: "error_page", locals: {
		heading: "Sorry, there is a problem",

Update error pages to use public layout template #2440

Update error pages to use public layout template #2440

Conversation

injms commented Mar 19, 2021 • edited Loading

What

Why

Visual differences

andysellick left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

owenatgov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

owenatgov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

injms commented Mar 19, 2021 •

edited

Loading