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

[iOS] Add additionalSafeAreaInsets prop for SafeAreaView #23784

Closed

Conversation

zhongwuzw
Copy link
Contributor

Summary

Currently, if we use SafeAreaView, we lose the padding prop, https://github.com/facebook/react-native/blob/master/React/Views/SafeAreaView/RCTSafeAreaShadowView.m#L36, so the insets can only determined by system, but, actually, user may wants some additional insets based on system safe area, like #22211, and we can also see Apple document, https://developer.apple.com/documentation/uikit/uiviewcontroller/2902284-additionalsafeareainsets?language=objc#, it also has additionalSafeAreaInsets for ViewController, so I think we can add additionalSafeAreaInsets prop for user to do custom insets.

Changelog

[iOS] [Added] - Add additionalSafeAreaInsets prop for SafeAreaView

Test Plan

After we added additionalSafeAreaInsets prop, the final safe area equals to system safe area plus user custom safe area:

      <SafeAreaView style={styles.container} additionalSafeAreaInsets={{top: 50, left: 0, bottom: 0, right: 0}}>
        <View style={styles.content} />
      </SafeAreaView>

  container: {
    flex: 1,
    backgroundColor: 'red',
    padding: 50, // This padding is not respected
  },
  content: {
    flex: 1,
    backgroundColor: 'yellow',
  },

image

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Mar 6, 2019
Copy link

@analysis-bot analysis-bot left a comment

Choose a reason for hiding this comment

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

Code analysis results:

  • flow found some issues.

@zhongwuzw zhongwuzw force-pushed the safeareaview_add_additionalinsets branch from e8dfd1c to 92987ef Compare March 6, 2019 12:29
[iOS] Add additionalSafeAreaInsets prop for SafeAreaView

[iOS] Add additionalSafeAreaInsets prop for SafeAreaView

fix flow check

Add EdgeInsetsProp import

Rename totalSafeAreaInsetsWithInsets
@zhongwuzw zhongwuzw force-pushed the safeareaview_add_additionalinsets branch from 472bda2 to 21a0e87 Compare March 6, 2019 12:33
@react-native-bot react-native-bot added PR: Includes Changelog Type: Enhancement A new feature or enhancement of an existing feature. labels Mar 14, 2019
@janicduplessis
Copy link
Contributor

@zhongwuzw Would it be possible to just support padding instead of adding a new prop that basically does the same? I think we override the padding in the shadow node so we could just add safe area and the current padding.

@zhongwuzw
Copy link
Contributor Author

@janicduplessis Emm, my opinions based on two reasons:

  1. We already removing support for padding in safe area view, please see https://github.com/facebook/react-native/blob/master/React/Views/SafeAreaView/RCTSafeAreaShadowView.m#L36, so at least, if we reuse padding, I think it's the API breaking?
  2. I added the same prop as Apple does, it provide additionalsafeareainsets prop for custom insets, https://developer.apple.com/documentation/uikit/uiviewcontroller/2902284-additionalsafeareainsets?language=objc#.

So I did this PR. 😂

@shergin
Copy link
Contributor

shergin commented Apr 6, 2019

  • As Janic said, there is no need to introduce new prop names, we can theoretically reuse padding.
  • On iOS, additionalSafeAreaInsets is for communicating safe area constrains to other view; RN does not have such concept at all, RN uses regular padding that applies only for SafeAreaView.
  • I think there is no harm to use additional View to add additional padding.
  • I think we should try to implement paddings for SafeAreView that replace the SafeArea-based ones if they are not zeros.

Copy link
Contributor

@shergin shergin left a comment

Choose a reason for hiding this comment

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

See my comment.

@shergin shergin closed this Apr 6, 2019
@janicduplessis
Copy link
Contributor

@shergin @zhongwuzw What about we add the padding with the safe area insets? There was a couple times I wanted to add padding to a SafeAreaView and the behaviour I was expecting was that.

A common case would be a footer bar:

<SafeAreaView style={{padding: 16}}>...</SafeAreaView>

In this case the padding bottom would be safeAreaInsets.bottom + 16

@zhongwuzw
Copy link
Contributor Author

@janicduplessis Before I added a new prop additionalSafeAreaInsets, I considered re-enable support of padding, at that time, I worry about it has breaking issue, but if it's acceptable, I agree to re-enable it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Component: SafeAreaView Platform: iOS iOS applications. Type: Enhancement A new feature or enhancement of an existing feature.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants