From e6e8f21de0888dfaa2caa44a548bc417e5d0d136 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Thu, 3 Mar 2022 14:11:00 -0800 Subject: [PATCH 01/20] UserStatusScreen [nfc]: Make a conditional more transparent We've currently got statusText: string | null so this new code gives the same results, but it's clearer. --- src/user-statuses/UserStatusScreen.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/user-statuses/UserStatusScreen.js b/src/user-statuses/UserStatusScreen.js index 4e00189df40..ec2842e0afe 100644 --- a/src/user-statuses/UserStatusScreen.js +++ b/src/user-statuses/UserStatusScreen.js @@ -70,7 +70,7 @@ export default function UserStatusScreen(props: Props): Node { maxLength={60} style={styles.statusTextInput} placeholder="What’s your status?" - value={statusText || ''} + value={statusText ?? ''} onChangeText={setStatusText} /> Date: Thu, 3 Mar 2022 16:05:54 -0800 Subject: [PATCH 02/20] UserStatusScreen [nfc]: Consistently represent "unset" in statusText state We've *said* statusText is `string | null` (changed from `string | void` in our recent model rework, 2106381e4). I would like to make it `string`, to match the `value` prop of the `Input` component type. That seems like a responsible thing for UserStatusScreen to do, since it's using the Input as a controlled input: https://reactjs.org/docs/forms.html#controlled-components Before we do that, we should at least fix this inconsistency, where the value of the input corresponds to "unset" (empty string) but statusText doesn't represent "unset" the way it says it will (null). It hasn't been a live bug, and this is NFC, because it just so happens that api.updateUserStatus converts to the empty string anyway, to talk to the server. --- src/user-statuses/UserStatusScreen.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/user-statuses/UserStatusScreen.js b/src/user-statuses/UserStatusScreen.js index ec2842e0afe..229bbcd83dc 100644 --- a/src/user-statuses/UserStatusScreen.js +++ b/src/user-statuses/UserStatusScreen.js @@ -71,7 +71,7 @@ export default function UserStatusScreen(props: Props): Node { style={styles.statusTextInput} placeholder="What’s your status?" value={statusText ?? ''} - onChangeText={setStatusText} + onChangeText={text => setStatusText(text || null)} /> Date: Thu, 3 Mar 2022 15:10:48 -0800 Subject: [PATCH 03/20] UserStatusScreen [nfc]: Make statusText state a string UserStatusScreen has undertaken to use Input as a controlled input, where at all times it's telling the Input what `value` it should have. https://reactjs.org/docs/forms.html#controlled-components It seems like the responsible thing for UserStatusScreen to do, for clarity, is to at least store the value meant for `value` in a proper state variable. We'll solidify that role with a rename, coming next. --- src/user-statuses/UserStatusScreen.js | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/user-statuses/UserStatusScreen.js b/src/user-statuses/UserStatusScreen.js index 229bbcd83dc..3f99e5d5dce 100644 --- a/src/user-statuses/UserStatusScreen.js +++ b/src/user-statuses/UserStatusScreen.js @@ -43,7 +43,7 @@ export default function UserStatusScreen(props: Props): Node { const ownUserId = useSelector(getOwnUserId); const userStatusText = useSelector(state => getUserStatus(state, ownUserId).status_text); - const [statusText, setStatusText] = useState(userStatusText); + const [statusText, setStatusText] = useState(userStatusText ?? ''); const _ = useContext(TranslationContext); const sendToServer = useCallback( @@ -59,7 +59,7 @@ export default function UserStatusScreen(props: Props): Node { }, [statusText, sendToServer]); const handlePressClear = useCallback(() => { - setStatusText(null); + setStatusText(''); sendToServer({ status_text: null }); }, [sendToServer]); @@ -70,8 +70,8 @@ export default function UserStatusScreen(props: Props): Node { maxLength={60} style={styles.statusTextInput} placeholder="What’s your status?" - value={statusText ?? ''} - onChangeText={text => setStatusText(text || null)} + value={statusText} + onChangeText={setStatusText} /> Date: Thu, 3 Mar 2022 12:32:26 -0800 Subject: [PATCH 04/20] UserStatusScreen [nfc]: Rename input-value state We can drop "status" because it's implied (this is the UserStatusScreen). And it's good to be clear that we're working with the current value of the input, which could be different from the status text that's actually in effect, that the server knows about. --- src/user-statuses/UserStatusScreen.js | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/user-statuses/UserStatusScreen.js b/src/user-statuses/UserStatusScreen.js index 3f99e5d5dce..d4f4b214ba9 100644 --- a/src/user-statuses/UserStatusScreen.js +++ b/src/user-statuses/UserStatusScreen.js @@ -43,7 +43,7 @@ export default function UserStatusScreen(props: Props): Node { const ownUserId = useSelector(getOwnUserId); const userStatusText = useSelector(state => getUserStatus(state, ownUserId).status_text); - const [statusText, setStatusText] = useState(userStatusText ?? ''); + const [textInputValue, setTextInputValue] = useState(userStatusText ?? ''); const _ = useContext(TranslationContext); const sendToServer = useCallback( @@ -55,11 +55,11 @@ export default function UserStatusScreen(props: Props): Node { ); const handlePressUpdate = useCallback(() => { - sendToServer({ status_text: statusText }); - }, [statusText, sendToServer]); + sendToServer({ status_text: textInputValue }); + }, [textInputValue, sendToServer]); const handlePressClear = useCallback(() => { - setStatusText(''); + setTextInputValue(''); sendToServer({ status_text: null }); }, [sendToServer]); @@ -70,8 +70,8 @@ export default function UserStatusScreen(props: Props): Node { maxLength={60} style={styles.statusTextInput} placeholder="What’s your status?" - value={statusText} - onChangeText={setStatusText} + value={textInputValue} + onChangeText={setTextInputValue} /> { - setStatusText(_(itemKey)); + setTextInputValue(_(itemKey)); }} /> )} From 32a127d876e682f39bbaf6afc0ea5a8112575442 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Thu, 3 Mar 2022 15:44:31 -0800 Subject: [PATCH 05/20] UserStatusScreen [nfc]: Pull out status <-> input-value converters These are nice and simple, and we could probably do fine without them. But it sets a good pattern for the emoji input, coming soon. --- src/user-statuses/UserStatusScreen.js | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/src/user-statuses/UserStatusScreen.js b/src/user-statuses/UserStatusScreen.js index d4f4b214ba9..e2598afdfca 100644 --- a/src/user-statuses/UserStatusScreen.js +++ b/src/user-statuses/UserStatusScreen.js @@ -14,6 +14,7 @@ import Screen from '../common/Screen'; import ZulipButton from '../common/ZulipButton'; import { getAuth, getOwnUserId } from '../selectors'; import { getUserStatus } from './userStatusesModel'; +import type { UserStatus } from '../api/modelTypes'; import { IconCancel, IconDone } from '../common/Icons'; import statusSuggestions from './userStatusTextSuggestions'; import * as api from '../api'; @@ -36,6 +37,10 @@ type Props = $ReadOnly<{| route: RouteProp<'user-status', void>, |}>; +const statusTextFromInputValue = (v: string): $PropertyType => v || null; + +const inputValueFromStatusText = (t: $PropertyType): string => t ?? ''; + export default function UserStatusScreen(props: Props): Node { const { navigation } = props; @@ -43,7 +48,9 @@ export default function UserStatusScreen(props: Props): Node { const ownUserId = useSelector(getOwnUserId); const userStatusText = useSelector(state => getUserStatus(state, ownUserId).status_text); - const [textInputValue, setTextInputValue] = useState(userStatusText ?? ''); + const [textInputValue, setTextInputValue] = useState( + inputValueFromStatusText(userStatusText), + ); const _ = useContext(TranslationContext); const sendToServer = useCallback( @@ -55,11 +62,11 @@ export default function UserStatusScreen(props: Props): Node { ); const handlePressUpdate = useCallback(() => { - sendToServer({ status_text: textInputValue }); + sendToServer({ status_text: statusTextFromInputValue(textInputValue) }); }, [textInputValue, sendToServer]); const handlePressClear = useCallback(() => { - setTextInputValue(''); + setTextInputValue(inputValueFromStatusText(null)); sendToServer({ status_text: null }); }, [sendToServer]); @@ -82,7 +89,7 @@ export default function UserStatusScreen(props: Props): Node { key={item} itemKey={item} title={item} - selected={item === textInputValue} + selected={item === statusTextFromInputValue(textInputValue)} onRequestSelectionChange={itemKey => { setTextInputValue(_(itemKey)); }} From 4304dea6f81d2a505f3d0e7c38fac07bc9476d0b Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Thu, 3 Mar 2022 16:34:54 -0800 Subject: [PATCH 06/20] UserStatusScreen: Trim entered text status before interpreting it This seems helpful. It's easy to end up with leading or trailing spaces in a text input, and want them to be treated as though they weren't there. The server may even trim the text status too, I'm not sure. Note that we don't do this by fussing with the actual value of the text input. That'd be easy to do, since we're using Input as a controlled input -- but it would be a misuse of that control. In particular, we'd introduce a bug like the following, on a topic input, fixed in #5098: > Some bug prevented adding a space at the end of what I was typing > specifically on the topic and maybe stream input (the space would > appear and then immediately disappear, I think) ... so I was only > able to create the topic "bog plants" by typing "bogplants" and > then moving my cursor. --- src/user-statuses/UserStatusScreen.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/user-statuses/UserStatusScreen.js b/src/user-statuses/UserStatusScreen.js index e2598afdfca..54d940634ad 100644 --- a/src/user-statuses/UserStatusScreen.js +++ b/src/user-statuses/UserStatusScreen.js @@ -37,7 +37,8 @@ type Props = $ReadOnly<{| route: RouteProp<'user-status', void>, |}>; -const statusTextFromInputValue = (v: string): $PropertyType => v || null; +const statusTextFromInputValue = (v: string): $PropertyType => + v.trim() || null; const inputValueFromStatusText = (t: $PropertyType): string => t ?? ''; From f25b469cc6d6ea692c6335d1cd515152de1c6218 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Wed, 2 Mar 2022 16:07:48 -0800 Subject: [PATCH 07/20] EmojiPickerScreen: Use plain-old navigation.goBack() We don't like using NavigationService (#4417), so this is nice to be able to do. Not *quite* NFC: if we somehow manage to have two consecutive EmojiPickerScreens at the top of the stack, we'll now just pop one of them instead of both, because we lose `navigateBack`'s special logic with its `sameRoutesCount` value. But that logic is designed for `ChatScreen` -- we do expect to have multiple of `ChatScreen`s at the top of the stack sometimes. We don't expect that with `EmojiPickerScreen`s. --- src/emoji/EmojiPickerScreen.js | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/src/emoji/EmojiPickerScreen.js b/src/emoji/EmojiPickerScreen.js index 8284e7e6b99..47b66071347 100644 --- a/src/emoji/EmojiPickerScreen.js +++ b/src/emoji/EmojiPickerScreen.js @@ -7,14 +7,12 @@ import { FlatList } from 'react-native'; import { TranslationContext } from '../boot/TranslationProvider'; import type { RouteProp } from '../react-navigation'; import type { AppNavigationProp } from '../nav/AppNavigator'; -import * as NavigationService from '../nav/NavigationService'; import * as api from '../api'; import Screen from '../common/Screen'; import EmojiRow from './EmojiRow'; import { getFilteredEmojis, reactionTypeFromEmojiType } from './data'; import { useSelector } from '../react-redux'; import { getAuth, getActiveImageEmojiByName } from '../selectors'; -import { navigateBack } from '../nav/navActions'; import * as logging from '../utils/logging'; import { showToast } from '../utils/info'; @@ -24,7 +22,7 @@ type Props = $ReadOnly<{| |}>; export default function EmojiPickerScreen(props: Props): Node { - const { route } = props; + const { navigation, route } = props; const { messageId } = route.params; const _ = useContext(TranslationContext); @@ -46,9 +44,9 @@ export default function EmojiPickerScreen(props: Props): Node { logging.error('Error adding reaction emoji', err); showToast(_('Failed to add reaction')); }); - NavigationService.dispatch(navigateBack()); + navigation.goBack(); }, - [auth, messageId, _], + [auth, messageId, _, navigation], ); const emojiNames = getFilteredEmojis(filter, activeImageEmojiByName); From 7a8208b75643961125bc2fbfdb6af5895b4ede60 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Wed, 2 Mar 2022 16:29:11 -0800 Subject: [PATCH 08/20] EmojiPickerScreen: Put callback in route params --- src/action-sheets/index.js | 14 +++++++-- src/emoji/EmojiPickerScreen.js | 57 ++++++++++++++++++++-------------- src/nav/navActions.js | 7 +++-- 3 files changed, 50 insertions(+), 28 deletions(-) diff --git a/src/action-sheets/index.js b/src/action-sheets/index.js index ba4bc462ac1..cddf00fc110 100644 --- a/src/action-sheets/index.js +++ b/src/action-sheets/index.js @@ -41,6 +41,7 @@ import * as logging from '../utils/logging'; import { getUnreadCountForTopic } from '../unread/unreadModel'; import getIsNotificationEnabled from '../streams/getIsNotificationEnabled'; import { getStreamTopicUrl, getStreamUrl } from '../utils/internalLinks'; +import { reactionTypeFromEmojiType } from '../emoji/data'; // TODO really this belongs in a libdef. export type ShowActionSheetWithOptions = ( @@ -343,8 +344,17 @@ const shareMessage = { const addReaction = { title: 'Add a reaction', errorMessage: 'Failed to add reaction', - action: ({ message }) => { - NavigationService.dispatch(navigateToEmojiPicker(message.id)); + action: ({ auth, message, _ }) => { + NavigationService.dispatch( + navigateToEmojiPicker(({ code, name, type }) => { + api + .emojiReactionAdd(auth, message.id, reactionTypeFromEmojiType(type, name), code, name) + .catch(err => { + logging.error('Error adding reaction emoji', err); + showToast(_('Failed to add reaction')); + }); + }), + ); }, }; diff --git a/src/emoji/EmojiPickerScreen.js b/src/emoji/EmojiPickerScreen.js index 47b66071347..66f47454b63 100644 --- a/src/emoji/EmojiPickerScreen.js +++ b/src/emoji/EmojiPickerScreen.js @@ -1,34 +1,50 @@ /* @flow strict-local */ -import React, { useState, useCallback, useContext } from 'react'; +import React, { useState, useCallback } from 'react'; import type { Node } from 'react'; -import { FlatList } from 'react-native'; +import { FlatList, LogBox } from 'react-native'; -import { TranslationContext } from '../boot/TranslationProvider'; import type { RouteProp } from '../react-navigation'; import type { AppNavigationProp } from '../nav/AppNavigator'; -import * as api from '../api'; import Screen from '../common/Screen'; import EmojiRow from './EmojiRow'; -import { getFilteredEmojis, reactionTypeFromEmojiType } from './data'; +import { getFilteredEmojis } from './data'; +import { getActiveImageEmojiByName } from '../selectors'; +import type { EmojiType } from '../types'; import { useSelector } from '../react-redux'; -import { getAuth, getActiveImageEmojiByName } from '../selectors'; -import * as logging from '../utils/logging'; -import { showToast } from '../utils/info'; type Props = $ReadOnly<{| navigation: AppNavigationProp<'emoji-picker'>, - route: RouteProp<'emoji-picker', {| messageId: number |}>, + route: RouteProp< + 'emoji-picker', + {| + // This param is a function, so React Nav is right to point out that + // it isn't serializable. But this is fine as long as we don't try to + // persist navigation state for this screen or set up deep linking to + // it, hence the LogBox suppression below. + // + // React Navigation doesn't offer a more sensible way to do have us + // pass the emoji data to the calling screen. …We could store the + // emoji data as a route param on the calling screen, or in Redux. But + // from this screen's perspective, that's basically just setting a + // global variable. Better to offer this explicit, side-effect-free + // way for the data to flow where it should, when it should. + onPressEmoji: ({| +type: EmojiType, +code: string, +name: string |}) => void, + |}, + >, |}>; +// React Navigation would give us a console warning about non-serializable +// route params. For more about the warning, see +// https://reactnavigation.org/docs/5.x/troubleshooting/#i-get-the-warning-non-serializable-values-were-found-in-the-navigation-state +// See comment on this param, above. +LogBox.ignoreLogs([/emoji-picker > params\.onPressEmoji \(Function\)/]); + export default function EmojiPickerScreen(props: Props): Node { const { navigation, route } = props; - const { messageId } = route.params; - - const _ = useContext(TranslationContext); + const { onPressEmoji } = route.params; const activeImageEmojiByName = useSelector(getActiveImageEmojiByName); - const auth = useSelector(getAuth); const [filter, setFilter] = useState(''); @@ -36,17 +52,12 @@ export default function EmojiPickerScreen(props: Props): Node { setFilter(text.toLowerCase()); }, []); - const addReaction = useCallback( - ({ type, code, name }) => { - api - .emojiReactionAdd(auth, messageId, reactionTypeFromEmojiType(type, name), code, name) - .catch(err => { - logging.error('Error adding reaction emoji', err); - showToast(_('Failed to add reaction')); - }); + const handlePressEmoji = useCallback( + (...args) => { + onPressEmoji(...args); navigation.goBack(); }, - [auth, messageId, _, navigation], + [onPressEmoji, navigation], ); const emojiNames = getFilteredEmojis(filter, activeImageEmojiByName); @@ -63,7 +74,7 @@ export default function EmojiPickerScreen(props: Props): Node { type={item.emoji_type} code={item.code} name={item.name} - onPress={addReaction} + onPress={handlePressEmoji} /> )} /> diff --git a/src/nav/navActions.js b/src/nav/navActions.js index 6d7135dff51..ebfcf2ac94e 100644 --- a/src/nav/navActions.js +++ b/src/nav/navActions.js @@ -7,7 +7,7 @@ import { } from '@react-navigation/native'; import * as NavigationService from './NavigationService'; -import type { Message, Narrow, UserId } from '../types'; +import type { Message, Narrow, UserId, EmojiType } from '../types'; import type { PmKeyRecipients } from '../utils/recipient'; import type { SharedData } from '../sharing/types'; import type { ApiResponseServerSettings } from '../api/settings/getServerSettings'; @@ -52,8 +52,9 @@ export const navigateToUsersScreen = (): GenericNavigationAction => StackActions export const navigateToSearch = (): GenericNavigationAction => StackActions.push('search-messages'); -export const navigateToEmojiPicker = (messageId: number): GenericNavigationAction => - StackActions.push('emoji-picker', { messageId }); +export const navigateToEmojiPicker = ( + onPressEmoji: ({| +type: EmojiType, +code: string, +name: string |}) => void, +): GenericNavigationAction => StackActions.push('emoji-picker', { onPressEmoji }); export const navigateToAuth = ( serverSettings: ApiResponseServerSettings, From 96901e31980f974c2f305afa1f59887ec9a8540d Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Thu, 3 Mar 2022 09:56:21 -0800 Subject: [PATCH 09/20] Emoji [nfc]: Allow setting size --- src/emoji/Emoji.js | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/src/emoji/Emoji.js b/src/emoji/Emoji.js index c35fd2ac948..b879ad5ddb2 100644 --- a/src/emoji/Emoji.js +++ b/src/emoji/Emoji.js @@ -1,5 +1,5 @@ /* @flow strict-local */ -import React from 'react'; +import React, { useMemo } from 'react'; import type { Node } from 'react'; import { Image } from 'react-native'; import { createIconSet } from 'react-native-vector-icons'; @@ -18,19 +18,20 @@ const UnicodeEmoji = createIconSet(codeToEmojiMap); type Props = $ReadOnly<{| type: EmojiType, code: string, + size?: number, |}>; -const componentStyles = createStyleSheet({ - image: { width: 20, height: 20 }, -}); - export default function Emoji(props: Props): Node { - const { code } = props; + const { code, size = 20 } = props; const imageEmoji = useSelector(state => props.type === 'image' ? getAllImageEmojiByCode(state)[props.code] : undefined, ); + const componentStyles = useMemo( + () => createStyleSheet({ image: { width: size, height: size } }), + [size], + ); if (imageEmoji) { return ; } - return ; + return ; } From bbba24688f3d5a6262bb32bb9f48e9439de87350 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Thu, 3 Mar 2022 12:50:18 -0800 Subject: [PATCH 10/20] UserStatusScreen [nfc]: Add "input row" View, to soon include emoji input For now, just leave the status-text input by itself, filling all of the new View. And move its margin to the new View; we'll want it to apply to the emoji input too, coming soon. --- src/user-statuses/UserStatusScreen.js | 27 ++++++++++++++++++--------- 1 file changed, 18 insertions(+), 9 deletions(-) diff --git a/src/user-statuses/UserStatusScreen.js b/src/user-statuses/UserStatusScreen.js index 54d940634ad..36c9f6ca81b 100644 --- a/src/user-statuses/UserStatusScreen.js +++ b/src/user-statuses/UserStatusScreen.js @@ -20,9 +20,13 @@ import statusSuggestions from './userStatusTextSuggestions'; import * as api from '../api'; const styles = createStyleSheet({ - statusTextInput: { + inputRow: { + flexDirection: 'row', margin: 16, }, + statusTextInput: { + flex: 1, + }, buttonsWrapper: { flexDirection: 'row', }, @@ -73,14 +77,19 @@ export default function UserStatusScreen(props: Props): Node { return ( - + + { + // TODO: Input for emoji status + } + + Date: Mon, 7 Mar 2022 13:38:34 -0800 Subject: [PATCH 11/20] UserStatusScreen [nfc]: Move `useContext` upward, with selectors As Greg suggests: https://github.com/zulip/zulip-mobile/pull/5277#discussion_r820978534 --- src/user-statuses/UserStatusScreen.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/user-statuses/UserStatusScreen.js b/src/user-statuses/UserStatusScreen.js index 36c9f6ca81b..0b13f649eeb 100644 --- a/src/user-statuses/UserStatusScreen.js +++ b/src/user-statuses/UserStatusScreen.js @@ -49,6 +49,7 @@ const inputValueFromStatusText = (t: $PropertyType): export default function UserStatusScreen(props: Props): Node { const { navigation } = props; + const _ = useContext(TranslationContext); const auth = useSelector(getAuth); const ownUserId = useSelector(getOwnUserId); const userStatusText = useSelector(state => getUserStatus(state, ownUserId).status_text); @@ -56,7 +57,6 @@ export default function UserStatusScreen(props: Props): Node { const [textInputValue, setTextInputValue] = useState( inputValueFromStatusText(userStatusText), ); - const _ = useContext(TranslationContext); const sendToServer = useCallback( partialUserStatus => { From 261341ca6aa8188d54266bd5171f03e78125b64b Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Mon, 7 Mar 2022 14:04:54 -0800 Subject: [PATCH 12/20] UserStatusScreen: Translate status-text suggestions --- src/user-statuses/UserStatusScreen.js | 24 +++++++++++++----------- 1 file changed, 13 insertions(+), 11 deletions(-) diff --git a/src/user-statuses/UserStatusScreen.js b/src/user-statuses/UserStatusScreen.js index 0b13f649eeb..ebc8e7f8b14 100644 --- a/src/user-statuses/UserStatusScreen.js +++ b/src/user-statuses/UserStatusScreen.js @@ -94,17 +94,19 @@ export default function UserStatusScreen(props: Props): Node { data={statusSuggestions} keyboardShouldPersistTaps="always" keyExtractor={item => item} - renderItem={({ item, index }) => ( - { - setTextInputValue(_(itemKey)); - }} - /> - )} + renderItem={({ item: text, index }) => { + const translatedText = _(text); + return ( + { + setTextInputValue(translatedText); + }} + /> + ); + }} /> Date: Mon, 7 Mar 2022 17:15:26 -0800 Subject: [PATCH 13/20] AccountDetails: Show status emoji, if set --- src/account-info/AccountDetails.js | 25 ++++++++++++++++++++++--- 1 file changed, 22 insertions(+), 3 deletions(-) diff --git a/src/account-info/AccountDetails.js b/src/account-info/AccountDetails.js index dbb1b04d03c..cb26b54be63 100644 --- a/src/account-info/AccountDetails.js +++ b/src/account-info/AccountDetails.js @@ -3,6 +3,8 @@ import React from 'react'; import type { Node } from 'react'; import { View } from 'react-native'; +import Emoji from '../emoji/Emoji'; +import { emojiTypeFromReactionType } from '../emoji/data'; import type { UserOrBot } from '../types'; import styles, { createStyleSheet } from '../styles'; import { useSelector } from '../react-redux'; @@ -21,6 +23,7 @@ const componentStyles = createStyleSheet({ }, statusWrapper: { justifyContent: 'center', + alignItems: 'center', flexDirection: 'row', }, presenceStatusIndicator: { @@ -42,6 +45,9 @@ export default function AccountDetails(props: Props): Node { const ownUser = useSelector(getOwnUser); const userStatusText = useSelector(state => getUserStatus(state, props.user.user_id).status_text); + const userStatusEmoji = useSelector( + state => getUserStatus(state, props.user.user_id).status_emoji, + ); const isSelf = user.user_id === ownUser.user_id; @@ -70,9 +76,22 @@ export default function AccountDetails(props: Props): Node { /> - {userStatusText !== null && ( - - )} + + {userStatusEmoji && ( + + )} + {userStatusEmoji && userStatusText !== null && } + {userStatusText !== null && ( + + )} + {!isSelf && ( From 254b00f219d3c7dea7acebf9c16946372fa673e0 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Wed, 2 Mar 2022 12:33:32 -0800 Subject: [PATCH 14/20] UserStatusScreen: Add and use EmojiInput, for status emoji Taking care not to send the status-emoji params to the server if the server doesn't support status emoji. --- src/user-statuses/EmojiInput.js | 89 +++++++++++++++++++++++++++ src/user-statuses/UserStatusScreen.js | 62 ++++++++++++++++--- 2 files changed, 142 insertions(+), 9 deletions(-) create mode 100644 src/user-statuses/EmojiInput.js diff --git a/src/user-statuses/EmojiInput.js b/src/user-statuses/EmojiInput.js new file mode 100644 index 00000000000..f08ef4d4a46 --- /dev/null +++ b/src/user-statuses/EmojiInput.js @@ -0,0 +1,89 @@ +/* @flow strict-local */ +import React, { useContext, useCallback, useMemo } from 'react'; +import type { Node } from 'react'; +import { Platform } from 'react-native'; +import type { AppNavigationProp } from '../nav/AppNavigator'; + +import { ThemeContext } from '../styles/theme'; +import Touchable from '../common/Touchable'; +import Emoji from '../emoji/Emoji'; +import { Icon } from '../common/Icons'; +import type { EmojiType } from '../types'; +import { createStyleSheet, BORDER_COLOR } from '../styles'; + +export type Value = null | {| +type: EmojiType, +code: string, +name: string |}; + +export type Props = $ReadOnly<{| + value: Value, + onChangeValue: Value => void, + + /** + * Component must be under the stack nav that has the emoji-picker screen + * + * Pass this down from props or `useNavigation`. + */ + navigation: AppNavigationProp<>, + + /** + * Give appropriate right margin + */ + rightMargin?: true, +|}>; + +/** + * A controlled input component to let the user choose an emoji. + * + * When pressed, opens the emoji-picker screen, and populates with the emoji + * chosen by the user, if any. + * + * Designed for harmony with our Input component. If changing the appearance + * of this or that component, we should try to keep that harmony. + */ +export default function EmojiInput(props: Props): Node { + const { value, onChangeValue, navigation, rightMargin } = props; + + const { color } = useContext(ThemeContext); + + const handlePress = useCallback(() => { + navigation.push('emoji-picker', { onPressEmoji: onChangeValue }); + }, [navigation, onChangeValue]); + + const styles = useMemo( + () => + createStyleSheet({ + touchable: { + // Min touch-target size + minWidth: 48, + minHeight: 48, + + alignItems: 'center', + justifyContent: 'center', + + marginRight: rightMargin ? 4 : undefined, + + // For harmony with the `Input` component, which differs between + // platforms because of platform conventions. Border on iOS, no + // border on Android. + ...(Platform.OS === 'ios' + ? { + borderWidth: 1, + borderColor: BORDER_COLOR, + borderRadius: 2, + padding: 8, + } + : Object.freeze({})), + }, + }), + [rightMargin], + ); + + return ( + + {value ? ( + + ) : ( + + )} + + ); +} diff --git a/src/user-statuses/UserStatusScreen.js b/src/user-statuses/UserStatusScreen.js index ebc8e7f8b14..35e7be833e6 100644 --- a/src/user-statuses/UserStatusScreen.js +++ b/src/user-statuses/UserStatusScreen.js @@ -9,10 +9,13 @@ import type { RouteProp } from '../react-navigation'; import type { AppNavigationProp } from '../nav/AppNavigator'; import { useSelector } from '../react-redux'; import Input from '../common/Input'; +import EmojiInput from './EmojiInput'; +import type { Value as EmojiInputValue } from './EmojiInput'; +import { emojiTypeFromReactionType, reactionTypeFromEmojiType } from '../emoji/data'; import SelectableOptionRow from '../common/SelectableOptionRow'; import Screen from '../common/Screen'; import ZulipButton from '../common/ZulipButton'; -import { getAuth, getOwnUserId } from '../selectors'; +import { getZulipFeatureLevel, getAuth, getOwnUserId } from '../selectors'; import { getUserStatus } from './userStatusesModel'; import type { UserStatus } from '../api/modelTypes'; import { IconCancel, IconDone } from '../common/Icons'; @@ -46,41 +49,82 @@ const statusTextFromInputValue = (v: string): $PropertyType): string => t ?? ''; +const statusEmojiFromInputValue = (v: EmojiInputValue): $PropertyType => + v + ? { + emoji_name: v.name, + emoji_code: v.code, + reaction_type: reactionTypeFromEmojiType(v.type, v.name), + } + : null; + +const inputValueFromStatusEmoji = (e: $PropertyType): EmojiInputValue => + e + ? { + type: emojiTypeFromReactionType(e.reaction_type), + code: e.emoji_code, + name: e.emoji_name, + } + : null; + export default function UserStatusScreen(props: Props): Node { const { navigation } = props; + // TODO(server-5.0): Cut conditionals on emoji-status support (emoji + // supported as of FL 86: https://zulip.com/api/changelog ) + const serverSupportsEmojiStatus = useSelector(getZulipFeatureLevel) >= 86; + const _ = useContext(TranslationContext); const auth = useSelector(getAuth); const ownUserId = useSelector(getOwnUserId); const userStatusText = useSelector(state => getUserStatus(state, ownUserId).status_text); + const userStatusEmoji = useSelector(state => getUserStatus(state, ownUserId).status_emoji); const [textInputValue, setTextInputValue] = useState( inputValueFromStatusText(userStatusText), ); + const [emojiInputValue, setEmojiInputValue] = useState( + inputValueFromStatusEmoji(userStatusEmoji), + ); const sendToServer = useCallback( partialUserStatus => { - api.updateUserStatus(auth, partialUserStatus); + const copy = { ...partialUserStatus }; + // TODO: Put conditional inside `api.updateUserStatus` itself; see + // https://github.com/zulip/zulip-mobile/issues/4659#issuecomment-914996061 + if (!serverSupportsEmojiStatus) { + delete copy.status_emoji; + } + api.updateUserStatus(auth, copy); navigation.goBack(); }, - [navigation, auth], + [serverSupportsEmojiStatus, navigation, auth], ); const handlePressUpdate = useCallback(() => { - sendToServer({ status_text: statusTextFromInputValue(textInputValue) }); - }, [textInputValue, sendToServer]); + sendToServer({ + status_text: statusTextFromInputValue(textInputValue), + status_emoji: statusEmojiFromInputValue(emojiInputValue), + }); + }, [textInputValue, emojiInputValue, sendToServer]); const handlePressClear = useCallback(() => { setTextInputValue(inputValueFromStatusText(null)); - sendToServer({ status_text: null }); + setEmojiInputValue(inputValueFromStatusEmoji(null)); + sendToServer({ status_text: null, status_emoji: null }); }, [sendToServer]); return ( - { - // TODO: Input for emoji status - } + {serverSupportsEmojiStatus && ( + + )} Date: Thu, 3 Mar 2022 13:53:22 -0800 Subject: [PATCH 15/20] UserStatusScreen: Move and re-style "clear" button This doesn't yet change the main behavior of the button when pressed, i.e., we still clear the inputs *and* tell the server that we want to unset the status. Soon, we'll have it match the web app by only clearing the inputs. Then the user can apply the change with the submit button, which we'll rename from "Update" to "Save". --- src/user-statuses/UserStatusScreen.js | 49 +++++++++++++++------------ 1 file changed, 27 insertions(+), 22 deletions(-) diff --git a/src/user-statuses/UserStatusScreen.js b/src/user-statuses/UserStatusScreen.js index 35e7be833e6..79de06411a8 100644 --- a/src/user-statuses/UserStatusScreen.js +++ b/src/user-statuses/UserStatusScreen.js @@ -1,10 +1,10 @@ /* @flow strict-local */ import React, { useState, useContext, useCallback } from 'react'; import type { Node } from 'react'; -import { FlatList, View } from 'react-native'; -import { TranslationContext } from '../boot/TranslationProvider'; -import { createStyleSheet } from '../styles'; +import { FlatList, View, Pressable } from 'react-native'; +import { TranslationContext } from '../boot/TranslationProvider'; +import { createStyleSheet, BRAND_COLOR, HIGHLIGHT_COLOR } from '../styles'; import type { RouteProp } from '../react-navigation'; import type { AppNavigationProp } from '../nav/AppNavigator'; import { useSelector } from '../react-redux'; @@ -18,7 +18,7 @@ import ZulipButton from '../common/ZulipButton'; import { getZulipFeatureLevel, getAuth, getOwnUserId } from '../selectors'; import { getUserStatus } from './userStatusesModel'; import type { UserStatus } from '../api/modelTypes'; -import { IconCancel, IconDone } from '../common/Icons'; +import { Icon, IconDone } from '../common/Icons'; import statusSuggestions from './userStatusTextSuggestions'; import * as api from '../api'; @@ -30,11 +30,18 @@ const styles = createStyleSheet({ statusTextInput: { flex: 1, }, - buttonsWrapper: { - flexDirection: 'row', + clearButton: { + // Min touch-target size + minWidth: 48, + minHeight: 48, + + alignItems: 'center', + justifyContent: 'center', + + // To match margin between the emoji and text inputs + marginLeft: 4, }, button: { - flex: 1, margin: 8, }, }); @@ -133,6 +140,13 @@ export default function UserStatusScreen(props: Props): Node { value={textInputValue} onChangeText={setTextInputValue} /> + {(emojiInputValue !== null || textInputValue.length > 0) && ( + + {({ pressed }) => ( + + )} + + )} - - - - + ); } From 5b0e2f80f0263295950fea50226df68c6f1a5219 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Thu, 3 Mar 2022 14:06:07 -0800 Subject: [PATCH 16/20] UserStatusScreen: Have clear button only clear the inputs, not save too --- src/user-statuses/UserStatusScreen.js | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/user-statuses/UserStatusScreen.js b/src/user-statuses/UserStatusScreen.js index 79de06411a8..7658282f76f 100644 --- a/src/user-statuses/UserStatusScreen.js +++ b/src/user-statuses/UserStatusScreen.js @@ -118,8 +118,7 @@ export default function UserStatusScreen(props: Props): Node { const handlePressClear = useCallback(() => { setTextInputValue(inputValueFromStatusText(null)); setEmojiInputValue(inputValueFromStatusEmoji(null)); - sendToServer({ status_text: null, status_emoji: null }); - }, [sendToServer]); + }, []); return ( From 5100d573e2c2e2da21c70fa646550248db962527 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Thu, 3 Mar 2022 14:02:59 -0800 Subject: [PATCH 17/20] UserStatusScreen: Rename "Update" button to "Save"; remove checkmark icon I think this is much more common wording. And the checkmark icon is potentially confusing because it looks just like the checkmark on one of the status suggestions (SelectableOptionRow), if you've selected one. And it just doesn't feel normal to have an icon at all for a simple "OK" button like this. --- src/user-statuses/UserStatusScreen.js | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/src/user-statuses/UserStatusScreen.js b/src/user-statuses/UserStatusScreen.js index 7658282f76f..a0882ae5724 100644 --- a/src/user-statuses/UserStatusScreen.js +++ b/src/user-statuses/UserStatusScreen.js @@ -18,7 +18,7 @@ import ZulipButton from '../common/ZulipButton'; import { getZulipFeatureLevel, getAuth, getOwnUserId } from '../selectors'; import { getUserStatus } from './userStatusesModel'; import type { UserStatus } from '../api/modelTypes'; -import { Icon, IconDone } from '../common/Icons'; +import { Icon } from '../common/Icons'; import statusSuggestions from './userStatusTextSuggestions'; import * as api from '../api'; @@ -108,7 +108,7 @@ export default function UserStatusScreen(props: Props): Node { [serverSupportsEmojiStatus, navigation, auth], ); - const handlePressUpdate = useCallback(() => { + const handlePressSave = useCallback(() => { sendToServer({ status_text: statusTextFromInputValue(textInputValue), status_emoji: statusEmojiFromInputValue(emojiInputValue), @@ -165,12 +165,7 @@ export default function UserStatusScreen(props: Props): Node { ); }} /> - + ); } From 977d59f0a278f6e0f2f1c534c53c5f50ece2b837 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Thu, 3 Mar 2022 16:47:41 -0800 Subject: [PATCH 18/20] UserStatusScreen: Disable "Save" button when status unchanged This is another place where it's nice to use our status <-> input-value conversion functions. The logic is not complicated -- "Has the emoji or text been touched?" -- so it's nice to see this code doesn't have ||, ??, and () ? () : () all over the place. --- src/user-statuses/UserStatusScreen.js | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/src/user-statuses/UserStatusScreen.js b/src/user-statuses/UserStatusScreen.js index a0882ae5724..869ebf8df11 100644 --- a/src/user-statuses/UserStatusScreen.js +++ b/src/user-statuses/UserStatusScreen.js @@ -1,4 +1,6 @@ /* @flow strict-local */ +// $FlowFixMe[untyped-import] +import isEqual from 'lodash.isequal'; import React, { useState, useContext, useCallback } from 'react'; import type { Node } from 'react'; import { FlatList, View, Pressable } from 'react-native'; @@ -165,7 +167,15 @@ export default function UserStatusScreen(props: Props): Node { ); }} /> - + ); } From ed6e6e1af330ee70cbe8919524f6519a7679ce74 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Thu, 3 Mar 2022 17:18:32 -0800 Subject: [PATCH 19/20] UserStatusScreen: Make status suggestions use new emoji-status feature! MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When available, that is. Support was added in FL 86. If emoji statuses are supported, we use the suggested emoji for the emoji status, and the suggested text for the text status. If emoji statuses aren't supported…we do end up just cutting the emoji out of the status suggestion, which I guess is a small downside. For these older servers, we could have kept up what we do now: stick the status emoji at the start of the suggested text. But I think achieving that has a higher complexity cost than it's worth. I've never really used this UI, I'm not sure how common it is to do so, and people are still free to depart from the suggested text, including by adding emojis, by using the text input. --- src/user-statuses/UserStatusScreen.js | 42 +++++++++++++++---- .../userStatusTextSuggestions.js | 11 ----- static/translations/messages_en.json | 10 ++--- 3 files changed, 40 insertions(+), 23 deletions(-) delete mode 100644 src/user-statuses/userStatusTextSuggestions.js diff --git a/src/user-statuses/UserStatusScreen.js b/src/user-statuses/UserStatusScreen.js index 869ebf8df11..3410c87c1ea 100644 --- a/src/user-statuses/UserStatusScreen.js +++ b/src/user-statuses/UserStatusScreen.js @@ -13,7 +13,12 @@ import { useSelector } from '../react-redux'; import Input from '../common/Input'; import EmojiInput from './EmojiInput'; import type { Value as EmojiInputValue } from './EmojiInput'; -import { emojiTypeFromReactionType, reactionTypeFromEmojiType } from '../emoji/data'; +import { unicodeCodeByName } from '../emoji/codePointMap'; +import { + emojiTypeFromReactionType, + reactionTypeFromEmojiType, + parseUnicodeEmojiCode, +} from '../emoji/data'; import SelectableOptionRow from '../common/SelectableOptionRow'; import Screen from '../common/Screen'; import ZulipButton from '../common/ZulipButton'; @@ -21,9 +26,24 @@ import { getZulipFeatureLevel, getAuth, getOwnUserId } from '../selectors'; import { getUserStatus } from './userStatusesModel'; import type { UserStatus } from '../api/modelTypes'; import { Icon } from '../common/Icons'; -import statusSuggestions from './userStatusTextSuggestions'; import * as api from '../api'; +type StatusSuggestion = [ + $ReadOnly<{| emoji_name: string, emoji_code: string, reaction_type: 'unicode_emoji' |}>, + string, +]; + +const statusSuggestions: $ReadOnlyArray = [ + ['calendar', 'In a meeting'], + ['bus', 'Commuting'], + ['sick', 'Out sick'], + ['palm_tree', 'Vacationing'], + ['house', 'Working remotely'], +].map(([emoji_name, status_text]) => [ + { emoji_name, emoji_code: unicodeCodeByName[emoji_name], reaction_type: 'unicode_emoji' }, + status_text, +]); + const styles = createStyleSheet({ inputRow: { flexDirection: 'row', @@ -152,16 +172,24 @@ export default function UserStatusScreen(props: Props): Node { item} - renderItem={({ item: text, index }) => { + keyExtractor={(item, index) => index.toString() /* list is constant; index OK */} + renderItem={({ item: [emoji, text], index }) => { const translatedText = _(text); return ( { setTextInputValue(translatedText); + setEmojiInputValue(inputValueFromStatusEmoji(emoji)); }} /> ); diff --git a/src/user-statuses/userStatusTextSuggestions.js b/src/user-statuses/userStatusTextSuggestions.js deleted file mode 100644 index d0137f52511..00000000000 --- a/src/user-statuses/userStatusTextSuggestions.js +++ /dev/null @@ -1,11 +0,0 @@ -/* @flow strict-local */ - -// This is a separate file due to Prettier having issues with formatting unicode -// Using `prettier-ignore` does not help. -export default [ - '📅 In a meeting', - '🚌 Commuting', - '🤒 Out sick', - '🌴 Vacationing', - '🏠 Working remotely', -]; diff --git a/static/translations/messages_en.json b/static/translations/messages_en.json index 40109d23206..9e33e250c80 100644 --- a/static/translations/messages_en.json +++ b/static/translations/messages_en.json @@ -274,11 +274,11 @@ "Couldn’t load information about {fullName}": "Couldn’t load information about {fullName}", "What’s your status?": "What’s your status?", "Click to join video call": "Click to join video call", - "📅 In a meeting": "📅 In a meeting", - "🚌 Commuting": "🚌 Commuting", - "🤒 Out sick": "🤒 Out sick", - "🌴 Vacationing": "🌴 Vacationing", - "🏠 Working remotely": "🏠 Working remotely", + "In a meeting": "In a meeting", + "Commuting": "Commuting", + "Out sick": "Out sick", + "Vacationing": "Vacationing", + "Working remotely": "Working remotely", "This message was hidden because it is from a user you have muted. Long-press to view.": "This message was hidden because it is from a user you have muted. Long-press to view.", "Signed out": "Signed out", "Remove account?": "Remove account?", From 6d62335351c2e19ec547bd9fe6648715311313e5 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Fri, 4 Mar 2022 17:56:34 -0800 Subject: [PATCH 20/20] UserStatusScreen: Add "Busy" option Fixes: #5255 --- src/user-statuses/UserStatusScreen.js | 1 + static/translations/messages_en.json | 1 + 2 files changed, 2 insertions(+) diff --git a/src/user-statuses/UserStatusScreen.js b/src/user-statuses/UserStatusScreen.js index 3410c87c1ea..c471432c078 100644 --- a/src/user-statuses/UserStatusScreen.js +++ b/src/user-statuses/UserStatusScreen.js @@ -34,6 +34,7 @@ type StatusSuggestion = [ ]; const statusSuggestions: $ReadOnlyArray = [ + ['working_on_it', 'Busy'], ['calendar', 'In a meeting'], ['bus', 'Commuting'], ['sick', 'Out sick'], diff --git a/static/translations/messages_en.json b/static/translations/messages_en.json index 9e33e250c80..01b750f5807 100644 --- a/static/translations/messages_en.json +++ b/static/translations/messages_en.json @@ -274,6 +274,7 @@ "Couldn’t load information about {fullName}": "Couldn’t load information about {fullName}", "What’s your status?": "What’s your status?", "Click to join video call": "Click to join video call", + "Busy": "Busy", "In a meeting": "In a meeting", "Commuting": "Commuting", "Out sick": "Out sick",