Add GOV.UK Frontend, upgrade the phase tag

tijmenb commented 5 years ago

We've been discussing in GDS how to upgrade this project to use GOV.UK Frontend.

This is a far less ambitious attempt than https://github.com/alphagov/tech-docs-gem/pull/34 by @Nooshu and https://github.com/alphagov/tech-docs-gem/pull/88 by @JonathanHallam.

It does 3 things:

Add all of the GOV.UK Frontend CSS to the stylesheets. This has no effect on the UI because GOV.UK Frontend is properly namespaced and we have no class name conflicts.
Upgrade the "tag" in the header to use GOV.UK Frontend instead of custom styles
Remove the CSS that was imported from GOV.UK Frontend to make https://github.com/alphagov/tech-docs-gem/pull/78 work

Difference

There's a bit size difference, but not enough to matter.

Before

After

Future work

After this PR, we should be able to chip away at the styles in this project and convert them to use GOV.UK Frontend components and mixins. We'll also need to include the Javascript and images - I've left those out for now.

jonathanglassman commented 5 years ago

@tijmenb sounds good. So, just to check I'm understanding you correctly:

this change makes no difference to the current appearance, style and function of the template, including the warning text functionality
this is a way to iteratively convert the template to use GOV.UK Frontend, thereby ensuring that our tech docs are in line with WCAG 2.1 (and eventually 3.0)

@JonathanHallam is this the same method that you suggested after the firebreak work?

Apologies if these are silly questions

tijmenb commented 5 years ago

@jonathanglassman Yes, that's entirely correct! At least it's the intention. Someone like @nickcolley or @36degrees may be able to confirm.

The only additional change for users should be that the stylesheets have increased in size with this update (haven't bothered to check by how much), since we load an additional framework. This is a temporary price worth paying during the transition period.

NickColley commented 5 years ago

Yes that's correct, GOV.UK Frontend is designed to work alongside the legacy projects without conflicting.

Adding GOV.UK Frontend in small iterations like how Tijmen has proposed is what we recommend teams do for bigger projects.

That said I would always do your regular review of your project as the world of frontend is an unpredictable place.

jonathanglassman commented 5 years ago

@tijmenb @nickcolley that's brilliant, thank you.

Do we need need more discussion / review of this, or are we happy? @36degrees any queries or concerns? Should anyone else review?

Obviously happy to trust everyone's judgement, just want to make sure we engage all the necessary people

Nooshu commented 5 years ago

Thumbs up from me, I think this is a great approach to tackle this project that no single team owns. This should allow others in the tech community to gradually integrate other GOV.UK Frontend components over time.

aliuk2012 commented 5 years ago

I'm still reviewing the code but seeing as this PR introduces Node.js. I think it would be worth adding a mention about using nvm. It would also be good to add a .nvmrc file and set it to LTS (10.16.0).

JonathanHallam commented 5 years ago

@tijmenb nice one 🍾

jonathanglassman commented 5 years ago

Another quick question - Will we need to raise another PR to include the Javascript and images?

aliuk2012 commented 5 years ago

@jonathanglassman I think the answer is yes to your question relating to another PR to add Javascript and images.

We'll also need to include the Javascript and images - I've left those out for now.

36degrees commented 5 years ago

I do wonder whether we should be somehow bundling frontend into the gem, rather than relying on everyone using the tech-docs to manage that npm dependency themselves, and having to update their deploy pipeline to add the npm install step.

Other than that, the gradual adoption makes sense, but there aren't actually that many components being used here and I think there's still a fairly large chunk of work to adopt the typography classes, which will need to be done as one step.

Things like the table of contents will also need thinking about in terms of how we adopt the new focus states once Frontend v3 ships.

tijmenb commented 5 years ago

@36degrees the intention is to bundle GOV.UK Frontend into the gem as you describe, so gem users don't have to have npm and don't need to upgrade. It's a bit magic though - I'll try to adds some more comments around this.

36degrees commented 5 years ago

Gotcha, I think I was confused by this change to the README:

Before you start, you need to run the following in your project to download GOV.UK Frontend.

More documentation around the magic™ would definitely be good.

tijmenb commented 5 years ago

I'm maybe changing my mind. Maybe we should just commit the node_modules directory, then we don't have to explain the magic and people will not even have to have npm to work on this repo.

aliuk2012 commented 5 years ago

Not a bad idea because then people will not have to worry about node/npm and what versions they are using.

I still think we would need to explain it for local development purposes.

tijmenb commented 5 years ago

Well there we go 😄

NickColley commented 5 years ago

If you do go down that route of commiting node_modules it's important you document the version of Node node --version and npm npm --version so that it can be reproduced.

Like Al says we use nvm with an .nvmrc file for this in development.

aliuk2012 commented 5 years ago

I found these errors https://travis-ci.org/alphagov/tech-docs-gem/builds/560019518#L696-L703

This is an issue with how this repo is setup and not govuk-frontend as identified here https://github.com/alphagov/govuk-frontend/issues/1350.

tijmenb commented 5 years ago

Great catch @aliuk2012 - this PR won't work in production as it stands. I've raised https://github.com/alphagov/tech-docs-gem/pull/92 so that the tests fail for these errors.

I'm unsure what to do about this though - it seems middleman insists on using the sass gem, which is incompatible with GOV.UK Frontend, and we should be using sassc? Or is it the other way around?

injms commented 5 years ago

I'm unsure what to do about this though - it seems middleman insists on using the sass gem, which is incompatible with GOV.UK Frontend, and we should be using sassc? Or is it the other way around?

The sass gem will cope with CSS calc() fine, but it's deprecated. The problem is that sassc, which uses libsass, can't cope with the difference between CSS calc() and Sass calc().

There is an issue raised against libsass, and it appears a fix could be released with version 3.6 which is scheduled for 1st September this year.

tijmenb commented 5 years ago

As far as I understand it, the sassc incompatibility with GOV.UK Frontend can only be solved by a) removing the CSS compression or b) waiting for a new libsass.

Given that removing CSS compression will affect users negatively, we'll wait for an update to libsass.

Note that I won't be in GDS anymore once it's released, so someone else needs to pick this up!

NickColley commented 5 years ago

For CSS files the biggest wins come from the compression done by the server serving the files, for example GZIP.

Considering this there might not be a big impact on users as initially might think, which means this could continue and be improved when the new version is released.

tijmenb commented 5 years ago

@nickcolley Ok, seems reasonable. I've added https://github.com/alphagov/tech-docs-gem/pull/89/commits/34e5b1c4987bc369e5b87fcbeee2360dea2db371 to remove precompilation.

DilwoarH commented 5 years ago

@tijmenb - Should we not include the package.json in there so that updating the module is easy?

tijmenb commented 5 years ago

The struggle continues! Nearly there I think.

I've released the current branch as a pre-released gem version:

https://rubygems.org/gems/govuk_tech_docs/versions/1.9.0.pre.rc2

This means we can try the gem version in a few sites to see if it works properly.

tijmenb commented 5 years ago

I've tried the prerelease gem on GDS Way, the example app and the tech docs docs, and it all works fine. This PR is ready for review again!

jonathanglassman commented 5 years ago

I've tried the prerelease gem on GDS Way, the example app and the tech docs docs, and it all works fine. This PR is ready for review again!

Thanks Tijmen. Just to be safe, can someone try it with https://github.com/alphagov/paas-tech-docs and https://github.com/alphagov/verify-tech-docs? This are fairly large doc sites with a mix of nested and non-nested multipages, and multiple content types. Just to be on the safe side.

tijmenb commented 5 years ago

Thanks @alex-ju!

Note that we've gone with the original approach of not committing node_modules after all (a reversal of https://github.com/alphagov/tech-docs-gem/pull/89#issuecomment-512290824). This because we now have standard-js (https://github.com/alphagov/tech-docs-gem/pull/91), which means we need to add node_modules to gitignore anyway.

tijmenb commented 5 years ago

@jonathanglassman those sites should be good, as they don't differ that much from the ones I tested. In the interest of progress I'm going merge this now! Users of the gem should of course test properly, as they would for all updates.

alphagov / tech-docs-gem