diff --git a/src/common/InputRowRadioButtons.js b/src/common/InputRowRadioButtons.js index 60c1d2b11d5..026c11cb0d0 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, @@ -57,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 @@ -67,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; @@ -128,6 +131,7 @@ export default function InputRowRadioButtons(props: Props createStyleSheet({ @@ -136,6 +140,10 @@ export default function InputRowRadioButtons(props: Props(props: Props - + 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/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; diff --git a/src/common/SwitchRow.js b/src/common/SwitchRow.js index e9f5c90381b..a7fb6121cbf 100644 --- a/src/common/SwitchRow.js +++ b/src/common/SwitchRow.js @@ -17,9 +17,14 @@ 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 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: { @@ -37,7 +42,7 @@ export default function SwitchRow(props: Props): Node { const themeContext = useContext(ThemeContext); return ( - + {!!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/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, 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",