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

[Fix #2719] App does not fit on iPhone X screen #2856

Merged
merged 1 commit into from
Feb 12, 2018

Conversation

foopang
Copy link
Contributor

@foopang foopang commented Dec 30, 2017

fixes #2719

I guess we will need another splash image for iPhone X portrait (1125px x 2436px) 🙂

screen shot 2017-12-30 at 9 42 17 pm

screen shot 2017-12-30 at 9 39 35 pm

status: ready

@foopang foopang changed the title [Fix #2719] iPhone X screen left empty [Fix #2719] iPhone X screen left empty space Dec 30, 2017
@foopang foopang changed the title [Fix #2719] iPhone X screen left empty space [Fix #2719] iPhone X screen left with empty space Dec 30, 2017
@foopang foopang force-pushed the fix/iphone-x-screen branch 2 times, most recently from 3a7e350 to ff93b57 Compare December 31, 2017 02:06
@oskarth oskarth added this to INBOX in Pipeline for QA via automation Jan 2, 2018
@oskarth oskarth added the blocked label Jan 2, 2018
@oskarth oskarth moved this from INBOX to DEV TODO (Review/Merge) in Pipeline for QA Jan 2, 2018
@oskarth oskarth moved this from DEV TODO (Review/Merge) to PENDING CONTRIBUTOR in Pipeline for QA Jan 2, 2018
@denis-sharypin denis-sharypin self-assigned this Jan 2, 2018
@denis-sharypin
Copy link

Hey, @foopang thanks for a contribution. Could you, please double check tab bar and top bar heights? Use my design as a reference.
iphonex

@foopang
Copy link
Contributor Author

foopang commented Jan 2, 2018

Hi @denis-sharypin please see if this looks good now.

screen shot 2018-01-03 at 6 29 25 am

:choose-recipient) common-styles/color-blue4
(:accounts :login) common-styles/color-blue2
:transparent)
:margin-top -20}))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems that the new safe-area-view RN component doesn't allow us to set the padding size for top and bottom, so I come up with this kinda odd :margin-top -20 to do the trick.

@denis-sharypin
Copy link

@foopang Thanks! All good from a design side

@denis-sharypin
Copy link

@foopang Please, add also a launch screen
launchscreen1125x2436

@foopang
Copy link
Contributor Author

foopang commented Jan 3, 2018

@denis-sharypin the launch screen has been updated!

screen shot 2018-01-03 at 8 37 05 pm

@oskarth oskarth moved this from CONTRIBUTOR to REVIEW in Pipeline for QA Jan 8, 2018
@oskarth
Copy link
Contributor

oskarth commented Jan 8, 2018

@foopang Sorry, got dropped out of our pipeline. Moving to REVIEW now as design seems to have been sorted. Thanks!

[(if android? menu-context view) common-styles/flex
[view common-styles/flex
[(if iphone-x? safe-area-view view) (merge common-styles/flex
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a bit gnarly, can we break this out in a fn or so?

@oskarth oskarth self-requested a review January 13, 2018 09:43
Copy link
Contributor

@oskarth oskarth left a comment

Choose a reason for hiding this comment

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

Minor comments, in general looks good

[status-im.utils.platform :refer [android?]]
[status-im.ui.components.react :refer [view modal]]
[status-im.utils.platform :refer [android? ios?]]
[status-im.ui.components.react :refer [view safe-area-view modal dimensions]]
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we turn these into react/foo etc? Generally preferable (think these are leftovers)

@@ -36,6 +36,7 @@
(def geolocation (when (exists? js/window)
js/navigator.geolocation.))
(def view (get-class "View"))
(def safe-area-view (get-class "SafeAreaView"))
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this always available? What happens if it is not?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it is always available. React Native ships with SafeAreaView as of version 0.50.

@oskarth
Copy link
Contributor

oskarth commented Jan 13, 2018

^ @jeluard @flexsurfer please review

@status-github-bot status-github-bot bot moved this from CONTRIBUTOR to TO TEST in Pipeline for QA Feb 7, 2018
@foopang foopang force-pushed the fix/iphone-x-screen branch 6 times, most recently from f78716d to 1f4da3a Compare February 8, 2018 00:43
@foopang
Copy link
Contributor Author

foopang commented Feb 8, 2018

@Serhy Sorry for bringing in so many issues in this PR. Hope all is fixed now! Please have a look.

@Serhy Serhy moved this from TO TEST to IN TESTING in Pipeline for QA Feb 8, 2018
@Serhy
Copy link
Contributor

Serhy commented Feb 8, 2018

Thanks a lot, @foopang !
We checked https://i.diawi.com/nZrTp8 build on iPhoneX and iPhone 5s (and Android 7.0 too!) as well this PR and it was noticed small regression:
On Assets screen - the device status bar with white background while it was a blue one.

iPhoneX view:
image uploaded from ios 11

iPhone 5s view:
31412

Develop and mockup view:
screen shot 2018-02-08 at 16 49 03

Could you please apply on 'Wallet' -> top-right button -> 'Manage Assets' -> 'Assets' screen the blue background to device status bar and fix conflicts?

@Serhy Serhy moved this from IN TESTING to CONTRIBUTOR in Pipeline for QA Feb 8, 2018
@status-im status-im deleted a comment from statustestbot Feb 9, 2018
@status-im status-im deleted a comment from statustestbot Feb 9, 2018
@flexsurfer
Copy link
Member

@foopang could you please resolve conflicts and take a look on @Serhy comment, thanks

@foopang foopang force-pushed the fix/iphone-x-screen branch 2 times, most recently from c619016 to ac6ee6f Compare February 10, 2018 04:22
@status-github-bot status-github-bot bot moved this from CONTRIBUTOR to TO TEST in Pipeline for QA Feb 10, 2018
@foopang
Copy link
Contributor Author

foopang commented Feb 10, 2018

@Serhy Thanks for checking! All layout issues should have been fixed now.

@flexsurfer @jeluard something to note about the iPhone X screen issues, I did some hacks to workaround.

https://github.com/status-im/status-react/pull/2856/files#diff-d8aa2157cacb46da51afecaef8554e32R294 The bottom spacing is gone when in modal view. So I put an invisible view element to the bottom for adding bottom spacing. but it is not needed when the keyboard is opened up for signing transaction.

https://github.com/status-im/status-react/pull/2856/files#diff-93d9c10db297821b5145f29bdbb7b6bcR189 The transaction sent modal won't show but will block the layout from being navigated away when closing the transaction modal and immediately opening a new modal, so adding a delay here to fix.

https://github.com/status-im/status-react/pull/2856/files#diff-dc0057ed633d7dc9f584171ce43774d5R28 The top and bottom spacing has to be reduced from the height in order to get the screen viewport height of iPhone X so that "Scan QR code" screen can show properly.

https://github.com/status-im/status-react/pull/2856/files#diff-d8aa2157cacb46da51afecaef8554e32R275 The top bar and the bottom bar can't be two different background colors. To work around that I added a view with a different background color and stuck to the bottom so that it can show different background colors on Wallet screens.

GitHub
fixes #2719 I guess we will need another splash image for iPhone X portrait (1125px x 2436px) 🙂

status: ready

GitHub
fixes #2719 I guess we will need another splash image for iPhone X portrait (1125px x 2436px) 🙂

status: ready

GitHub
fixes #2719 I guess we will need another splash image for iPhone X portrait (1125px x 2436px) 🙂

status: ready

GitHub
fixes #2719 I guess we will need another splash image for iPhone X portrait (1125px x 2436px) 🙂

status: ready

@status-im status-im deleted a comment from statustestbot Feb 10, 2018
@Serhy Serhy moved this from TO TEST to IN TESTING in Pipeline for QA Feb 12, 2018
@Serhy
Copy link
Contributor

Serhy commented Feb 12, 2018

Builds Android: https://i.diawi.com/jH87hH and iOS: https://i.diawi.com/ND1hGn has been tested on iPhoneX and Android 7.0 (Samsung J7).

It looks good, no issues and app fits on iPhoneX screen.

@foopang great job! Thank you a lot for the efforts in this PR!

Ready for merge!

@Serhy Serhy moved this from IN TESTING to MERGE in Pipeline for QA Feb 12, 2018
Signed-off-by: Andrey Shovkoplyas <motor4ik@gmail.com>
@flexsurfer flexsurfer merged commit eba8df6 into status-im:develop Feb 12, 2018
Pipeline for QA automation moved this from MERGE to DONE Feb 12, 2018
@chadyj
Copy link
Contributor

chadyj commented Feb 12, 2018

Thank you @foopang! This has been a much requested feature and will be a fantastic enhancement to Status.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Black bars are shown when using iPhone X
8 participants