From b7fbe8d4e67a64c0ce6fe4e0fff209de8de6236f Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Fri, 16 Jul 2021 10:46:59 -0400 Subject: [PATCH 01/14] docs: Add architecture doc on handling safe areas. --- docs/architecture/safe-areas.md | 146 ++++++++++++++++++++++++++++++++ docs/style.md | 9 ++ 2 files changed, 155 insertions(+) create mode 100644 docs/architecture/safe-areas.md diff --git a/docs/architecture/safe-areas.md b/docs/architecture/safe-areas.md new file mode 100644 index 00000000000..f881863d14f --- /dev/null +++ b/docs/architecture/safe-areas.md @@ -0,0 +1,146 @@ +# Handling "safe areas" in the visual layout + +Some devices, like the iPhone X, have notches and native UI elements that +can overlap important content if we're not careful. We generally want the +boring, background part of elements to occupy the insets, and nothing else. +(We do let the lightbox image extend through the insets. This is a common +design choice, and the user can always zoom out to see what was hidden.) + +We generally follow React Navigation's recommendations in their +[doc](https://github.com/zulip/react-native) on safe areas. React +Navigation's built-in UI elements help us by handling the insets in some +places. But the available built-ins don't solve the entire problem, and we +also [choose not to +use](https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/react-nav.3A.20headers/near/1126189) +some of them because of their questionable design. Our own custom components +have to be part of the solution. + +We use +[`react-native-safe-area-context`](https://www.npmjs.com/package/react-native-safe-area-context), +on React Navigation's recommendation. + +## Rules for handling safe areas + +1. As part of every component's interface, we should include how it's meant + to interact with the safe area. + - Most components aren't aware of the safe area: they don't do anything + about it, and they assume they are rendered entirely inside it. So + this is the default, and we generally won't mention it in the jsdoc in + those cases. + - Some components expect to occupy the horizontal insets, some the + bottom inset, and some the top inset. (In principle there could be + other valid combinations.) For example, a header component should + occupy at least the top inset so that the header's background extends + to the top edge of the screen. This should be announced in the jsdoc, + saying why the component occupies the inset(s). +2. If a component wants to occupy any of the insets, it should generally do + so with a `SafeAreaView`, with `"padding"` for `mode`, and with the + intended `edges`. We prefer this component over `useSafeAreaInsets` and + friends, where possible, because it does the layout calculations + natively. If we have to look at inset dimensions in JavaScript, they + couldn't be up-to-date because they've been carried over React Native's + asynchronous bridge, and there will be a flicker when the insets change + (e.g., on orientation changes). +3. If a React component is meant to occupy any insets, then its layout + parent should adapt to allow it to do so, so that the inset distance + isn't covered twice. This generally means omitting a padding + `SafeAreaView` wrapper from the parent, if present, putting one in the + child, and adjusting the wrapper's other children as necessary to + compensate (e.g., by giving them `SafeAreaView`s, preferably with margin + instead of padding, to show they don't care what fills the space). There + are two reasons the parent should adapt: + - When it's a case where the child has a stronger claim to choose what + goes in the insets than the parent does, e.g., to extend its own + background to the edge of the screen. This comes up a lot with + list-item elements that want to extend their backgrounds to the left + and right edges of the screen, while keeping their meaningful content + within the safe area. + - On iOS, at least, a `SafeAreaView` doesn't change what padding/margin + it applies based on its position relative to the actual insets or + other `SafeAreaView`s. It just statically applies the inset distance, + wherever it is; a `SafeAreaView` nested in another `SafeAreaView` + will duplicate the padding/margin. So it's not convenient to make + components agnostic to whether an ancestor occupies the insets, and + we have to keep careful track of where we handle the insets. See the + "iOS" section for more on the relevant constraints here. + + +## iOS + +We were surprised to +[find](https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/react-native-safe-area-context.20padding/near/1230454) +the following [hypothesis](https://github.com/zulip/zulip-mobile/pull/4893#discussion_r668412622) wasn't true: + +> I think if you make a `SafeAreaView` at a place where it's actually completely inside the safe area -- for example, because it's nested other `SafeAreaView`s that provided the appropriate padding on each side -- then it has no effect. + +It seemed like it should be true, especially because that's how the relevant +iOS API works, namely [the `safeAreaInsets` property on +UIView](https://developer.apple.com/documentation/uikit/uiview/2891103-safeareainsets?language=objc). +But Greg +[found](https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/react-native-safe-area-context.20padding/near/1230523), +"[F]or some reason it's [not calling that method on the relevant view +itself](https://github.com/th3rdwave/react-native-safe-area-context/blob/cd8dd60d035a44c22459b2c890e6512e5796396e/ios/SafeAreaView/RNCSafeAreaView.m#L76), +i.e. the `RNCSafeAreaView`. Instead, it [walks up the tree to the enclosing +`RNCSafeAreaProvider`](https://github.com/th3rdwave/react-native-safe-area-context/blob/cd8dd60d035a44c22459b2c890e6512e5796396e/ios/SafeAreaView/RNCSafeAreaView.m#L88-L94), +and asks what *that* view's `safeAreaInsets` is. Which is... just the wrong +question." + +So that explains why every `SafeAreaView` uses the same values for +margin/padding at a given time. They're not getting the values from their +own position, but from the position of their shared `SafeAreaProvider`. + +We +[tried](https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/react-native-safe-area-context.20padding/near/1231222) +changing it to get `safeAreaInsets` from the self instead of the provider. +This seemed to make the above-quoted hypothesis true, which was great—except +we noticed a bug, which is plausibly the reason for not doing it this way. + +To understand the bug, note that the "self" approach will predictably affect +how `SafeAreaView`s behave when they're involved in slide animations near +the edges of the screen. For example, if the left edge of a `SafeAreaView` +is sliding left toward the edge of the screen, and it has entered the left +inset, then its left padding should increase with each frame so that the +content it's guarding will stay still within the safe area. See +[videos](https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/react-native-safe-area-context.20padding/near/1231279) +of this happening with a navigation transition. + +We do see this happening approximately, but not perfectly. Greg +[describes](https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/react-native-safe-area-context.20padding/near/1231377) +after seeing those videos, + +> It looks like the whole content of the page has some jank -- specifically +> some jitter on the left edge, in the last moments before it lands where +> it's going. I think that probably indicates some lack of synchronicity. +> Like it's looking up what the inset is, and it gets the answer as of right +> now… but then it uses that answer for the next frame and the next. And +> when there's an animation happening, the answer is changing, so the answer +> is wrong by the time it's laying out those later frames. + +Greg then +[found](https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/react-native-safe-area-context.20padding/near/1231419) +a piece of code in the library's iOS native layer that "does look to me kind +of suspiciously like it's setting some data that the RN layout engine will +use for the next frame -- not like it's using an API where it gets asked to +provide some information on each frame." + +This lack of synchronicity (even within the native layer—this is independent +of the problem with `useSafeAreaInsets` and friends, due to RN's +asynchronous bridge) might be due to a fundamental limitation in React +Native. + +Anyway, we don't want that jitter, and this is plausibly explains why the +library has each `SafeAreaView` ask for its provider's, not its own, idea of +the necessary padding/margin. + +This bug is probably a factor in how rough things look when you change the +device orientation on iOS. + +## Android + +We've never seen a case where an Android device has nonzero insets. As far +as we've seen, the whole app seems to occupy a clear rectangle given to it +by the system. It's below the status bar, and there are no overlapping +notches or native UI. When this is the case, each `SafeAreaView` will +appropriately act as a plain `View`. See +[discussion](https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/react-native-safe-area-context.20padding/near/1230471) +for more observations on Android. diff --git a/docs/style.md b/docs/style.md index 4be1ccde437..ef8c14990db 100644 --- a/docs/style.md +++ b/docs/style.md @@ -556,6 +556,15 @@ code where we're using a given aspect of the `display_recipient` semantics, which makes refactoring easier. + +
+ +### Layout principles + +See our [architecture doc on handling safe +areas](architecture/safe-areas.md) in the visual layout. + +
## WebView: HTML, CSS, JS From a7c456fb39865a5bd139bcc8fd01621b15820c95 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Fri, 16 Jul 2021 11:06:00 -0400 Subject: [PATCH 02/14] FullScreenLoading: Stop padding the top inset. The logo should be centered with respect to the entire screen, not the safe area (or worse, as it's been doing, the safe area plus three insets). It's also not big enough to risk entering the insets on any real screen, so it's fine to ignore the insets here. Also, this was using `useSafeAreaInsets`, which we try to avoid. --- src/common/FullScreenLoading.js | 9 --------- 1 file changed, 9 deletions(-) diff --git a/src/common/FullScreenLoading.js b/src/common/FullScreenLoading.js index e1ca1bc7b6f..035b84dedc6 100644 --- a/src/common/FullScreenLoading.js +++ b/src/common/FullScreenLoading.js @@ -2,7 +2,6 @@ import React from 'react'; import type { Node } from 'react'; import { View } from 'react-native'; -import { useSafeAreaInsets } from 'react-native-safe-area-context'; import { BRAND_COLOR, createStyleSheet } from '../styles'; // eslint-disable-next-line import/no-useless-path-segments @@ -23,18 +22,10 @@ type Props = $ReadOnly<{||}>; * Meant to be used to cover the whole screen. */ export default function FullScreenLoading(props: Props): Node { - const insets = useSafeAreaInsets(); - return ( <> - From 0fb815299fe51b4453c4051ab1090522d79689ae Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Fri, 16 Jul 2021 11:04:26 -0400 Subject: [PATCH 03/14] layout [nfc]: Add to jsdocs about safe-area handling, per just-added doc. --- src/common/Screen.js | 2 ++ src/common/ServerCompatBanner.js | 2 ++ src/compose/ComposeBox.js | 5 +++++ src/lightbox/LightboxFooter.js | 5 +++++ src/lightbox/LightboxHeader.js | 2 ++ src/main/MainTabsScreen.js | 7 +++++++ src/nav/ChatNavBar.js | 5 +++++ src/nav/ModalNavBar.js | 5 +++++ src/nav/ModalSearchNavBar.js | 5 +++++ 9 files changed, 38 insertions(+) diff --git a/src/common/Screen.js b/src/common/Screen.js index f52b59cc3ee..ff1a83947a8 100644 --- a/src/common/Screen.js +++ b/src/common/Screen.js @@ -56,6 +56,8 @@ type Props = $ReadOnly<{| * Provides a nav bar, colors the status bar, can center the contents, etc. * The `children` are ultimately wrapped in a `ScrollView` from upstream. * + * Pads the bottom inset with a theme-appropriate background color. + * * @prop [centerContent] - Should the contents be centered. * @prop children - Components to render inside the screen. * @prop [keyboardShouldPersistTaps] - Passed through to ScrollView. diff --git a/src/common/ServerCompatBanner.js b/src/common/ServerCompatBanner.js index c13dcfeb7fc..69dfbcbe6c8 100644 --- a/src/common/ServerCompatBanner.js +++ b/src/common/ServerCompatBanner.js @@ -45,6 +45,8 @@ type Props = $ReadOnly<{||}>; /** * A "nag banner" saying the server version is unsupported, if so. + * + * Pads the horizontal insets with its background. */ // Made with somewhat careful attention to // https://material.io/components/banners. Please consult that before making diff --git a/src/compose/ComposeBox.js b/src/compose/ComposeBox.js index 7daf3f88f92..6143a4e96f4 100644 --- a/src/compose/ComposeBox.js +++ b/src/compose/ComposeBox.js @@ -134,6 +134,11 @@ const updateTextInput = (textInput, text) => { } }; +/** + * The compose box, for new messages or editing messages (PM or stream). + * + * Pads the bottom inset with its background. + */ class ComposeBoxInner extends PureComponent { static contextType = ThemeContext; context: ThemeData; diff --git a/src/lightbox/LightboxFooter.js b/src/lightbox/LightboxFooter.js index 5da3f82c29b..8bfb08c9f80 100644 --- a/src/lightbox/LightboxFooter.js +++ b/src/lightbox/LightboxFooter.js @@ -33,6 +33,11 @@ type Props = $ReadOnly<{| onOptionsPress: () => void, |}>; +/** + * The lightbox's footer. + * + * Pads the right, bottom, and left insets with its background. + */ export default class LightboxFooter extends PureComponent { render(): Node { const { displayMessage, onOptionsPress, style } = this.props; diff --git a/src/lightbox/LightboxHeader.js b/src/lightbox/LightboxHeader.js index dd2f36cca98..c61e11c39ec 100644 --- a/src/lightbox/LightboxHeader.js +++ b/src/lightbox/LightboxHeader.js @@ -48,6 +48,8 @@ type Props = $ReadOnly<{| /** * Shows sender's name and date of photo being displayed. * + * Pads the top, right, and left insets with its background. + * * @prop [senderName] - The sender's full name. * @prop [avatarUrl] * @prop [timestamp] diff --git a/src/main/MainTabsScreen.js b/src/main/MainTabsScreen.js index 3c6723f7de5..86e9310e6da 100644 --- a/src/main/MainTabsScreen.js +++ b/src/main/MainTabsScreen.js @@ -45,6 +45,13 @@ type Props = $ReadOnly<{| route: RouteProp<'main-tabs', void>, |}>; +/** + * Wrapper and bottom tab navigator for the app's primary interface. + * + * Pads the top inset with a theme-appropriate background color. (This could + * be delegated to the individual tab screens, only if the screens promise + * not to disrupt a consistent look-and-feel.) + */ export default function MainTabsScreen(props: Props): Node { const { backgroundColor } = useContext(ThemeContext); diff --git a/src/nav/ChatNavBar.js b/src/nav/ChatNavBar.js index a2ae3e7f9cb..191a1bff4d4 100644 --- a/src/nav/ChatNavBar.js +++ b/src/nav/ChatNavBar.js @@ -22,6 +22,11 @@ type Props = $ReadOnly<{| editMessage: EditMessage | null, |}>; +/** + * `ChatScreen`'s top nav bar / app bar. + * + * Pads the top, right, and left insets with its background. + */ export default function ChatNavBar(props: Props): Node { const { narrow, editMessage } = props; const streamColor = useSelector(state => getStreamColorForNarrow(state, narrow)); diff --git a/src/nav/ModalNavBar.js b/src/nav/ModalNavBar.js index 72953db2ef7..73c7349c440 100644 --- a/src/nav/ModalNavBar.js +++ b/src/nav/ModalNavBar.js @@ -15,6 +15,11 @@ type Props = $ReadOnly<{| title: LocalizableText, |}>; +/** + * A flavor of top nav bar / app bar for Screen, with title and back button. + * + * Pads the top, right, and left insets with its background. + */ export default function ModalNavBar(props: Props): Node { const { canGoBack, title } = props; const { backgroundColor } = useContext(ThemeContext); diff --git a/src/nav/ModalSearchNavBar.js b/src/nav/ModalSearchNavBar.js index d0b1b9a56b1..06e7a463b67 100644 --- a/src/nav/ModalSearchNavBar.js +++ b/src/nav/ModalSearchNavBar.js @@ -13,6 +13,11 @@ type Props = $ReadOnly<{| canGoBack?: boolean, |}>; +/** + * A flavor of top nav / app bar for Screen with search bar and back button. + * + * Pads the top, right, and left insets with its background. + */ export default function ModalSearchNavBar(props: Props): Node { const { autoFocus, searchBarOnChange, canGoBack = true } = props; const { backgroundColor } = useContext(ThemeContext); From 4f56cfe3222d01aa3f962fe4785165b34a0d15a1 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Mon, 12 Jul 2021 11:50:39 -0400 Subject: [PATCH 04/14] i18n: Allow better translations for "# unread message(s)". The new message takes further advantage of ICU Message syntax than we have before; see https://formatjs.io/docs/core-concepts/icu-syntax/. Now, translators have more complete information about the text being translated, and will be able to give better, more locale-appropriate translations. This might cause confusion if Transifex doesn't give the user a nice UI; see discussion at https://chat.zulip.org/#narrow/stream/58-translation/topic/ICU.20Message.20syntax/near/1230242. This will also be nice for the layout because the text will be in just one element, not split between two. --- src/chat/UnreadNotice.js | 17 +++++++++-------- static/translations/messages_en.json | 3 +-- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/src/chat/UnreadNotice.js b/src/chat/UnreadNotice.js index 01d898692c4..283b18a60ae 100644 --- a/src/chat/UnreadNotice.js +++ b/src/chat/UnreadNotice.js @@ -7,7 +7,7 @@ import type { Narrow } from '../types'; import { createStyleSheet } from '../styles'; import { useSelector } from '../react-redux'; import { getUnreadCountForNarrow } from '../selectors'; -import { Label, RawLabel } from '../common'; +import { Label } from '../common'; import MarkAsReadButton from './MarkAsReadButton'; import AnimatedScaleComponent from '../animation/AnimatedScaleComponent'; @@ -24,11 +24,6 @@ const styles = createStyleSheet({ flexDirection: 'row', alignItems: 'center', }, - unreadNumber: { - fontSize: 14, - color: 'white', - paddingRight: 4, - }, unreadText: { fontSize: 14, color: 'white', @@ -46,10 +41,16 @@ export default function UnreadNotice(props: Props) { return ( 0} style={styles.unreadContainer}> - diff --git a/static/translations/messages_en.json b/static/translations/messages_en.json index 25b2a722726..1e92dc1be06 100644 --- a/static/translations/messages_en.json +++ b/static/translations/messages_en.json @@ -147,8 +147,7 @@ "Mark all as read": "Mark all as read", "Mark stream as read": "Mark stream as read", "Mark topic as read": "Mark topic as read", - "unread message": "unread message", - "unread messages": "unread messages", + "{unreadCount, plural,\n =0 {No unread messages}\n =1 {# unread message}\n other {# unread messages}\n}": "{unreadCount, plural,\n =0 {No unread messages}\n =1 {# unread message}\n other {# unread messages}\n}", "Group PM": "Group PM", "Share": "Share", "Star message": "Star message", From ec3f51a70c0daeaf038b3a12a6f2be1bad3db8d4 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Tue, 13 Jul 2021 15:18:35 -0400 Subject: [PATCH 05/14] UnreadNotice: Handle left and right insets. We'll tidy this up a bit in the next commit by removing the inner `View` (the one that uses the `unreadTextWrapper` styles). --- src/chat/UnreadNotice.js | 32 +++++++++++++++++++++----------- 1 file changed, 21 insertions(+), 11 deletions(-) diff --git a/src/chat/UnreadNotice.js b/src/chat/UnreadNotice.js index 283b18a60ae..ba8aeef7f10 100644 --- a/src/chat/UnreadNotice.js +++ b/src/chat/UnreadNotice.js @@ -2,6 +2,7 @@ import React from 'react'; import { View } from 'react-native'; +import { SafeAreaView } from 'react-native-safe-area-context'; import type { Narrow } from '../types'; import { createStyleSheet } from '../styles'; @@ -16,9 +17,11 @@ const styles = createStyleSheet({ paddingHorizontal: 8, paddingVertical: 4, backgroundColor: 'hsl(232, 89%, 78%)', + overflow: 'hidden', + }, + safeAreaWrapper: { flexDirection: 'row', justifyContent: 'space-between', - overflow: 'hidden', }, unreadTextWrapper: { flexDirection: 'row', @@ -34,26 +37,33 @@ type Props = $ReadOnly<{| narrow: Narrow, |}>; +/** + * Says how many unread messages are in the narrow. + * + * Pads the left and right insets with its background. + */ export default function UnreadNotice(props: Props) { const { narrow } = props; const unreadCount = useSelector(state => getUnreadCountForNarrow(state, narrow)); return ( 0} style={styles.unreadContainer}> - - + + ); } From e9b6e754d553e88eb2a88561aaf1ebc9f29ce2ac Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Tue, 13 Jul 2021 15:15:59 -0400 Subject: [PATCH 06/14] UnreadNotice [nfc]: Remove an unnecessary `View`. As Greg suggests at https://github.com/zulip/zulip-mobile/pull/4893#discussion_r668408096. --- src/chat/UnreadNotice.js | 20 +++++++------------- 1 file changed, 7 insertions(+), 13 deletions(-) diff --git a/src/chat/UnreadNotice.js b/src/chat/UnreadNotice.js index ba8aeef7f10..bc10d22f625 100644 --- a/src/chat/UnreadNotice.js +++ b/src/chat/UnreadNotice.js @@ -1,7 +1,6 @@ /* @flow strict-local */ import React from 'react'; -import { View } from 'react-native'; import { SafeAreaView } from 'react-native-safe-area-context'; import type { Narrow } from '../types'; @@ -22,9 +21,6 @@ const styles = createStyleSheet({ safeAreaWrapper: { flexDirection: 'row', justifyContent: 'space-between', - }, - unreadTextWrapper: { - flexDirection: 'row', alignItems: 'center', }, unreadText: { @@ -49,19 +45,17 @@ export default function UnreadNotice(props: Props) { return ( 0} style={styles.unreadContainer}> - - + values: { unreadCount }, + }} + /> From 7f2be5929adfa3790b9ba57ca4fd94d39d7f5688 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Fri, 16 Jul 2021 13:30:23 -0400 Subject: [PATCH 07/14] Popup: Handle left and right insets. In the next commit, note the TODOs in `EmojiAutocomplete`, `PeopleAutocomplete`, and `StreamAutocomplete` (`TopicAutocomplete isn't affected) for not double-handling these insets, which we do by inappropriately padding the item rows. The padding comes from some reusable components whose other callsites *do* need the padding. As the TODOs say, we should probably stop reusing those components, and make new ones. The use-cases are different enough that this probably makes sense to do in principle anyway. But another reason not to reuse them is that it'd just be hard to deal with this issue with the insets. `SafeAreaView` doesn't make it easy to conditionally handle no edges or some edges; see th3rdwave/react-native-safe-area-context#204. --- src/common/Popup.js | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/src/common/Popup.js b/src/common/Popup.js index de7a084903e..5bb9ccac39c 100644 --- a/src/common/Popup.js +++ b/src/common/Popup.js @@ -1,7 +1,7 @@ /* @flow strict-local */ import React, { useContext } from 'react'; import type { Node } from 'react'; -import { View } from 'react-native'; +import { SafeAreaView } from 'react-native-safe-area-context'; import { ThemeContext, createStyleSheet } from '../styles'; @@ -21,11 +21,20 @@ type Props = $ReadOnly<{| children: Node, |}>; +/** + * A floating box to contain autocomplete popups. + * + * Avoids the horizontal insets by adding appropriate margin. + */ export default function Popup(props: Props): Node { const themeContext = useContext(ThemeContext); return ( - + {props.children} - + ); } From 89d0a1c8137484c39922d0f35880da32da8b470b Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Mon, 12 Jul 2021 13:56:28 -0400 Subject: [PATCH 08/14] layout: Handle safe areas for screen-spanning "row" elements. Each of these UI elements is a row that has meaningful content that we need to keep within the safe areas, but the rest of the row (its distinct background color, its touchable area, etc.) is meant to extend through the insets to the extreme left and right of the screen. See example screenshots at https://github.com/zulip/zulip-mobile/pull/4683#issue-619662894. So, make that happen. We do so by giving the rows left and right padding. This is easy and makes the row elements' styles pretty intuitive, but it does mean we'll have to take care, in the next few commits, not to add any padding to the elements that contain the rows -- we don't want to double up the padding by mistake. (This requirement ends up being kind of annoying on screens that have a mix of these kinds of rows and regular elements. But this is unfortunately the workable design we've found; see our architecture doc on handling safe areas.) Related: #3066 --- src/account-info/AwayStatusSwitch.js | 2 ++ src/account-info/ProfileScreen.js | 3 +++ src/autocomplete/EmojiAutocomplete.js | 3 +++ src/autocomplete/PeopleAutocomplete.js | 3 +++ src/autocomplete/StreamAutocomplete.js | 3 +++ src/common/NestedNavRow.js | 7 +++++-- src/common/SectionHeader.js | 15 ++++++++++++--- src/common/SelectableOptionRow.js | 7 +++++-- src/common/SwitchRow.js | 14 ++++++++++++-- src/emoji/EmojiPickerScreen.js | 6 ++++++ src/emoji/EmojiRow.js | 11 ++++++++--- src/main/HomeScreen.js | 6 ++++++ src/pm-conversations/GroupPmConversationItem.js | 10 ++++++---- src/pm-conversations/PmConversationList.js | 5 ++++- src/pm-conversations/PmConversationsScreen.js | 10 +++++++--- src/settings/LanguagePicker.js | 6 ++++++ src/settings/LanguageScreen.js | 6 ++++++ src/settings/NotificationsScreen.js | 6 ++++++ src/settings/SettingsScreen.js | 6 ++++++ src/streams/CreateStreamScreen.js | 5 +++++ src/streams/EditStreamCard.js | 7 +++++++ src/streams/EditStreamScreen.js | 5 +++++ src/streams/StreamItem.js | 7 +++++-- src/streams/StreamList.js | 6 ++++++ src/streams/StreamSettingsScreen.js | 6 ++++++ src/streams/SubscriptionsCard.js | 6 ++++++ src/streams/TopicItem.js | 13 ++++++++++--- src/subscriptions/StreamListCard.js | 6 ++++++ src/topics/TopicList.js | 6 ++++++ src/topics/TopicListScreen.js | 6 ++++++ src/unread/UnreadCards.js | 6 ++++++ src/user-picker/UserPickerCard.js | 6 ++++++ src/user-status/UserStatusScreen.js | 6 ++++++ src/users/UserItem.js | 11 +++++++++-- src/users/UserList.js | 6 ++++++ src/users/UsersCard.js | 7 +++++++ src/users/UsersScreen.js | 6 ++++++ 37 files changed, 223 insertions(+), 27 deletions(-) diff --git a/src/account-info/AwayStatusSwitch.js b/src/account-info/AwayStatusSwitch.js index 2c31f41deac..9fe80959061 100644 --- a/src/account-info/AwayStatusSwitch.js +++ b/src/account-info/AwayStatusSwitch.js @@ -13,6 +13,8 @@ type Props = $ReadOnly<{||}>; * This is a stand-alone component that: * * retrieves the current user's `user status` data and presents it * * allows by switching it to control the `away` status + * + * Needs to occupy the horizontal insets because `SwitchRow` does. */ export default function AwayStatusSwitch(props: Props): Node { const awayStatus = useSelector(getSelfUserAwayStatus); diff --git a/src/account-info/ProfileScreen.js b/src/account-info/ProfileScreen.js index 8ce9560845b..96ed74a5fea 100644 --- a/src/account-info/ProfileScreen.js +++ b/src/account-info/ProfileScreen.js @@ -102,6 +102,9 @@ type Props = $ReadOnly<{| * It does not have a "Send private message" but has "Switch account" and "Log out" buttons. * * The user can still open `AccountDetails` on themselves via the (i) icon in a chat screen. + * + * Needs to occupy the horizontal insets because the away-status switch + * does. */ export default function ProfileScreen(props: Props): Node { const ownUser = useSelector(getOwnUser); diff --git a/src/autocomplete/EmojiAutocomplete.js b/src/autocomplete/EmojiAutocomplete.js index c00aaa4ecb3..ae62c08d850 100644 --- a/src/autocomplete/EmojiAutocomplete.js +++ b/src/autocomplete/EmojiAutocomplete.js @@ -34,6 +34,9 @@ export default function EmojiAutocomplete(props: Props): Node { data={emojiNames.slice(0, MAX_CHOICES)} keyExtractor={item => item.name} renderItem={({ item }) => ( + // TODO: Make and use a new emoji-item component with no padding + // for the insets. The rows' content should be bounded by the + // popup, which renders within the safe area. item.stream_id.toString()} renderItem={({ item }) => ( + // TODO: Make and use a new stream-item component with no padding + // for the insets. The rows' content should be bounded by the + // popup, which renders within the safe area. - + {!!Icon && } + ); } diff --git a/src/common/SectionHeader.js b/src/common/SectionHeader.js index 3313fd1d117..c55f67cbc5f 100644 --- a/src/common/SectionHeader.js +++ b/src/common/SectionHeader.js @@ -1,7 +1,7 @@ /* @flow strict-local */ import React, { PureComponent } from 'react'; import type { Node, Context } from 'react'; -import { View } from 'react-native'; +import { SafeAreaView } from 'react-native-safe-area-context'; import type { ThemeData } from '../styles'; import { ThemeContext, createStyleSheet } from '../styles'; @@ -18,6 +18,11 @@ type Props = $ReadOnly<{| text: string, |}>; +/** + * (TODO: Is this meant to be reusable? How?) + * + * Pads the horizontal insets with its background. + */ export default class SectionHeader extends PureComponent { static contextType: Context = ThemeContext; context: ThemeData; @@ -25,9 +30,13 @@ export default class SectionHeader extends PureComponent { render(): Node { const { text } = this.props; return ( - + + ); } } diff --git a/src/common/SelectableOptionRow.js b/src/common/SelectableOptionRow.js index e29f4036276..318cab8d68f 100644 --- a/src/common/SelectableOptionRow.js +++ b/src/common/SelectableOptionRow.js @@ -2,6 +2,7 @@ import React from 'react'; import type { Node } from 'react'; import { View } from 'react-native'; +import { SafeAreaView } from 'react-native-safe-area-context'; import { RawLabel, Touchable } from '.'; import { BRAND_COLOR, createStyleSheet } from '../styles'; @@ -56,6 +57,8 @@ type Props = $ReadOnly<{| * it is either active or not. The event handler shouldn't do random * things that aren't related to that state, like navigating to a * different screen. + * + * Pads the horizontal insets with its background. */ export default function SelectableOptionRow( props: Props, @@ -64,13 +67,13 @@ export default function SelectableOptionRow( return ( onRequestSelectionChange(itemKey, !selected)}> - + {subtitle !== undefined && } {selected && } - + ); } diff --git a/src/common/SwitchRow.js b/src/common/SwitchRow.js index bc348f8a47c..0c8e47563d7 100644 --- a/src/common/SwitchRow.js +++ b/src/common/SwitchRow.js @@ -3,6 +3,7 @@ import React, { useContext } from 'react'; import type { Node } from 'react'; import { View } from 'react-native'; import type { ViewStyleProp } from 'react-native/Libraries/StyleSheet/StyleSheet'; +import { SafeAreaView } from 'react-native-safe-area-context'; import type { SpecificIconType } from './Icons'; import Label from './Label'; @@ -25,6 +26,11 @@ const componentStyles = createStyleSheet({ /** * A row with a label and a switch component. + * + * Pads the horizontal insets with its background. (A parent component + * could probably do this instead, if desired. The choice to do it here is + * just in line with our other "row" components, like `SelectableOptionRow`, + * which do need to pad the insets.) */ export default function SwitchRow(props: Props): Node { const { label, value, onValueChange, style, Icon } = props; @@ -32,12 +38,16 @@ export default function SwitchRow(props: Props): Node { const themeContext = useContext(ThemeContext); return ( - + {!!Icon && } + ); } diff --git a/src/emoji/EmojiPickerScreen.js b/src/emoji/EmojiPickerScreen.js index 8cbe7eb9059..12c2e05eba3 100644 --- a/src/emoji/EmojiPickerScreen.js +++ b/src/emoji/EmojiPickerScreen.js @@ -32,6 +32,12 @@ type State = {| filter: string, |}; +/** + * A screen to choose an emoji reaction. + * + * Needs to occupy the horizontal insets because its descendents (the + * `EmojiRow`s) do. + */ class EmojiPickerScreen extends PureComponent { state = { filter: '', diff --git a/src/emoji/EmojiRow.js b/src/emoji/EmojiRow.js index 19f42edc989..7bf7b5e8d0f 100644 --- a/src/emoji/EmojiRow.js +++ b/src/emoji/EmojiRow.js @@ -1,7 +1,7 @@ /* @flow strict-local */ import React, { useCallback } from 'react'; import type { Node } from 'react'; -import { View } from 'react-native'; +import { SafeAreaView } from 'react-native-safe-area-context'; import type { EmojiType } from '../types'; import { createStyleSheet } from '../styles'; @@ -26,6 +26,11 @@ type Props = $ReadOnly<{| onPress: (name: string) => void, |}>; +/** + * A list item for an emoji, e.g., for autocomplete or the reaction picker. + * + * Pads the horizontal insets with its background. + */ export default function EmojiRow(props: Props): Node { const { code, name, type, onPress } = props; @@ -35,10 +40,10 @@ export default function EmojiRow(props: Props): Node { return ( - + - + ); } diff --git a/src/main/HomeScreen.js b/src/main/HomeScreen.js index 62068ae4004..9a0caf0b140 100644 --- a/src/main/HomeScreen.js +++ b/src/main/HomeScreen.js @@ -33,6 +33,12 @@ type Props = $ReadOnly<{| route: RouteProp<'home', void>, |}>; +/** + * The first tab in the main-tabs screen, showing unread counts. + * + * Needs to occupy the horizontal insets because its descendents (the + * unread conversation items) do. + */ export default function HomeScreen(props: Props) { const dispatch = useDispatch(); diff --git a/src/pm-conversations/GroupPmConversationItem.js b/src/pm-conversations/GroupPmConversationItem.js index 3327454dd4d..d8235b35fee 100644 --- a/src/pm-conversations/GroupPmConversationItem.js +++ b/src/pm-conversations/GroupPmConversationItem.js @@ -1,7 +1,7 @@ /* @flow strict-local */ import React, { useCallback, useContext } from 'react'; import type { Node } from 'react'; -import { View } from 'react-native'; +import { SafeAreaView } from 'react-native-safe-area-context'; import { useSelector } from '../react-redux'; import type { UserOrBot } from '../types'; @@ -26,7 +26,9 @@ type Props = $ReadOnly<{| /** * A list item describing one group PM conversation. - * */ + * + * Pads the horizontal insets with its background. + */ export default function GroupPmConversationItem>( props: Props, ): Node { @@ -44,7 +46,7 @@ export default function GroupPmConversationItem>( return ( - + >( text={names.join(', ')} /> - + ); } diff --git a/src/pm-conversations/PmConversationList.js b/src/pm-conversations/PmConversationList.js index 4976d7c50d8..1fe549b8a1b 100644 --- a/src/pm-conversations/PmConversationList.js +++ b/src/pm-conversations/PmConversationList.js @@ -26,7 +26,10 @@ type Props = $ReadOnly<{| /** * A list describing all PM conversations. - * */ + * + * Needs to occupy the horizontal insets because its descendents (the PM + * conversation items) do. + */ export default function PmConversationList(props: Props): Node { const dispatch = useDispatch(); diff --git a/src/pm-conversations/PmConversationsScreen.js b/src/pm-conversations/PmConversationsScreen.js index f5fcb1d3b73..a2dd3972e50 100644 --- a/src/pm-conversations/PmConversationsScreen.js +++ b/src/pm-conversations/PmConversationsScreen.js @@ -3,6 +3,7 @@ import React, { useContext } from 'react'; import type { Node } from 'react'; import { View } from 'react-native'; +import { SafeAreaView } from 'react-native-safe-area-context'; import type { RouteProp } from '../react-navigation'; import type { MainTabsNavigationProp } from '../main/MainTabsScreen'; @@ -41,14 +42,17 @@ type Props = $ReadOnly<{| /** * The "PMs" page in the main tabs navigation. - * */ + * + * Needs to occupy the horizontal insets because its descendents (the PM + * conversation items) do. + */ export default function PmConversationsScreen(props: Props): Node { const conversations = useSelector(getRecentConversations); const context = useContext(ThemeContext); return ( - + NavigationService.dispatch(navigateToUsersScreen())); }} /> - + {conversations.length === 0 ? ( From 823c70f76c728987bab004d69fa3205feade074e Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Tue, 13 Jul 2021 15:05:51 -0400 Subject: [PATCH 13/14] ChatScreen: Fix blank strip between compose box and keyboard on iOS. Fixes: #3370 --- src/chat/ChatScreen.js | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/src/chat/ChatScreen.js b/src/chat/ChatScreen.js index 3942caf4756..9139a250ac8 100644 --- a/src/chat/ChatScreen.js +++ b/src/chat/ChatScreen.js @@ -2,6 +2,7 @@ import React from 'react'; import type { Node } from 'react'; import { useIsFocused } from '@react-navigation/native'; +import { useSafeAreaInsets } from 'react-native-safe-area-context'; import { useSelector, useDispatch } from '../react-redux'; import type { RouteProp } from '../react-navigation'; @@ -121,7 +122,24 @@ export default function ChatScreen(props: Props): Node { const showComposeBox = canSendToNarrow(narrow) && !showMessagePlaceholders; return ( - + From 7d53cd34a51c12539e38c7d36a22ca5a76f033ba Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Mon, 12 Jul 2021 13:43:52 -0400 Subject: [PATCH 14/14] message list: Quickly handle safe areas in the message list. This isn't ideal; it just puts the whole message list in a box that's contained within the safe area. The message list was designed to span the entire width of the screen, and some things look pretty bad when the insets are nonzero [1]: the timestamp pills, for one example, look like they get cut off artificially instead of being tucked away somewhere offscreen. Things like that would best be solved with a more in-depth design discussion. A more complete solution might start with the "env" CSS function [2], or, less optimally, a new WebView inbound event to be fired whenever the insets change. At least this solution lets things be visible and interactable that weren't before. I think this completes #3066 (modulo this message-list issue discussed here), which is our current audit for safe-area handling. [1] So far, I think this is just landscape mode on newer iOS devices, but I might be missing something. [2] https://webkit.org/blog/7929/designing-websites-for-iphone-x/ Fixes: #3066 --- src/chat/ChatScreen.js | 21 +++++++++++++-------- src/search/SearchMessagesCard.js | 9 ++++++--- 2 files changed, 19 insertions(+), 11 deletions(-) diff --git a/src/chat/ChatScreen.js b/src/chat/ChatScreen.js index 9139a250ac8..6f1afbe2717 100644 --- a/src/chat/ChatScreen.js +++ b/src/chat/ChatScreen.js @@ -2,7 +2,7 @@ import React from 'react'; import type { Node } from 'react'; import { useIsFocused } from '@react-navigation/native'; -import { useSafeAreaInsets } from 'react-native-safe-area-context'; +import { useSafeAreaInsets, SafeAreaView } from 'react-native-safe-area-context'; import { useSelector, useDispatch } from '../react-redux'; import type { RouteProp } from '../react-navigation'; @@ -152,13 +152,18 @@ export default function ChatScreen(props: Props): Node { return ; } else { return ( - + // TODO: Keep the meaningful content within the safe areas, but + // let the background elements extend through the insets all the + // way to the left and right of the screen. + + + ); } })()} diff --git a/src/search/SearchMessagesCard.js b/src/search/SearchMessagesCard.js index 9f25dfb7334..70c6db92bcf 100644 --- a/src/search/SearchMessagesCard.js +++ b/src/search/SearchMessagesCard.js @@ -2,7 +2,7 @@ import React, { PureComponent } from 'react'; import type { Node } from 'react'; -import { View } from 'react-native'; +import { SafeAreaView } from 'react-native-safe-area-context'; import type { Message, Narrow } from '../types'; import { createStyleSheet } from '../styles'; @@ -42,7 +42,10 @@ export default class SearchMessagesCard extends PureComponent { } return ( - + // TODO: Keep the meaningful content within the safe areas, but + // let the background elements extend through the insets all the + // way to the left and right of the screen. + { // or make this prop optional startEditMessage={() => undefined} /> - + ); } }