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

V2/V4 - template and install assume cssmodules: true #43

Closed
sthzg opened this issue Aug 4, 2016 · 5 comments
Closed

V2/V4 - template and install assume cssmodules: true #43

sthzg opened this issue Aug 4, 2016 · 5 comments
Assignees

Comments

@sthzg
Copy link
Member

sthzg commented Aug 4, 2016

I have just realized that the template's App.js and app.cssmodule.css implicitly assume a generator setup that will use css modules. When installing a V4 setup with prompting no for cssmodules, the dependency for "react-css-modules": "^3.7.6" is still present in package.json (thus it gets installed no matter what the user answers the prompt, and App.js will always have the @cssmodules annotation).

I see four paths from here:

  1. pulling out App.js/app.cssmodule.css from the template and into the generator
  2. defaulting App.js and app.css not to use css modules
  3. stop associating app.js with a style file at all
  4. keeping App.js and app.cssmodule.css as default in the template project and overriding it from the generator if necessary

I'd argument that 1 is the worst of these options, since the template would no longer work standalone, which I'd consider not being an option.

2, 3, and 4 should all be possible. 4 would require custom code (involving the deletion of app.cssmodule.css) and feel a bit unnatural, so at least as of now I think that I am pro 2 or 3.

@weblogixx what's your opinion, do I miss something more obvious?

@weblogixx
Copy link
Member

Thats a good question. I think we should remove the cssmodules from App.js alltogether. It was nothing more as a demo to get started with anyways. We could add the react-css-modules package as an install step. What do you think?

@sthzg
Copy link
Member Author

sthzg commented Aug 5, 2016

That's great, I also think it's the best decision. I will work on this asap.

  • remove cssmodules import in App.js
  • remove cssmodules annotation in App.js
  • change import stmt to not assign styles to a variable
  • remove dep to cssmodules in package.json

For conditionally adding the dep to cssmodules I will create a separate issue on the generator repository.

@sthzg sthzg self-assigned this Aug 5, 2016
sthzg added a commit that referenced this issue Aug 5, 2016
As the V4 generator supports projects with and without cssmodules, the
template repo may not make assumptions about them being enabled.
@sthzg
Copy link
Member Author

sthzg commented Aug 5, 2016

@weblogixx above's commit would be the changes required in this repo. Base.js still has the loader config for files with and without cssmodules, but that doesn't break anything and makes it easy for standalone users of the template to just install the dependency and start using css modules.

If you are fine with this I would merge and publish to start the work on the generator-side of this feature.

@weblogixx
Copy link
Member

@sthzg, looks fine for me. Give it a go 👍

sthzg added a commit that referenced this issue Aug 5, 2016
As the V4 generator supports projects with and without cssmodules, the
template repo may not make assumptions about them being enabled.

Closes #43
@sthzg sthzg removed the actionable label Aug 5, 2016
@sthzg
Copy link
Member Author

sthzg commented Aug 5, 2016

Merged with 59f0a2a

@sthzg sthzg closed this as completed Aug 5, 2016
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

No branches or pull requests

2 participants