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

Compatibility with create-react-app #1052

Closed
penx opened this issue Nov 2, 2018 · 13 comments
Closed

Compatibility with create-react-app #1052

penx opened this issue Nov 2, 2018 · 13 comments
Labels
feature request User requests a new feature

Comments

@penx
Copy link

penx commented Nov 2, 2018

What?

Rename scss files that are designed to work as CSS Modules to _{moduleName}.module.scss

or (maybe, may need more thought) include a second file called _{moduleName}.module.scss that composes _{moduleName}.scss

Why?

Create React App 2 now supports Sass and CSS Modules:

More styling options: you can use Sass and CSS Modules out of the box.
https://reactjs.org/blog/2018/10/01/create-react-app-v2.html

Create React App has the convention that if you do:

import styles from "./button.scss";

It will treat button.scss as a normal SCSS file and append the styles to your stylesheet.

However, if you do:

import styles from "./button.module.scss";

It will treat button.module.scss as a CSS module and return an object to JS for class name lookup.

Consider the following:

import React from "react";
import styles from "./button.module.scss"; // works
// import styles from "govuk-frontend/components/button/_button.scss"; // doesn't work
// import styles from "@penx/govuk-frontend/components/button/_button.module.scss"; // works

const Button = props => (
  <button className={styles["govuk-button"]} {...props} />
);

export default Button;

Where button.module.scss is:

@import "~govuk-frontend/components/button/button";

.govuk-button {
  composes: govuk-button;
}

At the moment I have to create a mostly redundant file to make sure that _button.scss is picked up as a CSS module.

Using:

import styles from "govuk-frontend/components/button/_button.scss"; 

Imports the CSS file but does not treat it as CSS module, so this does not work.

I have created a fork of govuk-frontend and renamed _button.scss to _button.module.scss, so if you do the following:

import styles from "@penx/govuk-frontend/components/button/_button.module.scss"; 

This makes govuk-frontend work with create-react-app out of the box.

You can try this out on the following sandbox:
https://codesandbox.io/s/w0x8xk5lk

@penx
Copy link
Author

penx commented Nov 2, 2018

(please do this for helpers such as objects/_main-wrapper.scss and not just components)

@NickColley
Copy link
Contributor

NickColley commented Nov 2, 2018

This looks really interesting, renaming them straight away would be a breaking change.

I wonder if we could have a build step that duplicates the original file with the different name, and deprecate the original file name for when we next do a breaking release?

Or is there a way to have the second file automatically compose the original file?

@NickColley
Copy link
Contributor

NickColley commented Nov 2, 2018

https://facebook.github.io/create-react-app/docs/adding-a-css-modules-stylesheet

I wonder if we could consider this a general way for people to be able to use Webpack with our CSS?

So support it for general CSS Module usecase and then we'd support whatever framework people are using.

Edit:

I think if you're a CSS Module user it already works, this is just specific to React Create App since they want to be able to support people just including raw CSS AND CSS Modules users so they use this naming convention.

@penx
Copy link
Author

penx commented Nov 2, 2018

On a side note, once this is done, it would allow a file at src/components/button/Button.jsx that contains:

import React from "react";
import cx from "classnames";
import styles from "./_button.module.scss";

const Button = ({className, ...props}) => (
  <button className={cx(styles["govuk-button"], className)} {...props} />
);

export default Button;

😀

Edit: though this wouldn't currently be supported by CRA facebook/create-react-app#5687

@penx
Copy link
Author

penx commented Nov 2, 2018

I think if you're a CSS Module user it already works, this is just specific to React Create App since they want to be able to support people just including raw CSS AND CSS Modules users so they use this naming convention.

I guess it's a CRA specific pattern, but only in that they want to pass a regex to webpack, so this seems like a reasonable convention.

It appears there is some work to make this a wider standard: css-modules/css-modules#229

@penx
Copy link
Author

penx commented Nov 2, 2018

Related facebook/create-react-app#5687

@kr8n3r
Copy link

kr8n3r commented Nov 10, 2018

angular-cli has a similar pattern. see https://github.com/igloosi/govuk-frontend-angular-cli-boilerplate/tree/master/src/app/app-button

@penx
Copy link
Author

penx commented Dec 7, 2018

Neutrino (webpack presets from Mozilla) also follows the *.module.css pattern:

Pre-configured to support CSS Modules via *.module.css file extensions
https://neutrinojs.org/packages/web/
https://neutrinojs.org/packages/style-loader/
https://neutrinojs.org/packages/react-components/
https://neutrinojs.org/packages/vue/

@penx
Copy link
Author

penx commented Dec 7, 2018

If there are any pointers on how you see this being implemented I could look at contributing this myself.

The two main questions I have are:

where in the build process would you recommend creating the *.module.css version, if it was automatically generated?

I am pretty sure your CSS naming convention assumes that if you have an element with .block__element--modifier then you should always have .block__element on the same element.

In the css modules version, should we enforce this by making .block__element--modifier always compose .block__element?

e.g. in _header.module.scss:

@import "./header";

.govuk-header__link {
  composes: govuk-header__link;
}
.govuk-header__link--homepage {
  composes: govuk-header__link;
  composes: govuk-header__link--homepage;
}

@penx
Copy link
Author

penx commented Dec 10, 2018

I've made good progress on a PoC of using CSS modules:

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

An example of this working out-of-the-box with Create React App, with code splitting/lazy loading and tree shaking:

https://penx.github.io/govuk-frontend-react-example/
https://www.github.com/penx/govuk-frontend-react-example

And a work in progress PoC for next.js (universal rendering):

https://www.github.com/penx/govuk-frontend-react-example-next

@36degrees 36degrees added this to This sprint in Design System Sprint Board via automation Jan 14, 2019
@hannalaakso hannalaakso removed this from This sprint in Design System Sprint Board Jan 16, 2019
@NickColley
Copy link
Contributor

Hey Alasdair!

Hanna and I have been going over your proposal for CSS Modules.

We've looked into the difference between importing what have now, and a file with the .module.css convention (CSS Modules).

When importing the regular file, you can use the class names as documented in the Design System.

In your issue you don't explain what barriers using markup in this way introduce.

We're assuming the two main benefits from using CSS Modules are as follows:

  1. Enables codesplitting at the class name level within a CSS file, so if you're not using a class in a CSS file it wont be built
  2. Enables you to avoid hand writing in a convention such as BEM

We think that for components, you'll need to use most if not all of the classes, and you can't get the benefits of avoiding BEM without rewriting all the CSS files.

We recognise in certain files such as the helpers or overrides, you could get better code splitting but we think if we looked at the compressed CSS gzip results that the main impact on file output would be from excluding certain components, which is already possible.

Is there something we've missed?

@penx
Copy link
Author

penx commented Jan 30, 2019

Ok first some history of govuk-react and why we use CSSinJS, skip this if you want the TL;DR:


When we started building govuk-react, govuk-frontend was not available and we weren't keen on the other alternatives for a 'component library' e.g. there was a fair amount of CSS that wasn't encapsulated (one component affecting the styles of another component, either with descendent or sibling selectors).

We decided to build our own bespoke CSS that matched the govuk look and feel, in a way that was more focussed towards encapsulated components.

We have other component libraries - govuk-react is the core styles and other component libraries follow the same structure for components specific to our application.

We wanted to ensure users could use govuk-react and our other libraries with confidence that components would be encapsulated and CSS would not conflict. We opted for CSSinJS because:

  • BEM achieves this but is a convention rather than being enforced in code, so can be fragile if not actively enforced
  • CSS modules enforces this but is hard to distribute as NPM modules (the fact that create-react-app and others now have a standard process for CSS modules goes some way to negate this, but this was not available at the time)

This is explained well in this blog post

https://medium.com/seek-blog/a-unified-styling-language-d0c208de2660

Another feature of govuk-react and it's associated libraries is that if you have a module structure like this:

└─┬ my-app
  ├─┬ module-one
  │ └─┬ govuk-react@0.4.0
  │   └── @govuk-react/date-input@0.4.0
  └─┬ module-two
    └─┬ govuk-react@0.4.0
      └── @govuk-react/date-input@0.4.0

If govuk-react@1 was released with breaking changes to date-input, and module-one wanted to upgrade to the latest version but module-two didn't, our structure would support this without having conflicting CSS.

In fact, because govuk-react is a monorepo, the developers of module-two would have several options if module-one upgraded:

  • use govuk-react@0.4.0 in module-two dependencies and govuk-react@1.0.0 in module-one dependencies
  • use govuk-react@1.0.0 in both module-one and module-two, but also specify @govuk-react/date-input@0.4.0 as a dependency of module two so that this component alone can be loaded in as an older version while still using the latest version of govuk-react for everything else
  • list govuk-react in the peerDependencies of module-one and use whatever version is specified by my-app

Again, these features are not just there for govuk-react, but for all other component libraries we build to also offer these features so that users of these libraries can easily work around breaking changes.


So this is why we would use CSSinJS or CSS modules for govuk-react, but I understand that may not provide sufficient argument for why you would want it for govuk-frontend.

The main benefits I see to govuk-frontend are:

  • if you introduce breaking changes in to govuk-frontend and peer applications want to use multiple versions alongside each other - I think this is a realistic scenario for a large and long lived applications
  • if other developers follow your structure, e.g. to provide extensions to govuk-frontend, and they introduce breaking changes Add an extension system for the prototype kit. govuk-design-system-architecture#9
  • so that peer applications can be sure that the CSS they import is encapsulated. We recently noticed govuk-frontend still has descendent selectors. From previous conversations with your team, I think around a year ago, we raised this as an issue and understood you were planning to remove any cross-component descendent or sibling selectors - we don't want this in govuk-react and CSS modules would prevent such code from conflicting with other components e.g.
    .govuk-character-count {
    @include govuk-responsive-margin(6, "bottom");
    .govuk-form-group,
    .govuk-textarea {
    )

@NickColley
Copy link
Contributor

We've spent time going over this with Alasdair and decided we won't support this directly in GOV.UK Frontend for now, if you'd like to see the details we've put together a proposal you can read..

Thanks for all your work on this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request User requests a new feature
Projects
None yet
Development

No branches or pull requests

3 participants