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

Update browserslist in package.json #1002

Merged
merged 2 commits into from
Sep 21, 2018
Merged

Conversation

hannalaakso
Copy link
Member

@hannalaakso hannalaakso commented Sep 18, 2018

This makes browserslist specific to the browsers we support. Once this has been approved, we can raise a PR to make the same change in GOV.UK Design System.

Tested /components/all page in (if only we had cross browser visual tests 🤓):
✅ Chrome 68, 69
✅ Firefox 61, 62 (with inverted colours)
✅ Safari 9.1, 10.1, 11.1
✅ Edge 16, 17
✅ iOS Safari 9
✅ Android Chrome 63
✅ Samsung Internet 6.4.0.5
✅ Internet Explorer 8
✅ Internet Explorer 9
✅ Internet Explorer 10
✅ Internet Explorer 11

Issue originally discussed here.

Syntax follows browserslist docs.

Fixes #952

https://trello.com/c/dvfqTy9D/1360-update-browser-list-in-packagejson

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1002 September 18, 2018 16:27 Inactive
@kr8n3r
Copy link

kr8n3r commented Sep 19, 2018

-webkit prefixes have been removed from built files. is that expected?

screen shot 2018-09-19 at 08 45 35

screen shot 2018-09-19 at 08 45 23

@hannalaakso
Copy link
Member Author

hannalaakso commented Sep 19, 2018

@igloosi This is inline with the specified browserslist, those prefixes cater for Safari 5 and 6 which we don't officially support.

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1002 September 19, 2018 11:14 Inactive
@hannalaakso
Copy link
Member Author

It sounds like we need a slightly more holistic approach: we might not want to remove prefixes for certain legacy browsers even if our browser matrix doesn't officially include them.

I've added >0.10% to browserslist which means that we add prefixes for these browsers (includes Opera and UC which should be okay), in addition to our core ones.

I've also changed support for desktop Safari to go down to 6 as we know older Safaris can be buggy and some extra ❤️ for them wouldn't hurt (moved iOS down to 8 as well).

Tested again in:
✅ Chrome 68, 69
✅ Firefox 61, 62 (with inverted colours)
✅ Safari 6, 7.1, 9.1, 10.1, 11.1
✅ Edge 16, 17
✅ iOS Safari 9
✅ Android Chrome 63
✅ Samsung Internet 6.4.0.5
✅ Internet Explorer 8
✅ Internet Explorer 9
✅ Internet Explorer 10
✅ Internet Explorer 11

package.json Outdated
"ie 8",
"ie 9",
"iOS 9"
">0.10%",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe my maths is wonky but should this be related to the 95% in the service manual?

https://www.gov.uk/service-manual/technology/designing-for-different-browsers-and-devices#browsers-to-test-in

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So…

I think the 'browsers to test in' matrix in the Service Manual should be treated exactly as that – the browsers that we can reasonably ask service teams to test with.

We then have our own browser support matrix, which is a different list that happens to look a lot like the 'browsers to test in' matrix. At the minute the only difference is that we keep IE8-10 around because we believe there are service teams that still need to support those browsers, and we want those service teams to be able to use GOV.UK Frontend.

Finally, there's this config which defines which browsers we want to auto-prefix for. I don't think it's a given that that list correlates to either of the previous two lists of browsers – for example, whilst we might not test or choose to fix bugs in e.g. Chrome 65, it still gets 0.44% of the traffic, which for GOV.UK would represent around ~200,000 users.

Assuming that auto-prefixing will provide a better experience for those users, and that the impact on the filesize of the generated CSS is minimal, I think it makes sense to continue to autoprefix for those browsers, even if they fall outside of the 95% figure we use to define the testing matrix? We still need to draw the line somewhere, and that figure is going to be arbitrary, but 0.1% seems like a good place to start?

Thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I agree only provided that the extra CSS does not have a reasonable fallback.

I've not seen good evidence that by removing these attributes these browsers would receive a broken experience.

For example with flexbox we're talking, 0.01% Safari requests that would get display: inline-block

Your example seems hypothetical, since newer versions of Chrome do not even have prefixing as prefixing has been discontinued.

That said, don't think this is worth blocking anything over, so 👍 to whatever you choose to do.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All good stuff, I'm really just trying to think out loud here more than anything – trying to make sure that we're not being overly selective.

TBH, I wasn't aware that vendor-prefixing had 'gone away' as much as you (and MDN) suggest, so maybe this is a bit academic…

@hannalaakso hannalaakso changed the title Update browserslist in package.json WIP - Update browserslist in package.json Sep 19, 2018
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1002 September 20, 2018 18:18 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1002 September 20, 2018 18:20 Inactive
@hannalaakso
Copy link
Member Author

hannalaakso commented Sep 20, 2018

So sticking to 0,1% and with Safari and iOS at >= 9, the prefixes removed are from our current code will be:

@hannalaakso hannalaakso changed the title WIP - Update browserslist in package.json Update browserslist in package.json Sep 20, 2018
"ie 8",
"ie 9",
"iOS 9"
">0.1%",

This comment was marked as resolved.

Copy link
Contributor

@NickColley NickColley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hannalaakso
Copy link
Member Author

I did a bit of investigation and for reference adding the results here. Going with 0,5% support would affect the footer in older versions on desktop Safari as some of the box and flex prefixes for -webkit would get dropped.

Safari 8 with 0,5% support (with dropped prefixes)
safari 8 with 95 support
Safari 8 with 0,1% support (this is what we currently have)
safari 8 with 99 support
So agree with @NickColley, 0,1% seems like a good balance for now and we can always tweak it later.

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1002 September 21, 2018 13:34 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1002 September 21, 2018 13:38 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1002 September 21, 2018 13:40 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1002 September 21, 2018 13:40 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1002 September 21, 2018 13:48 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1002 September 21, 2018 13:53 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1002 September 21, 2018 14:12 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1002 September 21, 2018 14:24 Inactive
This makes `browserslist` specific to the browsers we support.

Follows docs here:
https://github.com/browserslist/browserslist#full-list
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

Successfully merging this pull request may close these issues.

5 participants