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

Lighthouse 'Avoids old CSS flexbox' audit fails #1658

Closed
ryansully opened this issue Feb 27, 2017 · 13 comments · Fixed by #1771
Closed

Lighthouse 'Avoids old CSS flexbox' audit fails #1658

ryansully opened this issue Feb 27, 2017 · 13 comments · Fixed by #1771
Milestone

Comments

@ryansully
Copy link
Contributor

Can you reproduce the problem with latest npm?

Yes.

Description

The current autoprefixer options prefixes flexbox with old CSS rules, which fails the following Lighthouse audit:
https://developers.google.com/web/tools/lighthouse/audits/old-flexbox

Expected behavior

Lighthouse audit 'Avoids old CSS flexbox' should pass.

Actual behavior

Lighthouse 'Avoids old CSS flexbox' audit fails.
Expanding 'More information' in the audit reveals that autoprefixer has added display: -webkit-box; and display: -ms-flexbox; to the compiled CSS.

Reproducible Demo

  1. create-react-app my-app && cd my-app
  2. Add display: flex; to body selector in src/index.css.
  3. npm start or npm run build and open local page in browser.
  4. Run Lighthouse audit.
@gaearon
Copy link
Contributor

gaearon commented Feb 27, 2017

We'll be enabling support for specifying the browser list in package.json.
(In fact maybe this already works?)

@ryansully
Copy link
Contributor Author

Ah, good to know. :)
I tried specifying browserslist in package.json, but it doesn't seem to be available yet. (Is this with regard to #892 and #1249?)

Specifying browserslist partially fixes the issue, but the autoprefixer option flexbox: 'no-2009' will also be needed to pass the audit. Will the upcoming support for specifying browser list in package.json also allow other autoprefixer options to be specfied, or just browserslist per autoprefixer v6.6? Otherwise the flexbox option might need to be added to webpack config in addition to configuring targeted browsers.

For anyone else with this issue, I managed to pass the audit with the following autoprefixer options (with inspiration from GoogleChrome/lighthouse#1710 (comment)):

browsers: [
  'last 2 versions',
  'not ie <= 10',
  'not ie_mob <= 10'
],
flexbox: 'no-2009'

@cr101
Copy link
Contributor

cr101 commented Mar 6, 2017

@gaearon Please consider adding postcss-flexbugs-fixes

We'll be enabling support for specifying the browser list in package.json.

Being able to add the browser support of Bootstrap 4 to my project would be very helpful:
browsers: [
'Chrome >= 35',
'Firefox >= 38',
'Edge >= 12',
'Explorer >= 10',
'iOS >= 8',
'Safari >= 8',
'Android 2.3',
'Android >= 4',
'Opera >= 12'
]

@gaearon
Copy link
Contributor

gaearon commented Mar 6, 2017

I tried specifying browserslist in package.json, but it doesn't seem to be available yet. [...] Specifying browserslist partially fixes the issue,

So how did you specify it? I’m not sure I understand this comment.

Please consider adding postcss-flexbugs-fixes

How do we know it is safe? It doesn't look like a very popular project.

Being able to add the browser support of Bootstrap 4 to my project would be very helpful

What is your use case in particular? Are you trying to reduce how many rules are generated (original motivation of this thread)? Or something else?

@cr101
Copy link
Contributor

cr101 commented Mar 7, 2017

How do we know it is safe? It doesn't look like a very popular project.

Bootstrap 4 have been successfully using postcss-flexbugs-fixes for quite a while in their build process to compile SASS to CSS. See here

What is your use case in particular? Are you trying to reduce how many rules are generated (original motivation of this thread)? Or something else?

Being able to customize the Bootstrap 4 SASS and then compile the SASS files to same browser support requirements as Bootstrap 4

@gaearon
Copy link
Contributor

gaearon commented Mar 7, 2017

Bootstrap 4 have been successfully using postcss-flexbugs-fixes for quite a while in their build process to compile SASS to CSS. See here

Interesting. Would you like to send a PR to us to enable this plugin?

Being able to customize the Bootstrap 4 SASS and then compile the SASS files to same browser support requirements as Bootstrap 4

Compile which Sass files? Your app's? Are you using this method or some other way of using Sass in Create React App? Why is it important for app files to match Bootstrap's targeted versions, rather than browsers of your users? I'm a bit confused by this.

@cr101
Copy link
Contributor

cr101 commented Mar 7, 2017

Would you like to send a PR to us to enable this plugin?

Yes

Compile which Sass files? Your app's?

Bootstrap 4 SASS along with my app's SASS files

Are you using this method or some other way of using Sass in Create React App?

Yes I am using CRA's method

Why is it important for app files to match Bootstrap's targeted versions, rather than browsers of your users? I'm a bit confused by this.

My project is based on Bootstrap 4 and because I am compiling Bootstrap's SASS along with my app's SASS files I need to match Bootstrap's targeted browser versions. For example, Bootstrap 4 now only supports IE >= 10 so it makes no sense to have IE9 CSS code in my app if Bootstrap 4 doesn't support it.
So I am looking for a way to configure the autoprefixer browsers without ejecting/forking.

@gaearon
Copy link
Contributor

gaearon commented Mar 7, 2017

OK, this makes sense. Happy to review a PR that enables the flexbox bug plugin.

I’d also be happy to review a PR that provides browserslist from package.json to Autoprefixer. I’m not sure how to make it work though—maybe need to pass options to the loader or something like this.

If you need any help let me know.

@cr101
Copy link
Contributor

cr101 commented Mar 7, 2017

I’d also be happy to review a PR that provides browserslist from package.json to Autoprefixer. I’m not sure how to make it work though—maybe need to pass options to the loader or something like this.
If you need any help let me know.

Yes, I need help please. Thank you

@gaearon
Copy link
Contributor

gaearon commented Mar 7, 2017

Is there anything in particular I can help you with?
We have initial instructions for contributing here.

You'll probably want to edit Webpack configs in packages/react-scripts/configs and pass postscript plugins (or options) there.

@cr101
Copy link
Contributor

cr101 commented Mar 7, 2017

You'll probably want to edit Webpack configs in packages/react-scripts/configs and pass postscript plugins (or options) there.

I'll give it a go

@gaearon
Copy link
Contributor

gaearon commented May 16, 2017

Fixed by #1771.

@gaearon gaearon closed this as completed May 16, 2017
@gaearon gaearon added this to the 0.10.0 milestone May 16, 2017
@gaearon
Copy link
Contributor

gaearon commented May 16, 2017

Please help beta test the new version that includes this change!
#2172

@lock lock bot locked and limited conversation to collaborators Jan 21, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants