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

Update to GOV.UK Frontend #512

Merged
merged 41 commits into from
Jun 27, 2018
Merged

Update to GOV.UK Frontend #512

merged 41 commits into from
Jun 27, 2018

Conversation

NickColley
Copy link
Contributor

@NickColley NickColley commented Jun 25, 2018

This is an attempt to do a cleaner merge of the changes from the https://github.com/alphagov/govuk-prototype-kit-private-beta.

Includes:

  • Removal of Elements, Template, Toolkit
  • Migration of all code
  • Updates to guidance relating to Elements, Template, Toolkit
  • New cookie banner component

If you want to review all changes without whitespace: https://github.com/alphagov/govuk_prototype_kit/pull/512/files?w=1

The other parts that are missing are being tracked with this milestone: https://github.com/alphagov/govuk_prototype_kit/milestone/1

https://trello.com/c/SrxrezjZ/1061-add-govuk-frontend-to-the-govuk-prototype-kit

@NickColley NickColley force-pushed the update-to-govuk-frontend-7.0.0 branch 6 times, most recently from 647fca4 to c25335b Compare June 25, 2018 16:32
@NickColley NickColley added this to the 7.0.0 milestone Jun 25, 2018
.unbranded {
background: $white;
// Remove canvas background colour, as is used with the GOV.UK Footer.
$govuk-canvas-background-colour: $govuk-body-background-colour;
Copy link

Choose a reason for hiding this comment

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

no need for this as we're not including the footer

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we don't do this we get a grey box at the bottom of the screen, since the <body> does not extend the same length as <html>

server.js Outdated
@@ -190,6 +205,7 @@ app.get('/prototype-admin/download-latest', function (req, res) {
if (useDocumentation) {
// Copy app locals to documentation app locals
documentationApp.locals = app.locals
documentationApp.locals.serviceName = 'Prototype Kit'
Copy link

Choose a reason for hiding this comment

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

this needs to be changed to how we've amended it in beta
documentationApp.locals = Object.assign({}, app.locals)
documentationApp.locals.serviceName = 'Prototype Kit'

Copy link
Contributor

Choose a reason for hiding this comment

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

dunno if its github being weird, but the line looks how you suggested already?
documentationApp.locals = Object.assign({}, app.locals)

Copy link
Contributor

Choose a reason for hiding this comment

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

ah it's because you commented on line 199 instead of 198 :)

@NickColley NickColley force-pushed the update-to-govuk-frontend-7.0.0 branch from c25335b to 4f6df40 Compare June 26, 2018 10:01
package.json Outdated
@@ -21,6 +21,7 @@
"express": "4.15.2",
"express-session": "^1.13.0",
"express-writer": "0.0.4",
"govuk-frontend": "0.0.32",
Copy link
Contributor

Choose a reason for hiding this comment

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

should this be 1.0.0?

.gitignore Outdated
@@ -5,7 +5,6 @@
.port.tmp
public
lib/govuk_template.html
govuk_modules
Copy link
Contributor

Choose a reason for hiding this comment

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

good catch! This isn't in the beta work

@NickColley NickColley force-pushed the update-to-govuk-frontend-7.0.0 branch from 4f6df40 to 5f3a671 Compare June 26, 2018 11:16
@@ -1,22 +0,0 @@
@import "_colours";
Copy link
Contributor

Choose a reason for hiding this comment

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

another good catch! This file has been hanging around for ages

Copy link
Contributor

@alex-ju alex-ju left a comment

Choose a reason for hiding this comment

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

Great cleanup! 💯

Missing the following:

Update: the missing PRs will be added afterwards, I'm going to approve this.


<div class="govuk-box-highlight">
<h1 class="heading-xlarge">
<h1 class="govuk-heading-xl">
Copy link
Contributor

Choose a reason for hiding this comment

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

This page should use the panel component.

@@ -18,23 +18,12 @@ Every time a change happens in [application.scss](../app/assets/sass/application

Try starting the app and adding some styles to `application.scss`. If you open `application.css` you should now see the compiled version of those styles.

## Using the govuk_frontend_toolkit
## Using the GOV.UK Frontend
Copy link
Contributor

Choose a reason for hiding this comment

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

delete 'the'

Copy link
Contributor

Choose a reason for hiding this comment

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

to be honest its probably best to remove this section, it's not very useful and tells people to do something thats not very clear, or we already do for them?


Note that the convention is to start the name of any partial with an underscore, like those in the toolkit.
Note that the convention is to start the name of any partial with an underscore, like those in the GOV.UK Frontend.
Copy link
Contributor

Choose a reason for hiding this comment

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

like those in the GOV.UK Frontend

@include bold-48;

The line `@import '_typography';` makes all the code in [_typography.scss](https://github.com/alphagov/govuk_frontend_toolkit/blob/master/stylesheets/_typography.scss) available. The `h1` can therefore be styled in the 48pt bold form of the font by using `@include bold-48;` to call the `bold-48` mixin.
You can use the Sass libraries in the [govuk-frontend](https://github.com/alphagov/govuk-frontend) by importing the files from there directly into `application.scss`.
Copy link
Contributor

Choose a reason for hiding this comment

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

in the govuk-frontend

<ul class="govuk-list govuk-list--bullet">
<li><a href="/docs/tutorials-and-examples">Example pages and documentation</a></li>
<li>
<a href="https://govuk-design-system-production.cloudapps.digital">
Copy link
Contributor

@joelanman joelanman Jun 26, 2018

Choose a reason for hiding this comment

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

<p class="text">
<a href="https://govuk-elements.herokuapp.com/">GOV.UK Elements</a> is a guide to making your prototype look like a GOV.UK
<p>
<a href="https://govuk-design-system-production.cloudapps.digital">GOV.UK Design System</a> is a guide to making your prototype look like a GOV.UK
Copy link
Contributor

@joelanman joelanman Jun 26, 2018

Choose a reason for hiding this comment

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

@@ -42,26 +42,23 @@ <h2 class="govuk-heading-l">Our cookie message</h2>

<p>You will see a message about cookies when you first visit the GOV.UK Prototype Kit. We’ll store a cookie so that your computer knows you’ve seen it and knows not to show it again.</p>
Copy link
Contributor

Choose a reason for hiding this comment

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

does this fix a problem with tables? Or just move it to using a macro?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just moving everything to use macros.

</ul>
-->
</nav>
Import the header component macro place it in the `{% block header %}`and provide `navigation` items as shown below.
Copy link
Contributor

Choose a reason for hiding this comment

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

does this work fix something or just move it to macros? If its just about macros can we skip this commit, as we're not ready to push macros in prototyping

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is how you override the header contents now.

@NickColley NickColley force-pushed the update-to-govuk-frontend-7.0.0 branch from 0bfcfc9 to 06a7e28 Compare June 26, 2018 17:52
@joelanman
Copy link
Contributor

Sorry I meant small changes plus reverting macros, not that that was small, I wasn't very clear.

@joelanman joelanman changed the title [Do not merge] Update to govuk frontend 7.0.0 [Do not merge] Update to GOV.UK Frontend Jun 27, 2018
@joelanman
Copy link
Contributor

joelanman commented Jun 27, 2018

Wondering if the Cookie banner work would be better as a separate PR? Is it part of 'Update to govuk frontend'?

UPDATE

We decided to keep it, as Cookie banner used to be in the old stack, and needs replacing in a move to Frontend, as it doesnt exist there.

NickColley and others added 2 commits June 27, 2018 12:03
Avoid using JavaScript by using server side cookie setting.

JavaScript is only necessary for www.GOV.UK since it can't easily set cookies.
@NickColley NickColley force-pushed the update-to-govuk-frontend-7.0.0 branch from 06a7e28 to 4fd81b0 Compare June 27, 2018 11:03
@NickColley
Copy link
Contributor Author

@joelanman yeah, I think it makes sense since otherwise this PR would break cookie support.

@NickColley NickColley changed the title [Do not merge] Update to GOV.UK Frontend Update to GOV.UK Frontend Jun 27, 2018
Copy link
Contributor

@joelanman joelanman left a comment

Choose a reason for hiding this comment

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

Looks good, thanks!

@NickColley NickColley merged commit 48ad1c3 into master Jun 27, 2018
@NickColley NickColley deleted the update-to-govuk-frontend-7.0.0 branch June 27, 2018 11:49
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.

4 participants