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

Accommodate camera notches on new devices (iPhone X, Google Pixel 3 etc) #1176

Merged
merged 1 commit into from
Mar 1, 2019

Conversation

colinrotherham
Copy link
Contributor

@colinrotherham colinrotherham commented Feb 6, 2019

Noticed this a while ago, thought I'd contribute a new feature for it.

GOV.UK Frontend doesn't support the dreaded "notch" (or Display Cutouts as they're known) on various new devices. See the unused white area on the left hand side here:

notch safe area

I did a quick check and we now have:

  • iPhone X
  • iPhone Xs
  • Google Pixel 3
  • OnePlus 6
  • Huawei P20
  • LG G7
  • Nokia 6.1

The safe-area-inset-* variables were added in iOS 11, and recently Android P has added support via Chrome for Android 69: https://www.chromestatus.com/feature/5710044637167616

Firefox just added support in v65:
https://bugzilla.mozilla.org/show_bug.cgi?id=1462233

Before

before

After

after

This pull request adds the initial support to patch .govuk-width-container

@NickColley
Copy link
Contributor

NickColley commented Feb 6, 2019

Something to think about:

The env() function shipped in iOS 11 with the name constant(). Beginning with Safari Technology Preview 41 and the iOS 11.2 beta, constant() has been removed and replaced with env(). You can use the CSS fallback mechanism to support both versions, if necessary, but should prefer env() going forward.

Also worth checking the @supports query

It is important to use @supports to feature-detect min and max, because they are not supported everywhere, and due to CSS’s treatment of invalid variables, to not specify a variable inside your @supports query.

@colinrotherham
Copy link
Contributor Author

colinrotherham commented Feb 6, 2019

@NickColley Yeah Apple quickly backtracked on constant() as its quite rightly a variable! I chose not to support iOS 11.0 and 11.1 and only target the standard spec instead.

Thanks for the comment about @supports and variables though, I was taking my guidance from the WebKit article which originally suggested doing that (they've added yellow update boxes now, which clears things up).

I've added a new version with a simpler test:

/* Add safe area (e.g. iPhone X, Google Pixel 3) */
@supports (margin: unquote("max(0px)")) {
  /* Use safe area or mobile margin, whichever is larger */
  /* Escaped due to Sass max() vs. CSS native max() */
  margin-right: unquote("max(#{$govuk-gutter-half}, env(safe-area-inset-right))");
  margin-left: unquote("max(#{$govuk-gutter-half}, env(safe-area-inset-left))");
}

Since Sass already has a max() we had to do some horrible escaping with unquote() to avoid it, so the native max() survives into the output CSS without any warnings.

This example outputs the following CSS:

@supports (margin: max(0px)) {
  .govuk-width-container {
    margin-right: max(15px, env(safe-area-inset-right));
    margin-left: max(15px, env(safe-area-inset-left));
  }
}

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1176 February 7, 2019 08:47 Inactive
@colinrotherham
Copy link
Contributor Author

colinrotherham commented Feb 7, 2019

If anyone has a phone with a "notch" it's deployed here to test out:
https://govuk-frontend-review-pr-1176.herokuapp.com/examples/template-default

Whilst browser support has arrived, I'm not sure if the OS always sets the safe-area-inset-* variables for it to work across all notched devices. It'll likely get better over time though.

Should look like the After screenshot above.

Thanks

@paulwaitehomeoffice
Copy link

Looks good to me (iPhone XS Max, iOS 12.1.2).

I’ve got an old iPhone X at home which I can probably restore to an earlier version of iOS tonight, if it'd help to test there too.

@36degrees
Copy link
Contributor

Over the next few weeks we're planning to add a number of example pages to the review app in preparation for an audit against WCAG 2.1.

If it's OK with you, I think it'd be worth waiting until we've got those example pages added, as I think they'll be an ideal way to test that this going to do what we want in different contexts.

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1176 February 8, 2019 11:35 Inactive
@colinrotherham
Copy link
Contributor Author

@36degrees Sounds good to me. I'll keep this rebased with master in the meantime.

@colinrotherham
Copy link
Contributor Author

Rebased with latest master.

Good to see it in action with the new full page example @NickColley.

start page

@NickColley
Copy link
Contributor

@colinrotherham exciting, we will have a lot more examples in the review app by the end of the week, then we can merge this in once we're happy :)

@NickColley
Copy link
Contributor

NickColley commented Feb 20, 2019

We've updated the review application to include pages that cover as much as GOV.UK Frontend as possible so I think this is ready for review.

If you could bring this branch up to date with the current repository (master), we can give it another look. 😄

@colinrotherham
Copy link
Contributor Author

@NickColley Done 😊

@NickColley NickColley added this to the 2.8.0 milestone Feb 25, 2019
@NickColley
Copy link
Contributor

NickColley commented Feb 26, 2019

I'm wondering if it's a bit too tight at the moment? I've found one example where number lists can be blocked slightly, and it optically feels really close.

https://govuk-frontend-review.herokuapp.com/full-page-examples/announcements
screen shot 2019-02-26 at 17 29 00

Potentially we might want to keep the gutters the same width but add the additional spacing on top for these notches.

This would make it render nicely fullscreen while keeping the layout the same.

For example on http://localhost:3000/full-page-examples/how-do-you-want-to-sign-in

I tried something like

@supports (margin: calc(env(safe-area-inset-left))) {
    margin-right: calc(#{govuk-spacing(3)} + env(safe-area-inset-right));
    margin-left: calc(#{govuk-spacing(3)} + env(safe-area-inset-left));
}

Note here that I'm not using the gutter units since the notch gives a much bigger optical gap between the two, so I'm using a custom spacing value that 'feels' right.

There's an article that talks about this optical spacing and sizing versus reality: https://medium.com/@lukejones/optical-adjustment-b55492a1165c

screen shot 2019-02-26 at 18 23 08

This then gives some more room for things like numbered lists

screen shot 2019-02-26 at 18 25 13

Pinging @dashouse to see what he thinks on this one.

@colinrotherham
Copy link
Contributor Author

@NickColley These new example pages are so much better. Hadn't realised the numbered lists hang slightly off to the left—definitely looks too tight now! Should they hang off though?

I've chosen to use $govuk-gutter-half instead? Feels better (same size too?)

// Add safe area (e.g. iPhone X, Google Pixel 3)
@supports (padding: unquote("max(calc(0px))")) {
  // Use safe area or default padding, whichever is larger
  // Escaped due to Sass max() vs. CSS native max()
  padding-right: unquote("max(#{govuk-spacing(3)}, calc(#{$govuk-gutter-half} + env(safe-area-inset-right)))");
  padding-left: unquote("max(#{govuk-spacing(3)}, calc(#{$govuk-gutter-half} + env(safe-area-inset-left)))");
}

I've pushed up what you see above (added both max/calc into @supports but made sure the variable name isn't included) which you spotted in #1176 (comment).

Same logic applied to the skip link should it be visible on other notched devices in future.

It's looking good! Thanks

@colinrotherham
Copy link
Contributor Author

Can we trigger a new Heroku build too please?

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1176 February 28, 2019 13:09 Inactive
@hannalaakso
Copy link
Member

@colinrotherham Done 👍

@dashouse
Copy link

Sorry I'm late to comment. The <ol> style doesn't look great to me in general (nothing to do with this PR).

However, without doing much of a deep dive the only way I can think to fix this would be to change the padding-left to a number that would stop the numeric bullets breaking out of the layout (when two digits).

I was playing with list-style-position: inside; and losing the padding all together but in that scenario the list content wraps under the number despite it fixing the overflow.

@NickColley
Copy link
Contributor

@dashouse sounds good to raise as a separate issue regarding those specific list styles then...

Could you review the spacing that Colin has proposed originally and give any feedback on what you think we should be aiming for?

@dashouse
Copy link

dashouse commented Feb 28, 2019

I think the changes that were made or this reason look really good (based on the screen shots). So I'd go with the bigger gutter as suggested anyway and we'll look at the <ol> style separately too.

@NickColley
Copy link
Contributor

@dashouse sweet thanks for that, feel free to suggest any specific spacing as I kinda just fudged it and went with what felt OK.

@dashouse
Copy link

Really happy with it feel wise in browser stack, can't really see the outputted units but for something like this I think it's as much about feel as anything...

@colinrotherham
Copy link
Contributor Author

@dashouse @NickColley That's fab, glad you like it.

The current pull request includes Nick's suggestion (albeit using $govuk-gutter-half vs. govuk-spacing(3)) but let me know if you need anything else 😊

src/objects/_width-container.scss Outdated Show resolved Hide resolved
src/objects/_width-container.scss Outdated Show resolved Hide resolved
src/components/skip-link/_skip-link.scss Outdated Show resolved Hide resolved
Signed-off-by: Colin Rotherham <work@colinr.com>
@colinrotherham
Copy link
Contributor Author

I've resolved all the review comments.

They're on outdated diffs so add more if you need any other tweaks!

Thanks

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.

Thanks for making adjustments based on the feedback, I've tested this in browser stack and I'm happy with this.

This is a really neat polish feature and I love that you thought about devices in the future too. 👍

Will need a second approval, preferably from @dashouse

@dashouse dashouse self-requested a review March 1, 2019 14:52
Copy link

@dashouse dashouse left a comment

Choose a reason for hiding this comment

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

Looks great to me 👍 💯

@36degrees
Copy link
Contributor

Great stuff, thanks @colinrotherham 👍

@NickColley NickColley merged commit 9651423 into alphagov:master Mar 1, 2019
@NickColley
Copy link
Contributor

NickColley commented Mar 1, 2019

Will try to release this next week 👍 https://github.com/alphagov/govuk-frontend/milestone/18

@colinrotherham colinrotherham deleted the notch-support branch March 1, 2019 16:44
@36degrees 36degrees mentioned this pull request Mar 5, 2019
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.

7 participants