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

Add support for mobile safe areas #436

Draft
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

HiDeoo
Copy link
Member

@HiDeoo HiDeoo commented Jul 31, 2023

What kind of changes does this PR include?

  • Changes to Starlight code

Description

This pull request is an attempt to add support for mobile safe areas to Starlight. It is a draft for two main reasons:

  • I think it would be nice to have more people test it as I don't have that many devices with notches to test it on, and it's fairly easy to forget some edge cases.
  • As mentioned in Support safe area on mobile devices #433, we may want to tweak some breakpoints.

To talk a bit about the changes, the syntax to support mobile safe areas is pretty verbose and it's even more verbose when we use some logical CSS properties as there is no equivalent for the safe area inset properties, they are only physical.

When dealing with safe areas I always refer to this blog post that was released by the Webkit team when the iPhone X with its notch was released which I think does a good job at explaining how it works so sharing it here in case it can be useful and I also included it in the changeset.

Here are some screenshots from a device with a notch, starting with the home page:

Before After
home page before home page after

A doc page:

Before After
doc page before doc page after

A doc page with the toc opened:

Before After
doc page with toc before doc page with toc after

Even if we somehow forget some edge cases, I think it'll be nice to have support for this in Starlight and we can always fix the edge cases when we find them.

@changeset-bot
Copy link

changeset-bot bot commented Jul 31, 2023

🦋 Changeset detected

Latest commit: 4fca751

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@astrojs/starlight Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@netlify
Copy link

netlify bot commented Jul 31, 2023

Deploy Preview for astro-starlight ready!

Name Link
🔨 Latest commit 4fca751
🔍 Latest deploy log https://app.netlify.com/sites/astro-starlight/deploys/64ce57b00641c50007bbf2c3
😎 Deploy Preview https://deploy-preview-436--astro-starlight.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@github-actions github-actions bot added the 🌟 core Changes to Starlight’s main package label Jul 31, 2023
@HiDeoo HiDeoo marked this pull request as draft July 31, 2023 07:00
@fflaten
Copy link
Contributor

fflaten commented Jul 31, 2023

Thank you for looking into this so quickly!

If only the device emulator in Chromium included option for safe area. So painful to use physical devices for testing this. Please let me know if there's an option I haven't found yet :)

@HiDeoo
Copy link
Member Author

HiDeoo commented Jul 31, 2023

If only the device emulator in Chromium included option for safe area. So painful to use physical devices for testing this. Please let me know if there's an option I haven't found yet :)

Yeah, this is a pain. The only other "option" I have found for iOS is having a mac, installing the huge bundle that is Xcode just to get the iOS emulators, cry at every update that it takes forever to update, run manually an emulator for a device with a notch, start Safari on the simulator and visit the webpage.

@delucis
Copy link
Member

delucis commented Jul 31, 2023

Thanks for tackling this @HiDeoo! Yeah, I also have no devices with a notch and will have to open up the iOS simulator. Pretty gross that this requires SO MUCH CSS for their dumb design flaw 😅

Haven’t had a chance to dive in to the code in detail yet, so this may not work, but one trick I sometimes reach for when media queries are getting out of hand like this is to see if there’s a way to move things into a reusable custom property, e.g. this kind of concept:

:root {
  /* default value */
  --some-prop: 1rem;
}

@media (prefers-notchy-screen: true) {
  :root {
    /* value when condition is met */
    --some-prop: 1.5rem;
  }
}

Then in most styles you just use your value without worrying about media queries. Maybe that could help conciseness here too.

@HiDeoo
Copy link
Member Author

HiDeoo commented Jul 31, 2023

Excellent tip, I'll make a second pass tomorrow now that I have identified most of areas requiring changes and see if we can apply this in some of these cases.

@HiDeoo
Copy link
Member Author

HiDeoo commented Aug 1, 2023

So I made a second pass at this and tried to apply the trick you mentioned.

It works relatively well for elements that span the entire width of the screen so I managed to reduce the number of changes for the header for example. Altho, I don't think it scales well for other elements where we need to know if it's on the left on the right based on the direction, so rely on :global([dir='ltr']) and :global([dir='rtl']) to specifically adjust either the padding-left or padding-right values with either env(safe-area-inset-left) or env(safe-area-inset-right). This would be so much easier with logical values.

I also found a part of the UI that I missed in my first pass (I updated the screenshot in the initial post), so I think the overall number of changes is pretty much the same 😢

I may try a third pass in a few days as these safe areas have a tendency to mess with my brain 😅

@HiDeoo
Copy link
Member Author

HiDeoo commented Aug 5, 2023

Third pass done 😅

I managed to remove most @supports (padding: max(0px)) directives and only keep a single one in packages/starlight/style/props.css. It only hit me today that I could move some media queries to this file nested in this directive even tho I was already doing that in some components ^^ I guess the step back helped.

All references to env(safe-area-inset-*) and max() are properly located in this file and in this @supports directive so we should not hit any issues with some older browsers not supporting those.

We still have to rely unfortunately a lot more than before on :global([dir='rtl']) at various places and this is one thing I don't know how to avoid.

I also refactored some previous changes I introduced to reduce the number of changes overall and fix a padding that I previously forgot to adjust.

I think at this point I'll open this for review as I think I need some other eyes to look at this ^^ I'll be happy tho to fix any issue or places that I may have missed or try other approaches if some of you have other ideas.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🌟 core Changes to Starlight’s main package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support safe area on mobile devices
3 participants