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

Allow individual components to be imported into apps #1159

Merged
merged 8 commits into from
Feb 21, 2020
Merged

Conversation

andysellick
Copy link
Contributor

@andysellick andysellick commented Oct 11, 2019

What

Change the components gem to support the model proposed in RFC 108 where individual components are included in an application rather than all of them.

The Sass now contains two additional files necessary for applications when importing individual components:

  • govuk_frontend_support, which contains general settings from the gem plus everything from govuk-frontend that adds no weight to the compiled CSS (settings, tools and helpers)
  • component_support, which contains everything else from govuk-frontend, plus helpers and mixins from the gem used by various components

So an application would import both of these files, then the sass for the components it needs. This has been split into two files like this so that static can import the first one without incurring additional CSS size or duplication, while still being able to make use of govuk-frontend mixins and variables (we need this now we're removing toolkit from static).

The component guide's sass is separate and has been updated to include all components, since several are used.

The previous mechanism for including the gem's Sass is untouched, so this change should be backwards compatible.

Why

See the RFC.

Visual Changes

Visual changes to a component guide inside an application. Now generates a list of components at the top that are used by this application. Also added numbers to all of the headings.

Screen Shot 2019-10-11 at 16 55 16

Underneath this heading is a snippet of Sass that can be used in the app to replace the @import all_components line, again generated.

Screenshot 2020-02-14 at 14 25 39

View Changes

https://govuk-publishing-compo-pr-1159.herokuapp.com/component-guide

Impact of the change

I've done some testing to see what impact these changes have on CSS files in applications. Here's some early results.

Application application.css print.css
finder-frontend Before: 678 KB Before: 36 KB
finder-frontend After: 474 KB After: 9 KB
government-frontend Before: 692 KB Before: 36 KB
government-frontend After: 551 KB After: 10 KB
collections Before: 703 KB Before: 35.6 KB
collections After: 580 KB After: 16.4 KB

We also want to check how long the Sass takes to compile compared to previously. Here are some numbers, but they seem to vary considerably between tests with the same app, so they may not be reliable.

These tests are done by:

  • bundle exec rake assets:clean
  • rm -rf public/NAME-OF-APP/*
  • time bundle exec rake assets:precompile
Application Current compile time New compile time
government-frontend 1m57.699s 0m36.342s
finder-frontend 0m10.247s 0m10.957s
collections 1m3.144s 1m23.469s

The cookie banner question

The cookie banner is displayed from static. Currently, since every application pulls in every component, when a page is rendered the markup for the cookie banner comes from static but the styles come from the gem, via the app.

The new auto-generated list of sass files needed by an app therefore doesn't include the cookie banner, because the app doesn't use it. So I'm seeing the cookie banner unstyled when testing. There are two solutions:

  • hard code the cookie banner sass into the suggested sass for every application
  • import the cookie banner sass into static (this should be possible without extra imports, the sass seems to only depend on some govuk-frontend mixins, which static would have at that point)

Importing the cookie banner sass into static seems like the best option - it avoids duplication and puts the sass where it's actually used. This might need documenting somewhere when we change static to include these changes, I'm writing this to remind myself.

@bevanloon bevanloon temporarily deployed to govuk-publishing-compo-pr-1159 October 11, 2019 06:38 Inactive
@bevanloon bevanloon temporarily deployed to govuk-publishing-compo-pr-1159 October 11, 2019 15:34 Inactive
@bevanloon bevanloon temporarily deployed to govuk-publishing-compo-pr-1159 October 11, 2019 15:39 Inactive
@bevanloon bevanloon temporarily deployed to govuk-publishing-compo-pr-1159 October 11, 2019 15:45 Inactive
@bevanloon bevanloon temporarily deployed to govuk-publishing-compo-pr-1159 October 11, 2019 15:53 Inactive
@bevanloon bevanloon temporarily deployed to govuk-publishing-compo-pr-1159 October 14, 2019 08:04 Inactive
@kr8n3r
Copy link
Contributor

kr8n3r commented Oct 17, 2019

in GOV.UK Frontend we built a mechanism in Sass that will only export components Sass once, no matter how many times it's imported - that's why aevery component's Sass in wrapped in an exports include

is there the same need in this implementation?

@andysellick
Copy link
Contributor Author

@kr8n3r possibly, although I'm currently more concerned about importing too much of the rest of govuk-frontend than I am of duplicating sass imports for our components. Although at some point we definitely want to import all of it, since we'll use all of it in apps somewhere.

@kr8n3r
Copy link
Contributor

kr8n3r commented Oct 17, 2019

@andysellick 👍
see if you can work with just importing

@import "settings/all";
@import "tools/all";
@import "helpers/all";

from govuk-frontend as these should not output any code

@andysellick
Copy link
Contributor Author

Note to self: this change to the model may be beneficial for static, in that it might be possible to make govuk-frontend's mixins available to static via the 'component_support' sass file here, see alphagov/static#2023 for an example of duplication that this could remove.

@andysellick andysellick changed the title [WIP] [DO NOT MERGE] Spike gem changes [DO NOT MERGE] Spike gem Sass changes Feb 14, 2020
@andysellick andysellick marked this pull request as ready for review February 14, 2020 14:33
@andysellick andysellick changed the title [DO NOT MERGE] Spike gem Sass changes [DO NOT MERGE] Allow individual components to be imported into apps Feb 19, 2020
@andysellick andysellick changed the title [DO NOT MERGE] Allow individual components to be imported into apps Allow individual components to be imported into apps Feb 19, 2020
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.

I had a quick look at this in preparation for today's meeting. While I like the "suggested sass" feature, I'm not sure about the approach to split the helpers. In a world where static will be using the public-frontend layout I don't see a need for it to require any helpers.

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.

Technically this looks ok and I understand this unblocks the ongoing work in static (by reducing unnecessary code duplication and enabling mixins usage from govuk-frontend). I do however expect govuk_frontend_support.scss and component_support.scss not to be separated for long as we're working to introduce the public frontend layout which means static shouldn't require any styles. Also, long-term, applications should ideally only require one file to pull in sass from govuk_publishing_components.

- code searches the view for instances of component use and creates a list on the component guide of those components
- streamline supporting sass into one line
- only add a component to it if it has a sass file
- wrap in a textarea for ease of copying and pasting
- put govuk-frontend import into separate sass file
- static needs govuk-frontend but not everything else in component support
- that was fine, but brand colours added weight to the CSS (everything else is mixins and variables)
- splitting it out like this means that static can import govuk-frontend and use it, all other apps just have to import one extra file
- print sass doesn't need component_support, would drastically increase the print css file size
@andysellick andysellick merged commit 346e1d3 into master Feb 21, 2020
@andysellick andysellick deleted the spike-rfc-108 branch February 21, 2020 13:27
This was referenced Feb 21, 2020
@alex-ju alex-ju mentioned this pull request Apr 24, 2020
3 tasks
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