-
Notifications
You must be signed in to change notification settings - Fork 43
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
Comments
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? |
That's great, I also think it's the best decision. I will work on this asap.
For conditionally adding the dep to cssmodules I will create a separate issue on the generator repository. |
As the V4 generator supports projects with and without cssmodules, the template repo may not make assumptions about them being enabled.
@weblogixx above's commit would be the changes required in this repo. If you are fine with this I would merge and publish to start the work on the generator-side of this feature. |
@sthzg, looks fine for me. Give it a go 👍 |
Merged with 59f0a2a |
I have just realized that the template's
App.js
andapp.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, andApp.js
will always have the@cssmodules
annotation).I see four paths from here:
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?
The text was updated successfully, but these errors were encountered: