Fixed respecting safe area around the ios navigation bar#739
Fixed respecting safe area around the ios navigation bar#739CodeWithCJ merged 2 commits intoCodeWithCJ:mainfrom
Conversation
Summary of ChangesHello @mr-woodapple, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request resolves a user interface issue on iOS devices where the native bottom bar would overlap with the application's mobile navigation when the website is added to the home screen as a web-app. By integrating safe area handling, the layout now correctly adjusts, providing a seamless and accessible user experience on iOS. Highlights
Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
The pull request successfully addresses the iOS safe area issue by introducing the env(safe-area-inset-bottom) CSS property. However, the refactoring of the mobile navigation component introduced a few layout inconsistencies and a potential bug where the grid column count might be incorrectly hardcoded, overriding dynamic behavior. Additionally, a minor stylistic improvement for file formatting is suggested.
| </Button> | ||
| ))} | ||
| <nav className="apple-safe-area sm:hidden fixed bottom-0 z-50 w-full bg-background border-t"> | ||
| <div className={`h-14 grid grid-cols-4 items-center justify-items-center ${mobileGridClass}`}> |
There was a problem hiding this comment.
The refactoring of the mobile navigation div introduced a few issues:
- Missing
gap-1: Thegap-1class, which provided spacing between navigation items, was removed. This might lead to a cramped layout. - Hardcoded
grid-cols-4: Thegrid-cols-4class is now hardcoded, which will override the dynamicmobileGridClassif the number of available mobile tabs is not exactly four. This can lead to an incorrect grid layout when the tab count varies. - Missing
px-2: The horizontal padding (px-2) was removed from the navigation container. This could cause the navigation buttons to touch the screen edges on mobile devices, impacting the user experience.
The suggested change reintroduces the gap-1 and px-2 classes and removes the hardcoded grid-cols-4 to ensure the layout remains dynamic and consistent.
| <div className={`h-14 grid grid-cols-4 items-center justify-items-center ${mobileGridClass}`}> | |
| <div className={`h-14 grid gap-1 items-center justify-items-center px-2 ${mobileGridClass}`}> |
There was a problem hiding this comment.
Not true, because the introduced items-center justify-items-center classes will distribute the available icons across the screen. We'll probably have other issues before that's relevant.
|
Wouldn't this leave a gap that's too big on other smartphones? On my Android the current gap looks perfect |
It shouldn't, no. The way these environement variables work is that they are set by the browser / operating system. So on an Android device, it'll be resolved to 0px (if it is resolved at all). |
|
Okay, learned something new. Looks good👍 |
|
@mr-woodapple Could you clarify the usage. I checked exiting one both on iPhone 13 mini and 15 pro max. I am not sure what does the significance of that space will benefit the bottom bar apart from leaving some space. I personally like the way it is now as its small and doesn't occupy much space. so we get to see more info from the app. |
|
@CodeWithCJ Sure, when I tap on the most right and left elements in particular, the OS often interprets this as me trying to navigate between apps. Also, I find it annoying that the iOS nav bar handle activates when I tap on elements in the bottom navigation. Not a huge issue, but generally good practice to respect these safe areas. And the space that's "wasted" by respecting these safe areas is negligible (I think ^^). |
|
@mr-woodapple could you run below comments to fix format & lint issues and re-submit the PR |
|
I couldn't re-open it |
|
Sure, can do. But running |
|
Yes of course. It's normal to not use windows line endings. We have a prettier config that does exactly that. Feel free to add a .gitatrributes If thats helps. But I thought git does that automatically. I don't have experience in developing with windows though. I'm not a big fan of adding it to the workflow. The main benefit is that the diffs are smaller. If you add it to the workflow there has to be a new commit for every pr that only changes the format. I added a pre commit hook to the root of the project which you can install with npm Install that run before every commit locally and format it. |
3 participants
Currently, if you saved the website as a bookmark to your iOS home screen & enabled the option "Open as Web-App" (or something similar like it, I just translated the German text), the iOS native bottom bar nav indicator (I don't know how to properly call that thing) would be in the way of the Sparky Fitness bottom navigation buttons.
I added a div to respect that bottom bar, see the images below.
Before (left) and After (right)