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

#2243 optimizing for iphone x #2260

Merged
merged 4 commits into from
Nov 9, 2017
Merged

#2243 optimizing for iphone x #2260

merged 4 commits into from
Nov 9, 2017

Conversation

danielduan
Copy link
Member

@danielduan danielduan commented Nov 8, 2017

Issue:

fix - #2243 on iPhone X, menu button and header is partially hidden

What I did

  • Added vertical and horizontal margins on iPhone X for different orientations
  • Removed strange story preview container resize behavior when menu is open
  • Made menu background semi-transparent so users can see what's behind it
  • Made text sizes larger so users with larger device screens can see better
  • Changed all UI colors to black or white from different shades of alpha gray
  • Disabled status bar on app start: might fix - Bug : The hamburger goes behind the status bar on Android  #2102

screen shot 2017-11-08 at 11 16 17 pm

screen shot 2017-11-08 at 11 16 21 pm

screen shot 2017-11-08 at 11 16 33 pm

How to test

Is this testable with jest or storyshots?
yes
Does this need a new example in the kitchen sink apps?
no
Does this need an update to the documentation?
no
If your answer is yes to any of these, please make sure to include it in your PR.

@danielduan
Copy link
Member Author

can someone test this on android to make sure it still works?

@Hypnosphi
Copy link
Member

Hypnosphi commented Nov 8, 2017

can someone test this on android

will do

@codecov
Copy link

codecov bot commented Nov 8, 2017

Codecov Report

Merging #2260 into master will decrease coverage by <.01%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2260      +/-   ##
==========================================
- Coverage   22.16%   22.15%   -0.01%     
==========================================
  Files         268      268              
  Lines        5883     5885       +2     
  Branches      716      720       +4     
==========================================
  Hits         1304     1304              
+ Misses       4028     4007      -21     
- Partials      551      574      +23
Impacted Files Coverage Δ
...tive/src/preview/components/StoryListView/style.js 0% <ø> (ø) ⬆️
...-native/src/preview/components/OnDeviceUI/style.js 0% <ø> (ø) ⬆️
...-native/src/preview/components/OnDeviceUI/index.js 0% <0%> (ø) ⬆️
...s/left_panel/stories_tree/tree_decorators_utils.js 45.23% <0%> (ø) ⬆️
lib/ui/src/modules/ui/libs/filters.js 47.36% <0%> (ø) ⬆️
addons/knobs/src/components/PropForm.js 9.09% <0%> (ø) ⬆️
addons/knobs/src/components/types/Object.js 5.88% <0%> (ø) ⬆️
lib/codemod/src/transforms/update-addon-info.js 50% <0%> (ø) ⬆️
.../src/manager/containers/CommentsPanel/dataStore.js 34.04% <0%> (ø) ⬆️
addons/storyshots/src/storybook-channel-mock.js 0% <0%> (ø) ⬆️
... and 20 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e76fce3...7abae26. Read the comment docs.

@Hypnosphi
Copy link
Member

Hypnosphi commented Nov 8, 2017

Here's how it looks like on my Nexus 5X:
screenshot_20171108-041042

@Hypnosphi
Copy link
Member

Oddly enough the status bar is unhideable now (in master version, on contrary, it's hidden by default)

@Hypnosphi
Copy link
Member

Here's a screenshot from master:
photo5195226062701832233

@danielduan
Copy link
Member Author

Okay, looks like Android status bar still needs a bit of height to it. Let me add that.

@Hypnosphi
Copy link
Member

Ideally, the status bar should automatically hide like it does on master — otherwise we have a regression gere, I think

@danielduan
Copy link
Member Author

danielduan commented Nov 8, 2017

The implementation is a bit more complicated than that. I have no way of reliably detecting whether it's an iPhone X or not. I have to either use Expo's API to support CRNA or something like react-native-device-info which requires a native hook into the app.

Instead I just used react-native and its Platform API to determine when to pad the top. iOS will get 30px when the status bar is realistically only 20px on all other iPhones. Android I can push it and get 10px or 20px as well.

There's a new API SafeAreaView from react-native that needs to be backported so we don't regress on everything below 0.50.

I'm gonna experiment with this and see if it works for us: https://www.npmjs.com/package/react-native-iphone-x-helper

@codecov-io
Copy link

codecov-io commented Nov 9, 2017

Codecov Report

Merging #2260 into master will decrease coverage by 0.03%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2260      +/-   ##
==========================================
- Coverage   22.16%   22.13%   -0.04%     
==========================================
  Files         268      268              
  Lines        5883     5892       +9     
  Branches      697      704       +7     
==========================================
  Hits         1304     1304              
+ Misses       4073     4068       -5     
- Partials      506      520      +14
Impacted Files Coverage Δ
...tive/src/preview/components/StoryListView/style.js 0% <ø> (ø) ⬆️
...-native/src/preview/components/OnDeviceUI/style.js 0% <ø> (ø) ⬆️
...-native/src/preview/components/OnDeviceUI/index.js 0% <0%> (ø) ⬆️
app/vue/src/server/config/babel.js 0% <0%> (-100%) ⬇️
app/vue/src/server/babel_config.js 0% <0%> (-77.42%) ⬇️
lib/ui/src/modules/ui/routes.js 0% <0%> (ø) ⬆️
...react-native/src/manager/components/PreviewHelp.js 0% <0%> (ø) ⬆️
addons/knobs/src/components/types/Select.js 7.93% <0%> (ø) ⬆️
addons/knobs/src/components/types/Boolean.js 11.62% <0%> (ø) ⬆️
addons/knobs/src/KnobManager.js 31.37% <0%> (ø) ⬆️
... and 22 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7f535b4...19361ef. Read the comment docs.

@danielduan
Copy link
Member Author

@Hypnosphi can I request another test from you?

I think this should solve all of the problems. There's some minor style improvements in here too.

@Hypnosphi
Copy link
Member

The status bar is hidden again, thanks! Will also test in emulator to check if #2102 is fixed

@Hypnosphi
Copy link
Member

I failed to manage to launch an emulator =( Let's just merge this, but not close #2102 until the author confirms that it works

@Hypnosphi Hypnosphi merged commit f0588f6 into master Nov 9, 2017
@Hypnosphi Hypnosphi deleted the dd/iphone-x branch November 9, 2017 21:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants