From aee35d873f0d1e225162aab8a770c1dbe31205ce Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Wed, 23 Sep 2020 15:59:28 -0700 Subject: [PATCH 1/2] lightbox types [nfc]: Bring out `SelectorProps`, as usual. --- src/lightbox/Lightbox.js | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/src/lightbox/Lightbox.js b/src/lightbox/Lightbox.js index c5d3132ded6..78eb154afd4 100644 --- a/src/lightbox/Lightbox.js +++ b/src/lightbox/Lightbox.js @@ -36,12 +36,17 @@ const styles = createStyleSheet({ }, }); -type Props = $ReadOnly<{| +type SelectorProps = $ReadOnly<{| auth: Auth, - dispatch: Dispatch, +|}>; + +type Props = $ReadOnly<{| src: string, message: Message, showActionSheetWithOptions: ShowActionSheetWithOptions, + + dispatch: Dispatch, + ...SelectorProps, |}>; type State = {| From 5b3daeaa64f29ccec330006daae1478648ef32c1 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Wed, 23 Sep 2020 15:58:08 -0700 Subject: [PATCH 2/2] lightbox: Fix lack of access to close button on some iOS devices. Even though we've been passing the `hidden` prop for the lightbox's ZulipStatusBar since the beginning (3f8ad4a30), evidently it sometimes still appears. I don't have a clear answer for why, but I suspect it might have to do with a particular subtlety in react-navigation. From their docs [1]: """ If you're using a tab or drawer navigator, it's a bit more complex because all of the screens in the navigator might be rendered at once and kept rendered - that means that the last `StatusBar` config you set will be used (likely on the final tab of your tab navigator, not what the user is seeing). """ We do use a tab navigator (`MainTabs`), so this isn't implausible on its face. When the status bar appears, it's been causing #4267: the close button appears behind the status bar. The `ZulipStatusBar` component has been conscripted into doing part of the work of React Native's [2], or React Navigation's [3], `SafeAreaView` component, which we haven't started using yet. (#3067 is open for using it all over the app.) In particular, as long as the `hidden` prop is true, a `View` with the height of the top inset of the safe area (wrapping the status bar) prevents the rest of the screen's content from "unsafely" rendering in that area. When this screen's `ZulipStatusBar` has its `hidden` prop passed as `true`, however, it doesn't defend the safe-area view at the top, whether or not a status bar is actually showing. That's because the wrapping `View` gets its height set to zero in the `hidden` case. So, without answering why a status bar might actually be showing when we tell it not to, remove the wrapping `View`'s height difference between `hidden` being true and false, conceding that `ZulipStatusBar` should be the defender of the top safe area, and in particular that it should still do so when it's been marked as `hidden`. At least until we make the sweeping changes of #3067. Also, we've got a sliding animation for the header, and the distance it needs to travel has increased by the height of the safe area. So, account for that by adding `safeAreaInsets.top` to `NAVBAR_SIZE` in the appropriate place. (Without that addition, the header just retreats into the top inset instead of leaving the screen entirely.) There are no other places where we pass `hidden` as `true` for `ZulipStatusBar`, so changing its behavior in that situation shouldn't have any nasty side effects. [1] https://reactnavigation.org/docs/4.x/status-bar/#tabs-and-drawer [2] https://reactnative.dev/docs/0.62/safeareaview [3] https://reactnavigation.org/docs/4.x/handling-iphonex/ Fixes: #4267 --- src/common/ZulipStatusBar.js | 2 +- src/lightbox/Lightbox.js | 10 ++++++---- 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/src/common/ZulipStatusBar.js b/src/common/ZulipStatusBar.js index 564a26a9d89..de0f3f4e88d 100644 --- a/src/common/ZulipStatusBar.js +++ b/src/common/ZulipStatusBar.js @@ -56,7 +56,7 @@ class ZulipStatusBar extends PureComponent { render() { const { theme, hidden, safeAreaInsets, orientation } = this.props; const backgroundColor = this.props.backgroundColor ?? this.props.narrowTitleBackgroundColor; - const style = { height: hidden ? 0 : safeAreaInsets.top, backgroundColor }; + const style = { height: safeAreaInsets.top, backgroundColor }; const statusBarColor = getStatusBarColor(backgroundColor, theme); return ( orientation === 'PORTRAIT' && ( diff --git a/src/lightbox/Lightbox.js b/src/lightbox/Lightbox.js index 78eb154afd4..d50b48d666b 100644 --- a/src/lightbox/Lightbox.js +++ b/src/lightbox/Lightbox.js @@ -5,10 +5,10 @@ import { View, Dimensions, Easing } from 'react-native'; import PhotoView from 'react-native-photo-view'; import { connectActionSheet } from '@expo/react-native-action-sheet'; -import type { Auth, Dispatch, Message } from '../types'; +import type { Auth, Dispatch, Message, Dimensions as ZulipDimensions } from '../types'; import { connect } from '../react-redux'; import type { ShowActionSheetWithOptions } from '../message/messageActionSheet'; -import { getAuth } from '../selectors'; +import { getAuth, getSession } from '../selectors'; import { getResource } from '../utils/url'; import { SlideAnimationView } from '../common'; import LightboxHeader from './LightboxHeader'; @@ -38,6 +38,7 @@ const styles = createStyleSheet({ type SelectorProps = $ReadOnly<{| auth: Auth, + safeAreaInsets: ZulipDimensions, |}>; type Props = $ReadOnly<{| @@ -95,7 +96,7 @@ class Lightbox extends PureComponent { }); render() { - const { src, message, auth } = this.props; + const { src, message, auth, safeAreaInsets } = this.props; const footerMessage = message.type === 'stream' ? `Shared in #${message.display_recipient}` : 'Shared with you'; const resource = getResource(src, auth); @@ -113,7 +114,7 @@ class Lightbox extends PureComponent { @@ -141,5 +142,6 @@ class Lightbox extends PureComponent { export default connectActionSheet( connect(state => ({ auth: getAuth(state), + safeAreaInsets: getSession(state).safeAreaInsets, }))(Lightbox), );