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

Enable grid prefixing via autoprefixer options object #4756

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
6 participants
@BuckyMaler

BuckyMaler commented Jul 8, 2018

To verify grid prefixing I swapped out flexbox for grid in App.css.

yarn build with no autoprefixer changes:

grid-prefixing-disabled

yarn build with autoprefixer grid option enabled:

grid-prefixing-enabled

This fixes #4017

@facebook-github-bot

This comment has been minimized.

Show comment
Hide comment
@facebook-github-bot

facebook-github-bot Jul 8, 2018

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 cla@fb.com. Thanks!

facebook-github-bot commented Jul 8, 2018

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 cla@fb.com. Thanks!

@facebook-github-bot

This comment has been minimized.

Show comment
Hide comment
@facebook-github-bot

facebook-github-bot Jul 8, 2018

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

facebook-github-bot commented Jul 8, 2018

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

@bugzpodder bugzpodder added this to the 2.0.0 milestone Jul 15, 2018

@Timer Timer closed this Sep 26, 2018

@Timer Timer reopened this Sep 26, 2018

@Timer Timer modified the milestones: 2.0.x, 2.0.0-final, 2.0.0 Sep 26, 2018

@Timer

This comment has been minimized.

Show comment
Hide comment
@Timer

Timer Sep 27, 2018

Collaborator

@ai does this have negative implications other than it doesn't provide full support in older browsers?

i.e. will this not break support in browsers that support grid? is it safe to turn on for everybody?

Collaborator

Timer commented Sep 27, 2018

@ai does this have negative implications other than it doesn't provide full support in older browsers?

i.e. will this not break support in browsers that support grid? is it safe to turn on for everybody?

@Timer Timer changed the base branch from next to master Sep 27, 2018

@ai

This comment has been minimized.

Show comment
Hide comment
@ai

ai Sep 27, 2018

Contributor

This option should not be enabled by default. Developer must understand limits before using it.

I suggest to add postcss.config.js support. In this case developers will be able to redefine CRA PostCSS’s plugins and plugin's options. It will be there best way to enable Grid.

Contributor

ai commented Sep 27, 2018

This option should not be enabled by default. Developer must understand limits before using it.

I suggest to add postcss.config.js support. In this case developers will be able to redefine CRA PostCSS’s plugins and plugin's options. It will be there best way to enable Grid.

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Sep 27, 2018

Member

I think it's reasonable to leave it on here. In our case, the user doesn't set up autoprefixing themselves. If they were, it makes sense for the option to be off because Autoprefixer is expected to be correct.

However in our case autoprefixing happens "behind the curtains". So you don't expect it to work in 100% of cases because you don't think about it that much in the first place. Therefore, I think it makes sense that we do best effort to support IE here. Worst case — your layout is just as broken as it would be if we left it off. You'd have to test it in IE regardless.

I agree it would be bad for Autoprefixer to make it on by default because it would hurt its brand as a "correct" tool. But in our case I think it's okay because I can't see the harmful scenario it attempts to prevent. We can add it to User Guide in any case.

Member

gaearon commented Sep 27, 2018

I think it's reasonable to leave it on here. In our case, the user doesn't set up autoprefixing themselves. If they were, it makes sense for the option to be off because Autoprefixer is expected to be correct.

However in our case autoprefixing happens "behind the curtains". So you don't expect it to work in 100% of cases because you don't think about it that much in the first place. Therefore, I think it makes sense that we do best effort to support IE here. Worst case — your layout is just as broken as it would be if we left it off. You'd have to test it in IE regardless.

I agree it would be bad for Autoprefixer to make it on by default because it would hurt its brand as a "correct" tool. But in our case I think it's okay because I can't see the harmful scenario it attempts to prevent. We can add it to User Guide in any case.

@ai

This comment has been minimized.

Show comment
Hide comment
@ai

ai Sep 27, 2018

Contributor

It could a another solution. We can add /* autoprefixer grid: on */ control comments.

Contributor

ai commented Sep 27, 2018

It could a another solution. We can add /* autoprefixer grid: on */ control comments.

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Sep 27, 2018

Member

Interesting. That would be good I think. We could document that and leave it off then.

Member

gaearon commented Sep 27, 2018

Interesting. That would be good I think. We could document that and leave it off then.

@Timer

This comment has been minimized.

Show comment
Hide comment
@Timer

Timer Sep 27, 2018

Collaborator

How would this work when you import CSS from a package/node_modules?

Collaborator

Timer commented Sep 27, 2018

How would this work when you import CSS from a package/node_modules?

@ai

This comment has been minimized.

Show comment
Hide comment
@ai

ai Sep 27, 2018

Contributor

@Timer it could not automagically apply to any CSS. CSS should be written with limits in mind.

Here is a good guide about the limits:
https://css-tricks.com/css-grid-in-ie-css-grid-and-the-new-autoprefixer/

This is why calling grid: true on some node_modules code is not the best idea.

Also, it is a popular way to exclude node_modules from Babel and PostCSS.

But I agree that we may have cases when we need to run PostCSS and Babel on npm packages. But I do not see a way to resolve it without adding postcss.config.js support.

Contributor

ai commented Sep 27, 2018

@Timer it could not automagically apply to any CSS. CSS should be written with limits in mind.

Here is a good guide about the limits:
https://css-tricks.com/css-grid-in-ie-css-grid-and-the-new-autoprefixer/

This is why calling grid: true on some node_modules code is not the best idea.

Also, it is a popular way to exclude node_modules from Babel and PostCSS.

But I agree that we may have cases when we need to run PostCSS and Babel on npm packages. But I do not see a way to resolve it without adding postcss.config.js support.

@Timer

This comment has been minimized.

Show comment
Hide comment
@Timer

Timer Sep 27, 2018

Collaborator

Will grid prefixes get stripped if imported when we have the flag set to false? This sounds like unacceptable behavior to me for node_modules.

Is there a way to say "don't strip grid prefixes, but don't add them either"? Or is that the default behavior?

Collaborator

Timer commented Sep 27, 2018

Will grid prefixes get stripped if imported when we have the flag set to false? This sounds like unacceptable behavior to me for node_modules.

Is there a way to say "don't strip grid prefixes, but don't add them either"? Or is that the default behavior?

@ai

This comment has been minimized.

Show comment
Hide comment
@ai

ai Sep 27, 2018

Contributor

Nope. Prefixes will not be stripped, if IE 11 is in Browserslist.

Contributor

ai commented Sep 27, 2018

Nope. Prefixes will not be stripped, if IE 11 is in Browserslist.

@Timer

This comment has been minimized.

Show comment
Hide comment
@Timer

Timer Sep 28, 2018

Collaborator

OK, we won't merge this and will document /* autoprefixer grid: on */ instead. Is there an estimated timeline on this feature?

Collaborator

Timer commented Sep 28, 2018

OK, we won't merge this and will document /* autoprefixer grid: on */ instead. Is there an estimated timeline on this feature?

@Timer Timer closed this Sep 28, 2018

@ai

This comment has been minimized.

Show comment
Hide comment
@ai

ai Sep 28, 2018

Contributor

Yeap. I added the task to our Cult of Martians https://cultofmartians.com/tasks/autoprefixer-grid-comment.html

I think it will be done in next week (maybe two).

Contributor

ai commented Sep 28, 2018

Yeap. I added the task to our Cult of Martians https://cultofmartians.com/tasks/autoprefixer-grid-comment.html

I think it will be done in next week (maybe two).

@ai

This comment has been minimized.

Show comment
Hide comment
@ai

ai Oct 12, 2018

Contributor

/* autoprefixer grid: on */ support was added to Autoprefixer postcss/autoprefixer#1138

Contributor

ai commented Oct 12, 2018

/* autoprefixer grid: on */ support was added to Autoprefixer postcss/autoprefixer#1138

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment