From 9318bf327f2aa23f318d9fc2946791d7f14b1334 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Wed, 17 Aug 2022 17:44:48 -0700 Subject: [PATCH 1/6] InputRowRadioButtons: Use theme color for IconRight, like NestedNavRow --- src/common/InputRowRadioButtons.js | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/common/InputRowRadioButtons.js b/src/common/InputRowRadioButtons.js index 60c1d2b11d5..6a21c36d5e6 100644 --- a/src/common/InputRowRadioButtons.js +++ b/src/common/InputRowRadioButtons.js @@ -1,5 +1,5 @@ /* @flow strict-local */ -import React, { useCallback, useRef, useMemo, useEffect } from 'react'; +import React, { useCallback, useRef, useMemo, useEffect, useContext } from 'react'; import type { Node } from 'react'; import { View } from 'react-native'; import invariant from 'invariant'; @@ -8,11 +8,12 @@ import { CommonActions } from '@react-navigation/native'; import type { LocalizableText } from '../types'; import type { GlobalParamList } from '../nav/globalTypes'; import { randString } from '../utils/misc'; -import { BRAND_COLOR, createStyleSheet } from '../styles'; +import { createStyleSheet } from '../styles'; import Touchable from './Touchable'; import ZulipTextIntl from './ZulipTextIntl'; import { IconRight } from './Icons'; import type { AppNavigationMethods } from '../nav/AppNavigator'; +import { ThemeContext } from '../styles/theme'; type Item = $ReadOnly<{| key: TKey, @@ -128,6 +129,7 @@ export default function InputRowRadioButtons(props: Props createStyleSheet({ @@ -164,7 +166,7 @@ export default function InputRowRadioButtons(props: Props - + From f39fcad18a4cd3fc74d08af29361921c58846032 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Wed, 17 Aug 2022 17:48:23 -0700 Subject: [PATCH 2/6] InputRowRadioButtons: Set minHeight: 48, since this is a touch target --- src/common/InputRowRadioButtons.js | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/common/InputRowRadioButtons.js b/src/common/InputRowRadioButtons.js index 6a21c36d5e6..02d6dcc47b5 100644 --- a/src/common/InputRowRadioButtons.js +++ b/src/common/InputRowRadioButtons.js @@ -138,6 +138,10 @@ export default function InputRowRadioButtons(props: Props Date: Wed, 17 Aug 2022 17:42:35 -0700 Subject: [PATCH 3/6] settings: Add option to mark read on scroll only in conversation views Using the handy new InputRowRadioButtons component we added in edf53220f. Fixes: #5241 --- src/common/InputRowRadioButtons.js | 2 +- src/common/SwitchRow.js | 4 +- src/reduxTypes.js | 2 +- src/settings/SettingsScreen.js | 25 ++++++++--- src/settings/settingsReducer.js | 2 +- src/storage/__tests__/migrations-test.js | 43 +++++++++++++++++-- src/storage/migrations.js | 17 ++++++++ src/webview/MessageList.js | 18 +++++++- .../__tests__/generateInboundEvents-test.js | 2 +- static/translations/messages_en.json | 7 ++- 10 files changed, 103 insertions(+), 19 deletions(-) diff --git a/src/common/InputRowRadioButtons.js b/src/common/InputRowRadioButtons.js index 02d6dcc47b5..2adc7b8c80b 100644 --- a/src/common/InputRowRadioButtons.js +++ b/src/common/InputRowRadioButtons.js @@ -58,7 +58,7 @@ type Props = $ReadOnly<{| |}>; /** - * A form-input row for the user to make a choice, radio-button style. + * An input row for the user to make a choice, radio-button style. * * Shows the current value (the selected item), represented as the item's * `.title`. When tapped, opens the selectable-options screen, where the diff --git a/src/common/SwitchRow.js b/src/common/SwitchRow.js index e9f5c90381b..a00b0681333 100644 --- a/src/common/SwitchRow.js +++ b/src/common/SwitchRow.js @@ -18,8 +18,8 @@ type Props = $ReadOnly<{| const componentStyles = createStyleSheet({ container: { // For uniformity with other rows this might share a screen with, e.g., - // NestedNavRow on the settings screen. See height-related attributes on - // those rows. + // NestedNavRow and InputRowRadioButtons on the settings screen. See + // height-related attributes on those rows. minHeight: 48, }, icon: { diff --git a/src/reduxTypes.js b/src/reduxTypes.js index a84abaf4609..9d2f3d1c171 100644 --- a/src/reduxTypes.js +++ b/src/reduxTypes.js @@ -397,7 +397,7 @@ export type GlobalSettingsState = $ReadOnly<{ // Possibly this should be per-account. If so it should probably be put // on the server, so it can also be cross-device for the account. - doNotMarkMessagesAsRead: boolean, + markMessagesReadOnScroll: 'always' | 'never' | 'conversation-views-only', ... }>; diff --git a/src/settings/SettingsScreen.js b/src/settings/SettingsScreen.js index 915bb0ae50d..bf1a5a2f061 100644 --- a/src/settings/SettingsScreen.js +++ b/src/settings/SettingsScreen.js @@ -8,6 +8,7 @@ import type { AppNavigationProp } from '../nav/AppNavigator'; import { useGlobalSelector, useDispatch } from '../react-redux'; import { getGlobalSettings } from '../selectors'; import NestedNavRow from '../common/NestedNavRow'; +import InputRowRadioButtons from '../common/InputRowRadioButtons'; import SwitchRow from '../common/SwitchRow'; import Screen from '../common/Screen'; import { @@ -27,8 +28,8 @@ type Props = $ReadOnly<{| export default function SettingsScreen(props: Props): Node { const theme = useGlobalSelector(state => getGlobalSettings(state).theme); const browser = useGlobalSelector(state => getGlobalSettings(state).browser); - const doNotMarkMessagesAsRead = useGlobalSelector( - state => getGlobalSettings(state).doNotMarkMessagesAsRead, + const markMessagesReadOnScroll = useGlobalSelector( + state => getGlobalSettings(state).markMessagesReadOnScroll, ); const dispatch = useDispatch(); const { navigation } = props; @@ -47,11 +48,23 @@ export default function SettingsScreen(props: Props): Node { dispatch(setGlobalSettings({ browser: value ? 'embedded' : 'external' })); }} /> - { - dispatch(setGlobalSettings({ doNotMarkMessagesAsRead: value })); + dispatch(setGlobalSettings({ markMessagesReadOnScroll: value })); }} /> { }, }; + // What `base` becomes after migrations up through 52. + const base52 = { + ...base37, + migrations: { version: 52 }, + settings: { + language: 'en', + theme: 'default', + offlineNotification: true, + onlineNotification: true, + experimentalFeaturesEnabled: false, + streamNotification: false, + browser: 'default', + // renamed/retyped from boolean doNotMarkMessagesAsRead + markMessagesReadOnScroll: 'always', + }, + }; + // What `base` becomes after all migrations. const endBase = { - ...base37, - migrations: { version: 51 }, + ...base52, + migrations: { version: 52 }, }; for (const [desc, before, after] of [ @@ -206,12 +223,12 @@ describe('migrations', () => { [ 'check 37 with setting already false', { ...base15, settings: { ...base15.settings, doNotMarkMessagesAsRead: false } }, - { ...endBase, settings: { ...endBase.settings, doNotMarkMessagesAsRead: false } }, + { ...endBase, settings: { ...endBase.settings, markMessagesReadOnScroll: 'always' } }, ], [ 'check 37 with setting already true', { ...base15, settings: { ...base15.settings, doNotMarkMessagesAsRead: true } }, - { ...endBase, settings: { ...endBase.settings, doNotMarkMessagesAsRead: true } }, + { ...endBase, settings: { ...endBase.settings, markMessagesReadOnScroll: 'never' } }, ], [ 'check 38', @@ -241,6 +258,24 @@ describe('migrations', () => { drafts: { 'topic:8:stuff': 'text', 'stream:8': 'more text', 'pm:12': 'pm text' }, }, ], + [ + 'check 52 with old setting false', + { + ...base37, + migrations: { version: 51 }, + settings: { ...base37.settings, doNotMarkMessagesAsRead: false }, + }, + { ...endBase, settings: { ...endBase.settings, markMessagesReadOnScroll: 'always' } }, + ], + [ + 'check 52 with old setting true', + { + ...base37, + migrations: { version: 51 }, + settings: { ...base37.settings, doNotMarkMessagesAsRead: true }, + }, + { ...endBase, settings: { ...endBase.settings, markMessagesReadOnScroll: 'never' } }, + ], ]) { /* eslint-disable no-loop-func */ test(desc, async () => { diff --git a/src/storage/migrations.js b/src/storage/migrations.js index 8e5a9e98c95..044f51211c4 100644 --- a/src/storage/migrations.js +++ b/src/storage/migrations.js @@ -371,12 +371,16 @@ const migrationsInner: {| [string]: (LessPartialState) => LessPartialState |} = // Handle explicitly the migrations above (before 30, 34, 36, and this // one) that were done implicitly by the behavior of `autoRehydrate` on a // REHYDRATE action. + /* $FlowIgnore[prop-missing]: `doNotMarkMessagesAsRead` renamed to + `markMessagesReadOnScroll` in 52 */ '37': state => ({ // This handles the migrations affecting RealmState, before 34, 36, and here. ...dropCache(state), // Handle the migration before 30. settings: { ...state.settings, + /* $FlowIgnore[prop-missing]: `doNotMarkMessagesAsRead` renamed to + `markMessagesReadOnScroll` in 52 */ doNotMarkMessagesAsRead: state.settings.doNotMarkMessagesAsRead ?? false, }, }), @@ -443,6 +447,19 @@ const migrationsInner: {| [string]: (LessPartialState) => LessPartialState |} = // Add serverEmojiData to state.realm. '51': dropCache, + // Change boolean doNotMarkMessagesAsRead to enum markMessagesReadOnScroll + '52': state => { + // $FlowIgnore[prop-missing] + const { doNotMarkMessagesAsRead, ...restSettings } = state.settings; + return { + ...state, + settings: { + ...restSettings, + markMessagesReadOnScroll: (doNotMarkMessagesAsRead: boolean) ? 'never' : 'always', + }, + }; + }, + // TIP: When adding a migration, consider just using `dropCache`. // (See its jsdoc for guidance on when that's the right answer.) }; diff --git a/src/webview/MessageList.js b/src/webview/MessageList.js index 466807a4959..8e6e7353560 100644 --- a/src/webview/MessageList.js +++ b/src/webview/MessageList.js @@ -35,8 +35,9 @@ import { handleWebViewOutboundEvent } from './handleOutboundEvents'; import { base64Utf8Encode } from '../utils/encoding'; import * as logging from '../utils/logging'; import { tryParseUrl } from '../utils/url'; -import { caseNarrow } from '../utils/narrow'; +import { caseNarrow, isConversationNarrow } from '../utils/narrow'; import { type BackgroundData, getBackgroundData } from './backgroundData'; +import { ensureUnreachable } from '../generics'; type OuterProps = $ReadOnly<{| narrow: Narrow, @@ -300,7 +301,20 @@ const MessageList: ComponentType = connect( ), typingUsers: getCurrentTypingUsers(state, props.narrow), doNotMarkMessagesAsRead: - !marksMessagesAsRead(props.narrow) || globalSettings.doNotMarkMessagesAsRead, + !marksMessagesAsRead(props.narrow) + || (() => { + switch (globalSettings.markMessagesReadOnScroll) { + case 'always': + return false; + case 'never': + return true; + case 'conversation-views-only': + return !isConversationNarrow(props.narrow); + default: + ensureUnreachable(globalSettings.markMessagesReadOnScroll); + return false; + } + })(), }; }, )(connectActionSheet(withGetText(MessageListInner))); diff --git a/src/webview/__tests__/generateInboundEvents-test.js b/src/webview/__tests__/generateInboundEvents-test.js index 191a5d33704..e870d420522 100644 --- a/src/webview/__tests__/generateInboundEvents-test.js +++ b/src/webview/__tests__/generateInboundEvents-test.js @@ -15,7 +15,7 @@ describe('generateInboundEvents', () => { messages: [], messageListElementsForShownMessages: [], typingUsers: [], - doNotMarkMessagesAsRead: eg.baseReduxState.settings.doNotMarkMessagesAsRead, + doNotMarkMessagesAsRead: false, }); type FudgedProps = {| diff --git a/static/translations/messages_en.json b/static/translations/messages_en.json index 678f451b7e9..7e508fe32f5 100644 --- a/static/translations/messages_en.json +++ b/static/translations/messages_en.json @@ -283,7 +283,12 @@ "Active": "Active", "Idle": "Idle", "Offline": "Offline", - "Do not mark messages read on scroll": "Do not mark messages read on scroll", + "Mark messages as read on scroll": "Mark messages as read on scroll", + "When scrolling through messages, should they automatically be marked as read?": "When scrolling through messages, should they automatically be marked as read?", + "Always": "Always", + "Never": "Never", + "Only in conversation views": "Only in conversation views", + "Messages will be marked as read in single-topic or private-message views, but not in interleaved views, such as whole-stream views.": "Messages will be marked as read in single-topic or private-message views, but not in interleaved views, such as whole-stream views.", "Topics": "Topics", "Add subscribers": "Add subscribers", "Subscribe": "Subscribe", From ec67df4bee0c8f0264ce6a83aa149cc570d7f30f Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Thu, 1 Sep 2022 18:09:58 -0700 Subject: [PATCH 4/6] styles [nfc]: Simplify padding in miscStyles.listItem I checked, and no consumers were separately setting any padding on a component where this was being used. (If one did, then we'd want to investigate to see if this changed what padding was applied, e.g., because of how more-specific attributes prevail over less-specific ones.) --- src/styles/miscStyles.js | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/styles/miscStyles.js b/src/styles/miscStyles.js index d65e7cf9532..02bca090f75 100644 --- a/src/styles/miscStyles.js +++ b/src/styles/miscStyles.js @@ -12,10 +12,8 @@ export const statics = { listItem: { flexDirection: 'row', alignItems: 'center', - paddingTop: 8, - paddingBottom: 8, - paddingLeft: 16, - paddingRight: 16, + paddingVertical: 8, + paddingHorizontal: 16, }, flexed: { flex: 1, From 7ee4bd4e7da687ff53dcf5cd222b676e4777eb60 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Thu, 1 Sep 2022 18:21:30 -0700 Subject: [PATCH 5/6] settings screen [nfc]: Simplify comparing *Row layouts on this screen Now, there's less clicking required to check, by reading code, that the three components NestedNavRow, SwitchRow, and InputRowRadioButtons will look reasonably uniform. That's nice when they're all seen together, as they are when they make a list of settings on the settings screen (with the last of these being a newcomer there; see recent commits). This also continues our progress toward removing the constants in miscStyles. --- src/common/NestedNavRow.js | 7 ++++++- src/common/SwitchRow.js | 7 ++++++- 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/src/common/NestedNavRow.js b/src/common/NestedNavRow.js index 1a6adb07426..8aeb0a57c64 100644 --- a/src/common/NestedNavRow.js +++ b/src/common/NestedNavRow.js @@ -37,6 +37,11 @@ export default function NestedNavRow(props: Props): Node { () => createStyleSheet({ container: { + flexDirection: 'row', + alignItems: 'center', + paddingVertical: 8, + paddingHorizontal: 16, + // Minimum touch target height (and width): // https://material.io/design/usability/accessibility.html#layout-and-typography minHeight: 48, @@ -60,7 +65,7 @@ export default function NestedNavRow(props: Props): Node { return ( - + {!!Icon && } diff --git a/src/common/SwitchRow.js b/src/common/SwitchRow.js index a00b0681333..a7fb6121cbf 100644 --- a/src/common/SwitchRow.js +++ b/src/common/SwitchRow.js @@ -17,6 +17,11 @@ type Props = $ReadOnly<{| const componentStyles = createStyleSheet({ container: { + flexDirection: 'row', + alignItems: 'center', + paddingVertical: 8, + paddingHorizontal: 16, + // For uniformity with other rows this might share a screen with, e.g., // NestedNavRow and InputRowRadioButtons on the settings screen. See // height-related attributes on those rows. @@ -37,7 +42,7 @@ export default function SwitchRow(props: Props): Node { const themeContext = useContext(ThemeContext); return ( - + {!!Icon && } From 63f46ebe9f9b90cfe62fe42b5907d60925ba5660 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Wed, 31 Aug 2022 17:56:47 -0700 Subject: [PATCH 6/6] InputRowRadioButtons types: Relax TItemKey to accept `number` And also for SelectableOptionsScreen, which this uses. These now match SelectableOptionRow, which has already been accepting `number`. --- src/common/InputRowRadioButtons.js | 4 +++- src/common/SelectableOptionsScreen.js | 4 +++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/src/common/InputRowRadioButtons.js b/src/common/InputRowRadioButtons.js index 2adc7b8c80b..026c11cb0d0 100644 --- a/src/common/InputRowRadioButtons.js +++ b/src/common/InputRowRadioButtons.js @@ -68,7 +68,9 @@ type Props = $ReadOnly<{| // represented by IconRight. NestedNavRow would probably be the wrong // abstraction, though, because it isn't an imput component; it doesn't have // a value to display. -export default function InputRowRadioButtons(props: Props): Node { +export default function InputRowRadioButtons( + props: Props, +): Node { const { navigation, label, description, valueKey, items, onValueChange } = props; const screenKey: string = useRef(`selectable-options-${randString()}`).current; diff --git a/src/common/SelectableOptionsScreen.js b/src/common/SelectableOptionsScreen.js index 04193aa8b33..4b1e11b4c4d 100644 --- a/src/common/SelectableOptionsScreen.js +++ b/src/common/SelectableOptionsScreen.js @@ -78,7 +78,9 @@ LogBox.ignoreLogs([/selectable-options > params\.onRequestSelectionChange \(Func // If we need separate components dedicated to checkboxes and radio buttons, // we can split this. Currently it's up to the caller to enforce the // radio-button invariant (exactly one item selected) if they want to. -export default function SelectableOptionsScreen(props: Props): Node { +export default function SelectableOptionsScreen( + props: Props, +): Node { const { route } = props; const { title, description, items, onRequestSelectionChange } = route.params;