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

Allow modification of SafeAreaView props #3496

Merged
merged 3 commits into from Mar 1, 2018

Conversation

Projects
None yet
6 participants
@ArsenyYankovsky
Contributor

ArsenyYankovsky commented Feb 11, 2018

Motivation:

So my use case is the following. I have this offline thing on top of my screens:

rsz_screenshot_1518292144

Which I achieve by doing following in my App.js:

    <SafeAreaView/>
    <NetworkStatus isOffline={this.state.isOffline}/>
    <RootNavigation />

This works fine on Android, but on iOS I get some additional padding on the header which looks like this:

rsz_2s6a5205_k

By introducing new safeAreaViewProps option we allow users to override forceInset property which solves my use case.

Test plan

I used following code to fix my issue described above:

    Story: {
      screen: StoryScreen,
      navigationOptions: ({navigation}) => ({
        title: "Story",
        safeAreaViewProps: { forceInset: {}}
      }),
    },

For the screens with unmodified options default behavior is applied. I also tested it on Android which led to the same results.

All tests pass and eslint doesn't report any errors.

@codecov-io

This comment has been minimized.

codecov-io commented Feb 11, 2018

Codecov Report

Merging #3496 into master will increase coverage by 0.03%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3496      +/-   ##
==========================================
+ Coverage   69.77%   69.81%   +0.03%     
==========================================
  Files          53       53              
  Lines        1704     1706       +2     
==========================================
+ Hits         1189     1191       +2     
  Misses        515      515
Impacted Files Coverage Δ
src/views/Header/Header.js 70.28% <100%> (+0.43%) ⬆️

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 416fe58...0a1095f. Read the comment docs.

@brentvatne

This comment has been minimized.

Member

brentvatne commented Feb 17, 2018

can you rebase this? looks like there is a conflict.

also, this needs to be added to the flow/react-native.js file and docs.

as an aside, and I think you pointed this out in a previous issue -- I'm not sure that this is a good solution in the long-term but if it solves problems for people for now that's great and we can revisit it when @ericvicenti and I revisit the problem of safe areas in the coming months.

@ArsenyYankovsky

This comment has been minimized.

Contributor

ArsenyYankovsky commented Feb 17, 2018

I've added flow stuff. I think this is more flexible as the PR I submitted before, it allows users to access SafeAreaView properties. Where should I add the docs? Couldn't find the place in the repo.

@brentvatne

This comment has been minimized.

Member

brentvatne commented Feb 23, 2018

thanks @ArsenyYankovsky! can you possibly rebase again? I made some changes to header recently that resulted in conflicts.

as for docs -- https://github.com/react-navigation/react-navigation.github.io

@react-navigation-ci

This comment has been minimized.

@ArsenyYankovsky ArsenyYankovsky force-pushed the ArsenyYankovsky:master branch 2 times, most recently from 958de4f to 8597370 Feb 25, 2018

@react-navigation-ci

This comment has been minimized.

1 similar comment
@react-navigation-ci

This comment has been minimized.

@ArsenyYankovsky

This comment has been minimized.

Contributor

ArsenyYankovsky commented Feb 25, 2018

I've rebased repo and created a pull request for the documentation repo react-navigation/react-navigation.github.io#70.

return (
<SafeAreaView
forceInset={{ top: 'always', bottom: 'never' }}
style={containerStyles}
{...safeAreaViewProps}

This comment has been minimized.

@brentvatne

brentvatne Feb 26, 2018

Member

I thought about this some more and I think we probably don't want people to inject any random prop in here, they could accidentally break the layout in their app. I think instead we should only pay attention to the forceInset prop. so we would change it to something like the following:

const { safeAreaViewProps = {} } = options;
const forceInset = safeAreaViewProps.forceInset || { top: 'always', bottom: 'never' }; 

return (
  <SafeAreaView
    forceInset={forceInset}
    style={containerStyles}
  >

This comment has been minimized.

@satya164

satya164 Feb 27, 2018

Collaborator

Maybe it makes more sense to have the prop named as headerForceInset instead of safeAreaViewProps since we're not passing all props here and SafeAreaView is an implementation detail.

This comment has been minimized.

@dhirendrarathod2000

dhirendrarathod2000 Feb 27, 2018

or we can put safe header style inside that too.
Does that sound right?
So that all the new props we add in the future we can add under that object.

This comment has been minimized.

@brentvatne

brentvatne Feb 27, 2018

Member

@satya164 - makes sense. let's do what @satya164 suggested

@dhirendrarathod2000

This comment has been minimized.

dhirendrarathod2000 commented Feb 27, 2018

I am also facing the same problem.
The code that is suggested under code review works for me.

Any ETA when this will get released?

Thanks
D

@ArsenyYankovsky

This comment has been minimized.

Contributor

ArsenyYankovsky commented Feb 27, 2018

@dhirendrarathod2000 I will do the changes @brentvatne suggested tonight.

@react-navigation-ci

This comment has been minimized.

@ArsenyYankovsky

This comment has been minimized.

Contributor

ArsenyYankovsky commented Feb 27, 2018

I've updated both this and documentation pull requests (react-navigation/react-navigation.github.io#70)

@dhirendrarathod2000

This comment has been minimized.

dhirendrarathod2000 commented Mar 1, 2018

@brentvatne Can you please approve/merge this?

@react-navigation-ci

This comment has been minimized.

@brentvatne brentvatne merged commit 214eeb1 into react-navigation:master Mar 1, 2018

1 check passed

ci/circleci Your tests passed on CircleCI!
Details

brentvatne added a commit that referenced this pull request Mar 1, 2018

Allow modification of SafeAreaView props (#3496)
* SafeAreaView fix

* Updated to only allow modification of forceInset property of SafeAreaView
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment