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

Adds PostCSS Normalize #5810

Merged
merged 8 commits into from
Apr 5, 2019
Merged

Adds PostCSS Normalize #5810

merged 8 commits into from
Apr 5, 2019

Conversation

mrchief
Copy link
Contributor

@mrchief mrchief commented Nov 15, 2018

This change brings in postcss-normalize as default reset css.

postcss-normalize has 3 options and their default values would suffice for most use cases. The one that users would care for most is the browsers options which can be overridden by .browserslistrc or browserslist setting in pacakge.json, so there is no issue leaving it as empty.

The remaining 2 are better left off (default) but I'm not sure if having a way for users to override those default would violate the core ideas. I'm not sure how often they are used so it's kind of hard for me to make a call here. I'd love to hear from the authors and community about this.

Fixes #2273.

@facebook-github-bot
Copy link

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed.

If you have received this in error or have any questions, please contact us at [email protected]. Thanks!

@facebook-github-bot
Copy link

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@ianschmitz
Copy link
Contributor

Hey thanks for your contribution!

To clairfy, the usage of this would be opt-in by using the @import as directed in the plugin?

@mrchief
Copy link
Contributor Author

mrchief commented Nov 15, 2018

@ianschmitz Yes, this is opt-in via the @import-normalize directive since forceImport defaults to false.

@mrchief
Copy link
Contributor Author

mrchief commented Nov 15, 2018

I'm not sure why one of the test suites failed. I ran them locally using WSL + Node 8.12 and I don't see them failing.

@mrchief
Copy link
Contributor Author

mrchief commented Nov 20, 2018

Is this waiting on me? If it's one of those tests then I'm not sure how to go about fixing them. To me, it seems random and I feel another dummy push may succeed. Please let me know if that's not the case.

@stale
Copy link

stale bot commented Dec 20, 2018

This pull request has been automatically marked as stale because it has not had any recent activity. It will be closed in 5 days if no further activity occurs.

@stale stale bot added the stale label Dec 20, 2018
@mrchief
Copy link
Contributor Author

mrchief commented Dec 21, 2018

Anyone?

@stale stale bot removed the stale label Dec 21, 2018
@yordis
Copy link

yordis commented Jan 16, 2019

I wouldn't like CRA to be injecting any CSS into my application.

If I want to use normalize.css I could install the package and include it on my files.

@mrchief
Copy link
Contributor Author

mrchief commented Jan 16, 2019

@yordis How is bringing in reset.css any different? This PR simply gives a choice to choose normalize.css over reset.css.

@yordis
Copy link

yordis commented Jan 16, 2019

@mrchief I didn't say that bringing reset.css is something that I support ... I said I wouldn't like CRA to be injecting ---> any <--- CSS into my application.

@mrchief
Copy link
Contributor Author

mrchief commented Jan 16, 2019

@yordis I understand but that seems to be a different discussion altogether. I assumed, maybe incorrectly, that that was some sort of reasoning to not merge this PR. With all due respect, to me, that discussion should happen in a different thread as I don't want others to turn this PR into a discussion about whether CRA should inject any css or not.

@yordis
Copy link

yordis commented Jan 16, 2019

@mrchief would you mind sharing where in the code CRA uses reset.css? Maybe I am lost track of CRA source code and it got introduced to it

@mrmckeb
Copy link
Contributor

mrmckeb commented Jan 16, 2019

As far as I'm aware, there isn't a reset in CRA. I think using reset/normalise/reboot is great, and encourage it - but we want to leave such decisions in the hands of developers as much as possible.

@yordis
Copy link

yordis commented Jan 16, 2019

@mrmckeb I was double checking with him since his response assumed that there was a reset.css inside CRA today and I wanted to make sure I was not talking without knowing.

Going back to my original message, which @mrmckeb said it better, CRA shouldn't do any of these things.

@mrchief
Copy link
Contributor Author

mrchief commented Jan 16, 2019

I had to dig in a bit but here's the issue that was the inspiration for this PR. It seems there is community interest in bringing in some kind of reset css and postcss-normalize was the most preferable. It's an opt-in behavior and by default, CRA won't bring in anything unless you opt-in to it.

Sorry about my previous assumtion about reset being brought in by default - my memory was fuzzy about that.

@yordis
Copy link

yordis commented Jan 17, 2019

As CLI and scripts for helping your development, this feature is out of CRA scope.

What if I am using reset.css on my project because I want to use it over normalize.css?

The number of questions and complain about this decision will be endless.

Again, people are not being stopped today from adding normalize.css.

@mrchief
Copy link
Contributor Author

mrchief commented Jan 17, 2019

I created this PR based on the interest of the community (myself included). Of all the possible choices, post-css-normalize seemed to be the most favored one due to it's flexibility. (I urge you again to go back to the thread I referenced if you haven't done so).

As CLI and scripts for helping your development, this feature is out of CRA scope.

This helps in development so I don't think that this is out of scope. It is quite painful and bit ugly to bring in your own without CRA inherently supporting it (many don't want to eject nor take over webpacking by themselves). It's all about having sane defaults. Not everyone is as experienced nor they necessarily know about all different options. This helps towards that.

Besides, that argument can be applied to any number of things that CRA (or any other CLI script in this world) does, so how this is any different?

What if I am using reset.css on my project because I want to use it over normalize.css?

If you don't want use normalize.css, then don't opt-in. You can always use your favorite css.

The number of questions and complain about this decision will be endless.

Maybe. Maybe not. This is not a forceful default, nor a mandatory choice so who knows. People are complaining about hooks too, ¯_(ツ)_/¯ !

@Timer @Fabianopb @ianschmitz your thoughts?

@yordis
Copy link

yordis commented Jan 17, 2019

I created this PR based on the interest of the community (myself included).

Which community? Maybe because English is my second language I perceived your wording to be entitled to speak for the entire community.

People could complain as much as they want, that does not make them right, so what is your point?

I can't remember when was the last time I used normalize.css since Component-based systems bring closer the CSS spec/styles to the component instead of relying on CSS cascading so you can truly have a component agnostic of the environment.

If you don't want use normalize.css, then don't opt-in.

Right, that is why this plugin shouldn't be included by default since I would have to opt-out, no opt-in.

This conversation will get nowhere, you have personal and subjective opinions about it and I respect that.

But please do not speak out for the community since either you nor people who complained defines the community, and I wouldn't like you to assume that I am not part of the community nor I agree with you.

You are right, subjectively speaking; and I am right subjectively speaking. So better to leave this to the Core contributors to decide their best decision about this topic.

@mrchief
Copy link
Contributor Author

mrchief commented Jan 17, 2019

Which community?

Have you read #2273 at all? Do you not consider them to be part of the community?

Right, that is why this plugin shouldn't be included by default since I would have to opt-out, no opt-in.

There is no opting out. If you don't opt-in, you don't bring in anything (see my earlier comment which clarifies this). I guess that's where your confusion is.

You are right, subjectively speaking; and I am right subjectively speaking. So better to leave this to the Core contributors to decide their best decision about this topic.

100% agree!

@stale
Copy link

stale bot commented Feb 16, 2019

This pull request has been automatically marked as stale because it has not had any recent activity. It will be closed in 5 days if no further activity occurs.

@stale stale bot added the stale label Feb 16, 2019
@stale
Copy link

stale bot commented Feb 21, 2019

This pull request has been automatically closed because it has not had any recent activity. If you have a question or comment, please open a new issue. Thank you for your contribution!

@stale stale bot closed this Feb 21, 2019
@mrchief
Copy link
Contributor Author

mrchief commented Feb 28, 2019

@ianschmitz Please take a look. I added it as a separate file since:

  • The contents of post-processing-css do not exactly align with this. It feels unrelated to Post Processing of CSS as the imports happen before the build goes into post-processing stages.
  • "CSS Reset" can be a frequently asked question so having it in sidebar helps faster discovery.
  • And lastly, IMO, burying such an important topic under "Post Processing CSS" topic felt somewhat odd.

That said, I'm open to any suggestions regarding file name, placing etc. Also, please let me know if the content itself needs tweaking.

@iansu
Copy link
Contributor

iansu commented Apr 4, 2019

Please resolve merge conflicts.

@iansu iansu closed this Apr 5, 2019
@iansu iansu reopened this Apr 5, 2019
@iansu iansu merged commit 550274e into facebook:master Apr 5, 2019
@iansu
Copy link
Contributor

iansu commented Apr 5, 2019

Thanks!

@lock lock bot locked and limited conversation to collaborators Apr 10, 2019
@mrchief mrchief deleted the postcss-normalize branch October 3, 2019 02:03
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants