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

Automate synchronisation of styles with govuk-frontend #76

Open
penx opened this issue Feb 21, 2018 · 20 comments
Open

Automate synchronisation of styles with govuk-frontend #76

penx opened this issue Feb 21, 2018 · 20 comments

Comments

@penx
Copy link
Member

penx commented Feb 21, 2018

We want to:

  • ensure govuk-react (CSSinJS) provides the same features as govuk-frontend (CSS Modules) but with React specific components/templates
  • reduce the maintenance burden of keeping in line with govuk-frontend, ideally by automating the process.

And in the process, ensure we continue to support:

  • SSR, both with next.js and bespoke
  • tree shaking, critical rendering and path splitting compatibility via create-react-app

Options

1. Script the conversion of govuk-frontend to CSSinJS

WIP: https://github.com/penx/govuk-frontend-emotion

The main issues with this approach so far:

  • Using the CSS classes directly, we would always get declarations such as font. How would this work with theming + freedom of font choice for non GDS domains?
  • how to deal with js-enabled class

2. Use CSS Modules

WIP: https://github.com/penx/govuk-frontend-react

We had previously avoided CSS modules because:

  • govuk-frontend was in closed alpha and we weren't aware of it
  • we didn't want to reuse any other existing govuk CSS projects
  • CSS modules assume that the parent application's build system is configured to handle import styles from 'my-css-file.css' including any processors (sass, postcss)

Sass and CSS modules are now supported in create-react-app now, which means there is an established pattern for handling the import of CSS files that contain Sass.

The plan with this approach would be to use CSS modules for the core style import and continue using CSSinJS for any custom styles (e.g. the spinner).

Blocked by:

3. Use govuk-frontend CSS classes directly

This could be done either via importing the CSS files from each React component or just using the CSS classes and assuming they are present in the DOM. The latter, I think, is what govuk-react-components does.

We would still fall back to CSSinJS where we want custom styles.

We may not get any benefit from tree shaking or critical rendering.

Last time we checked, there was some cascading element selectors in govuk-frontend that we wanted to avoid.

We could look in to this route temporarily before transitioning to CSS modules later

4. Keep govuk-react bespoke

We could still have unit tests to check that the output is in line, e.g.

@penx
Copy link
Member Author

penx commented Feb 23, 2018

GOV.UK Frontend, the new frontend codebase which will replace Frontend Toolkit and GOV.UK Elements

https://designnotes.blog.gov.uk/2018/02/19/developing-new-typography-and-spacing-for-gov-uk-frontend/

@penx penx changed the title Consider sass-extract and sass-extract-js Ensure synchronisation of styles with other govuk open source projects Feb 23, 2018
@marksy
Copy link
Collaborator

marksy commented Mar 8, 2018

Yep, i have the sass-extract-js working in a branch. It was the wrong syntax on the example and also missing a dependency (sass-extract-loader) that was holding me up.

The sass-loader generates one large object so not sure what the best way to approach this would be... import just the button sass for our button component etc?

This will still generate a huge object because each component in govuk-frontend imports their global/common.scss, which in turn imports a whole bunch of stuff; settings tools, helpers, core, objects, overrides.

@kr8n3r
Copy link

kr8n3r commented Apr 5, 2018

hi, we're redoing this, to import only stuff it needs

@penx
Copy link
Member Author

penx commented Apr 9, 2018

Blocked by alphagov/govuk-frontend#636

@penx
Copy link
Member Author

penx commented May 9, 2018

govuk-frontend seems to have been written in a modular way that may be compatible with CSS modules.

  1. is there any way to get the full Sass output converted to something that is usable in JS?
  2. what would be the benefits, drawbacks and difficulties in using both CSS Modules (for external dependencies) and CSSinJS (for internal/custom CSS)?

Some interesting thoughts here:

http://bradfrost.com/blog/link/whats-wrong-with-css-in-js/

Though I think
(2) is a stylistic problem that is easily solved by having small components
(3) is about being better developers rather than a core issue

...only (1) seems to have any potential issue, but I would imagine ISTF will provide a good cross framework solution in future (and you could apply a similar argument saying Sass isn't portable to LESS)

@penx penx unassigned marksy May 24, 2018
@penx
Copy link
Member Author

penx commented Aug 17, 2018

i have the sass-extract-js working in a branch

@marksy which branch?

@penx
Copy link
Member Author

penx commented Aug 17, 2018

@penx
Copy link
Member Author

penx commented Aug 17, 2018

I made a script that converts govuk-frontend in to emotion, it lives here https://github.com/penx/govuk-frontend-emotion

it's published on npm and there's example usage here https://github.com/penx/govuk-frontend-emotion-example

it's still a work in progress.. but it works

screen shot 2018-08-17 at 20 36 42

@marksy
Copy link
Collaborator

marksy commented Aug 17, 2018

Hi yes that branch. Hope it is okay.

@penx
Copy link
Member Author

penx commented Aug 17, 2018

we could probably extend this technique to make govuk-react, to some degree, work directly from the govuk-frontend styles

@penx
Copy link
Member Author

penx commented Aug 17, 2018

we would still need to manually create React components using the appropriate DOM element that makes use of these styles, there may be a way to use the existing nunjucks or html templates to power this but probably a lot of work

@marksy
Copy link
Collaborator

marksy commented Aug 17, 2018

You could probably stick it in a render function though

@penx
Copy link
Member Author

penx commented Oct 26, 2018

Interesting interaction between govuk-frontend-toolkit and emotion here:

https://github.com/cds-snc/ircc-rescheduler/blob/master/src/components/forms/Button.js#L89

As linked from
#435 (comment)

@pcraig3
Copy link

pcraig3 commented Oct 29, 2018

Hi there,

Yeah, the idea was to take keep the default button styles separated from the app-specific styling as a nod towards portability. In theory, the govuk_button rules could live somewhere else entirely (although then you would have to be pretty familiar with them to know what removing an overridden rule would do).

Those styles never were used outside of that repo though, so there was never any problem of having to update a general set of rules and worrying about others downstream.

It might be an evolutionary cul-de-sac, but it was super flexible for our project and let us pass styles around the app pretty easily (for example, our visuallyhidden styles, which we could add to any other component where convenient).

@penx penx changed the title Ensure synchronisation of styles with other govuk open source projects Automate synchronisation of styles with govuk-frontend Nov 1, 2018
@penx
Copy link
Member Author

penx commented Nov 2, 2018

Related facebook/create-react-app#5687

@penx
Copy link
Member Author

penx commented Nov 22, 2018

I'm not sure how we would deal with the js-enabled class in any automated conversion:

  .js-enabled {
    .govuk-header__menu-button {
      display: block;
      @include mq ($from: desktop) {
        display: none;
      }
    }

https://github.com/alphagov/govuk-frontend/blob/dd31d1d863be57d66ea285bbe8b6470dc84a1990/src/components/header/_header.scss#L189-L195

It would be good to have the govuk-react Header/TopNav support JS disabled on the frontend using something to similar to this, but not sure how this could be done automatically.

@penx penx self-assigned this Dec 5, 2018
@penx penx added this to To do in Version 2 Dec 8, 2018
@penx
Copy link
Member Author

penx commented Dec 11, 2018

Regarding js-enabled, there are 2 approaches:

  1. have some js that manually adds this to the body (or similar) element and have a *.module.css that uses :global(.js-enabled) with the equivalent content of the *.css file.

  2. use React to detect js-enabled via componentDidMount, perhaps using the context api to ensure it can be optimised to only run once.

This also highlights an important point about why we can't use govuk-frontend directly when doing React work, and there is need for a React component library that does more than just templating:

React development encourages you to not modify the DOM directly:

React is unaware of changes made to the DOM outside of React. It determines updates based on its own internal representation, and if the same DOM nodes are manipulated by another library, React gets confused and has no way to recover.
https://reactjs.org/docs/integrating-with-other-libraries.html

(also see https://reactjs.org/docs/refs-and-the-dom.html)

govuk-frontend is dependent on scripts such as:

  if ($toggleButton && $target) {
    this.toggleClass($target, 'govuk-header__navigation--open')
    this.toggleClass($toggleButton, 'govuk-header__menu-button--open')

https://github.com/alphagov/govuk-frontend/blob/master/src/components/header/header.js

Such scripts need to be rewritten to be handled via React rather than allowing direct DOM manipulation. In addition, the means a component like the Header has internal state (open or closed, and potentially js enabled or disabled). In React, it's good to allow stateful components to be able to act as both controlled and uncontrolled.

@marksy
Copy link
Collaborator

marksy commented Dec 11, 2018

.js-enabled doesn't have to go on the body though. Can't it just be applied to the component (or it's parent)?

So from your example app above (govuk-frontend-emotion-example/src/App.js line 21)
<div className="App js-enabled">

@penx
Copy link
Member Author

penx commented Dec 11, 2018

@marksy yep :-)

penx/govuk-frontend-react@38a2a70

@penx
Copy link
Member Author

penx commented Jun 2, 2021

@stevesims @marksy @Loque- I am pretty convinced option 2 (via https://github.com/penx/govuk-frontend-react) is the way to go here.

I'm keen to know if anyone has any reservations on this approach on this so they can be considered/discussed before I spend any significant time heading in that direction :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Version 2
  
To do
Development

No branches or pull requests

4 participants