Skip to content

[WIP] Remove GOV.UK frontend toolkit and convert to GOV.UK Frontend#34

Closed
Nooshu wants to merge 8 commits into
masterfrom
update-to-govuk-frontend
Closed

[WIP] Remove GOV.UK frontend toolkit and convert to GOV.UK Frontend#34
Nooshu wants to merge 8 commits into
masterfrom
update-to-govuk-frontend

Conversation

@Nooshu
Copy link
Copy Markdown

@Nooshu Nooshu commented Jul 19, 2018

See task list of areas left to be integrated

  • Can we use NPM to install Frontend?
  • Check colours all map correctly from Frontend.
  • Move JavaScript from FE into the correct place.
  • Use the Phase banner component from Frontend.
  • Use skip link component from Frontend.
  • Adopt new typography classes from Frontend.
  • Adopt new spacing scale from Frontend.
  • Adopt new header from Frontend.
  • Adopt new footer from Frontend.

Related issue: #154 raised here.

The dependency on GOV.UK frontend toolkit has been removed and been replaced by GOV.UK Frontend.

Note: this also fixes the Base64 encoded font issue / PR too.

Below are some screenshots comparing the homepage for both on mobile, tablet and desktop:

Mobile

Toolkit

mob-toolkit

Frontend

mob-frontend

Tablet

Toolkit

tab-toolkit

Frontend

tab-frontend

Desktop

Toolkit

dt-toolkit

Frontend

dt-frontend

@Nooshu Nooshu force-pushed the update-to-govuk-frontend branch 3 times, most recently from 923a604 to 3883f3f Compare July 19, 2018 15:17
@Nooshu Nooshu force-pushed the update-to-govuk-frontend branch from 3883f3f to 96e9916 Compare July 19, 2018 15:19
Copy link
Copy Markdown
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.

This feels like it's adopting the framework but avoiding adopting any of the conventions - is that intentional?

Comment thread example/Gemfile
source 'https://rubygems.org'

gem 'govuk_tech_docs', path: '..'
gem 'middleman-search', git: 'git://github.com/alphagov/middleman-search.git'
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Don't think this change is related?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Without this change the build freezes. This is just about to be rolled into master I believe.

@@ -36,12 +57,12 @@ $desktop-breakpoint: 992px !default;
@import "accessibility";

html, body {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ideally the template should be updated to use the classes from https://github.com/alphagov/govuk-frontend/blob/master/src/core/_template.scss


a:link {
color: $link-colour;
color: govuk-colour("blue");
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggest enabling global link and paragraph styling in Frontend and removing these link rules.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If not, this should be $govuk-link-colour

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I'm going to raise all of your suggestions as GH suggestions

@@ -0,0 +1,75 @@
# The `src` directory
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ideally GOV.UK Frontend would be installed via npm install to avoid having to vendor it in.

If that's not possible, I strongly recommend using the package folder from the repo rather than the contents of source, as the Sass in source is not vendor-prefixed, and the source folder includes e.g. all the tests.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

@tijmenb thoughts on use of npm here?

@@ -0,0 +1,53 @@
import { nodeListForEach } from './common'
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The javascript from Frontend is inside a folder called 'stylesheets'. Suggest moving the Frontend dependency into a vendor directory or something (assuming installing it via npm install is not possible)


a:hover {
color: $link-hover-colour;
color: govuk-colour("light-blue");
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

$govuk-link-hover-colour

}
}
}

@include print {
a:link, a:visited {
color: $black;
color: govuk-colour("black");
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

$govuk-print-text-colour (which is actually #000!)

@@ -9,14 +9,14 @@
*/

.phase-banner {
@include bold-14;
@include govuk-font(14, $weight: bold);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can we use the phase banner component from Frontend?

@@ -19,13 +19,13 @@
&:focus {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can we use the skip link component from Frontend?

@@ -42,65 +46,65 @@

// Headings
h1 {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can we use the new typography classes from Frontend? We do this in the Design System like this

@Nooshu
Copy link
Copy Markdown
Author

Nooshu commented Jul 19, 2018

This feels like it's adopting the framework but avoiding adopting any of the conventions - is that intentional?

At the moment yes to get it to a state where it looks and functions fine. I've raised the PR for general feedback and also in the hope that someone will be able to pick up some of the other areas of integration as it is quite a time consuming task.

@Nooshu Nooshu changed the title Remove GOV.UK frontend toolkit and converted to GOV.UK Frontend [WIP] Remove GOV.UK frontend toolkit and converted to GOV.UK Frontend Jul 19, 2018
@Nooshu Nooshu changed the title [WIP] Remove GOV.UK frontend toolkit and converted to GOV.UK Frontend [WIP] Remove GOV.UK frontend toolkit and convert to GOV.UK Frontend Jul 19, 2018
Copy link
Copy Markdown
Contributor

@lewisnyman lewisnyman left a comment

Choose a reason for hiding this comment

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

It's great to see this being implemented., thanks for working on this.
I ran a quick visual regression test. It looks like the new logo implementation increases the logo height by one pixel, I don't know if that's worth worrying about or not.

@@ -14,7 +14,7 @@
.collapsible__toggle {
position: absolute;
top: 0;
right: -25px;
right: $govuk-gutter-half * -1;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This was the only other visual regression I found, this changes the value and moves the placement of the toggle button. I'd recommend using the previous value.

@Nooshu
Copy link
Copy Markdown
Author

Nooshu commented Aug 9, 2018

Currently in the process or integrating the DS components into the tech docs gem. Integration of the header component / phase banner has brought up a couple of design considerations.

Is the plan to keep the header / navigation the same but directly bring code across from the DS? If so the output will look like this:
screen shot 2018-08-09 at 14 38 53

Or should we be considering using the navigation that is now being used for the Design System:
screen shot 2018-08-09 at 14 39 07

I personally think the overall design on the DS nav / header looks better than "Header with service name and navigation", but I'm happy to go with whatever the community would prefer.

@MatMoore
Copy link
Copy Markdown
Contributor

@Nooshu what's the name of that component the design system uses?

I like how it works on the design system site, but I'm wondering how it would work for sites where the tech docs is only part of the service. For example on registers this page has a different layout to the documentation part, which is a bit jarring, but the top level navigation still looks the same wherever you are.

Also on the GDS way site, one of the top level links is an external link, which might not work well with the new thing.

@Nooshu
Copy link
Copy Markdown
Author

Nooshu commented Aug 10, 2018

@MatMoore It is the "Header with service name and navigation" which can be seen on this page. I did wonder if it may not work for all documentation, so it is probably a safer to stick with what we know.

@Nooshu
Copy link
Copy Markdown
Author

Nooshu commented Aug 10, 2018

Pushing latest updates:

  • NPM now being used to pull in Frontend. This may be temporary until a discussion around the Ruby pipeline is had.
  • DS footer now being used.
  • Initial version of the DS Header is being used. This will be swapped out with a version that closely matches the current setup (after discussion with @36degrees and @dashouse).
  • DS Phase banner has been integrated, this may be removed depending on what happens with the updated header.
  • DS JS has been copied across to the assets folder as a temporary measure until the Ruby pipeline integration has been discussed.

Issues:

  • Small issue to be solved with horizontal padding when moving between breakpoints with the new footer.
  • Two sets of JS detection CSS classes currently in place. Modernizr and DS systems. Modernizr is used extensively for the overall layout, this could be used in the DS header with a little tweaking (js-enabled to js).

@bendyson
Copy link
Copy Markdown

@Nooshu Just out of interest, is this still being worked on?

@Nooshu
Copy link
Copy Markdown
Author

Nooshu commented Feb 14, 2019

@bendyson not at the moment I'm afraid. Leaving the information here and I'm hoping someone else will have the capacity to finish it.


a:link {
color: $link-colour;
color: govuk-colour("blue");
Copy link
Copy Markdown

@tlwr tlwr Mar 18, 2019

Choose a reason for hiding this comment

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

Suggested change
color: govuk-colour("blue");
color: $govuk-link-colour;


a:hover {
color: $link-hover-colour;
color: govuk-colour("light-blue");
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Suggested change
color: govuk-colour("light-blue");
color: $govuk-link-hover-colour;


a:active {
color: $link-active-colour;
color: govuk-colour("light-blue");
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Suggested change
color: govuk-colour("light-blue");
color: $govuk-link-active-colour;

@tijmenb
Copy link
Copy Markdown
Contributor

tijmenb commented Jul 31, 2019

This is superseded by #89 now, thanks @Nooshu!

@tijmenb tijmenb closed this Jul 31, 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.

7 participants