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

Make UI respect safe space #152

Merged
merged 4 commits into from Mar 22, 2020

Conversation

@darcien
Copy link
Contributor

darcien commented Mar 22, 2020

A quick fix.
This only works for portrait orientation. In the future, we might need to handle other orientation.

image

This will fix #143

@SamMousa

This comment has been minimized.

Copy link
Member

SamMousa commented Mar 22, 2020

Let's do this for all sides: https://css-tricks.com/the-notch-and-css/

darcien added 2 commits Mar 22, 2020
@darcien darcien force-pushed the darcien:fix/safe-inset branch from 46ba69f to c9b49af Mar 22, 2020
@darcien darcien changed the title Make UI respect top safe space Make UI respect safe space Mar 22, 2020
@darcien

This comment has been minimized.

Copy link
Contributor Author

darcien commented Mar 22, 2020

Let's do this for all sides: css-tricks.com/the-notch-and-css

Done!

Copy link
Member

SamMousa left a comment

Please add all padding variables for safe space.

@darcien

This comment has been minimized.

Copy link
Contributor Author

darcien commented Mar 22, 2020

Please add all padding variables for safe space.

Padding doesn't work, that's why I used margin instead.

Here's with padding:
image

And the one with margin:
image

EDIT: The padding doesn't work on body element, changed the selector to :root to make it work with padding

@SamMousa SamMousa self-requested a review Mar 22, 2020
@SamMousa

This comment has been minimized.

Copy link
Member

SamMousa commented Mar 22, 2020

It seems there are theme variables available for this; also it should work out of the box. Could you have a look at this https://ionicframework.com/docs/theming/advanced?

If it doesn't work we might have just misconfigured something. We should be able to reconfigure, or worst case manually initialize those variables with the values you used in your css proposal.

@darcien

This comment has been minimized.

Copy link
Contributor Author

darcien commented Mar 22, 2020

It seems there are theme variables available for this; also it should work out of the box. Could you have a look at this https://ionicframework.com/docs/theming/advanced?

I looked into it, but seems like the value provided by ionic is the same with the env variable, i.e. env(safe-area-inset-top) === var(--ion-safe-area-top).

Which means it's already initialized but not used.

I'm not really familiar with the Ionic environment, but I think this should be handled by ionic.

There's a similar issue on capacitor repo:
image
ionic-team/capacitor#777 (comment)

But I'm not sure about injecting some stylesheet from the network, especially when we might need offline support later on.

@SamMousa

This comment has been minimized.

Copy link
Member

SamMousa commented Mar 22, 2020

We should have that locally available in node modules folder.

@darcien

This comment has been minimized.

Copy link
Contributor Author

darcien commented Mar 22, 2020

Ah right, I updated the PR to use those variables, could you please check again, thanks.

Copy link
Member

SamMousa left a comment

LGTM. @darcien seeing as this is not a blocker I'll leave this open for one of our react adepts to approve / merge.

Thanks for your contribution!

@SamMousa SamMousa requested a review from hspinks Mar 22, 2020
@sstur
sstur approved these changes Mar 22, 2020
Copy link
Collaborator

sstur left a comment

This looks solid from my perspective. I think it's good to land. Thanks @darcien!

@SamMousa SamMousa merged commit 987c068 into WorldHealthOrganization:master Mar 22, 2020
5 checks passed
5 checks passed
test (ubuntu-latest)
Details
test (macos-latest)
Details
build-android-debug (ubuntu-latest)
Details
build-android-debug (macos-latest)
Details
build-ios-debug
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

4 participants
You can’t perform that action at this time.