-
-
Notifications
You must be signed in to change notification settings - Fork 86
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
Comments
|
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. |
hi, we're redoing this, to import only stuff it needs |
Blocked by alphagov/govuk-frontend#636 |
govuk-frontend seems to have been written in a modular way that may be compatible with CSS modules.
Some interesting thoughts here: http://bradfrost.com/blog/link/whats-wrong-with-css-in-js/ Though I think ...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) |
@marksy which branch? |
nm, think it's this one https://github.com/UKHomeOffice/govuk-react/tree/feature/sassExtract |
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 |
Hi yes that branch. Hope it is okay. |
we could probably extend this technique to make govuk-react, to some degree, work directly from the govuk-frontend styles |
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 |
You could probably stick it in a render function though |
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 |
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 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 |
Related facebook/create-react-app#5687 |
I'm not sure how we would deal with the .js-enabled {
.govuk-header__menu-button {
display: block;
@include mq ($from: desktop) {
display: none;
}
} 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. |
Regarding js-enabled, there are 2 approaches:
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:
(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. |
So from your example app above ( |
@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 :-) |
We want to:
And in the process, ensure we continue to support:
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:
2. Use CSS Modules
WIP: https://github.com/penx/govuk-frontend-react
We had previously avoided CSS modules because:
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.
The text was updated successfully, but these errors were encountered: