settings: Add option to mark read on scroll only in conversation views #5469

chrisbobbe · 2022-08-18T21:18:02Z

Thanks to the handy new InputRowRadioButtons component we added in edf5322, this is now a quick user-visible feature we can check off the list.

Screenshots below.

Fixes: #5241
Replaces: #5294

chrisbobbe · 2022-08-18T21:21:56Z

Before:

After:

chrisbobbe · 2022-08-18T21:22:56Z

I see that the right-facing arrows aren't lined up. Let's check back in after #5446 is merged; I expect that to go in the right direction towards fixing that (see screenshots there), and may even be the right, complete fix. Marking as blocked on #5446, but we can still review wording, etc., in the meantime.

gnprice · 2022-08-30T23:17:00Z

src/common/InputRowRadioButtons.js

-          <IconRight size={kRightArrowIconSize} color={BRAND_COLOR} />
+          <IconRight size={kRightArrowIconSize} color={themeData.color} />


What's the effect of this first commit -- is there an existing screen where it changes something?

Yep, the edit-stream screen. Here's a before-and-after:

Great, thanks. Seems good.

gnprice · 2022-08-31T19:14:36Z

#5446 has been merged, so I think the next step is to rebase atop that and see how that alignment looks.

chrisbobbe · 2022-09-01T01:02:37Z

#5446 has been merged, so I think the next step is to rebase atop that and see how that alignment looks.

Yep, I agree. I haven't yet checked the alignment in this latest revision—for now, this just resolves rebase conflicts, and adds two commits on top that convert to using Flow enums now that #5444 has landed. We can see how we like that for this use case. 🙂 For now I have to run, though.

chrisbobbe · 2022-09-02T01:57:04Z

I haven't yet checked the alignment in this latest revision

OK, and done that for my new revision; I think we're good with the alignment. 🙂 Here's the before-and-after:

gnprice · 2022-09-02T20:58:36Z

Thanks for the revision! Will take a look at the code momentarily.

@alya, do you have any thoughts on the text in the new screen? See the lower-right screenshot in the first comment above.

gnprice · 2022-09-02T21:07:54Z

src/reduxTypes.js

+export enum MarkMessagesReadOnScroll {
+  Always = 0,
+  Never = 1,
+  ConversationViewsOnly = 3,


Hmm -- why skip 2? Do the values end up mattering?

gnprice · 2022-09-02T21:08:09Z

src/storage/__tests__/migrations-test.js

+    migrations: { version: 52 },
+    ...base37,


This will clobber migrations, no?

Oops, I should switch the order of these two lines.

gnprice · 2022-09-02T21:50:58Z

src/settings/SettingsScreen.js

-          dispatch(setGlobalSettings({ doNotMarkMessagesAsRead: value }));
+          dispatch(
+            setGlobalSettings({
+              markMessagesReadOnScroll: MarkMessagesReadOnScroll.cast(value),


Hmm, awkward to have to do this. (Also the static casts (…: number) above.)

It looks like the root of this is that we say key={item.key} in SelectableOptionsScreen, passing these keys as React key pseudoprops. Ideally the type accepted there could be like "string, or number, or any enum of string or number", but there's no way to express the "any enum" part:
https://flow.org/en/docs/enums/using-enums/#toc-generic-enums
so it isn't.

I think in light of this bit I'd lean toward skipping the enum here, and sticking with the disjoint union of strings. There isn't a lot that we gain in this instance from the enum. But this price for using an enum isn't too bad, either.

Using the handy new InputRowRadioButtons component we added in edf5322. Fixes: zulip#5241

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.)

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.

And also for SelectableOptionsScreen, which this uses. These now match SelectableOptionRow, which has already been accepting `number`.

chrisbobbe · 2022-09-02T22:42:53Z

Thanks for the review! Revision pushed.

gnprice · 2022-09-02T22:58:42Z

Thanks! Looks good -- merging.

@alya, if you have any thoughts on the settings text, happy to make a change.

alya · 2022-09-11T07:01:46Z

Sorry I missed the question earlier! I started a thread about tweaking the hint on CZO.

Following Alya's proposal at https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/mark.20as.20read.20setting.20.23M5469/near/1433721 Related: zulip#5469

chrisbobbe requested review from gnprice and alya August 18, 2022 21:18

chrisbobbe added the blocked on other work To come back to after another related PR, or some other task. label Aug 18, 2022

gnprice reviewed Aug 30, 2022

View reviewed changes

gnprice removed the blocked on other work To come back to after another related PR, or some other task. label Aug 31, 2022

chrisbobbe force-pushed the pr-mark-read-conversation-views branch 2 times, most recently from 56c1e95 to d224289 Compare September 1, 2022 01:02

chrisbobbe force-pushed the pr-mark-read-conversation-views branch from d224289 to b6b5074 Compare September 2, 2022 01:49

gnprice mentioned this pull request Sep 2, 2022

settings_page: Add marks as read on scroll options #5294

Closed

chrisbobbe marked this pull request as draft September 2, 2022 20:57

gnprice reviewed Sep 2, 2022

View reviewed changes

chrisbobbe added 6 commits September 2, 2022 15:34

InputRowRadioButtons: Use theme color for IconRight, like NestedNavRow

9318bf3

InputRowRadioButtons: Set minHeight: 48, since this is a touch target

f39fcad

settings: Add option to mark read on scroll only in conversation views

6e88153

Using the handy new InputRowRadioButtons component we added in edf5322. Fixes: zulip#5241

InputRowRadioButtons types: Relax TItemKey to accept number

63f46eb

And also for SelectableOptionsScreen, which this uses. These now match SelectableOptionRow, which has already been accepting `number`.

chrisbobbe force-pushed the pr-mark-read-conversation-views branch from b6b5074 to 63f46eb Compare September 2, 2022 22:42

chrisbobbe marked this pull request as ready for review September 2, 2022 22:42

gnprice merged commit 63f46eb into zulip:main Sep 2, 2022

chrisbobbe deleted the pr-mark-read-conversation-views branch September 2, 2022 23:00

This was referenced Sep 13, 2022

SettingsScreen: Make conversation-views-only hint text more concise #5485

Merged

Automatically follow user's OS-level dark/light preference, take 1 #4009

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

settings: Add option to mark read on scroll only in conversation views #5469

settings: Add option to mark read on scroll only in conversation views #5469

chrisbobbe commented Aug 18, 2022 •

edited

Loading

chrisbobbe commented Aug 18, 2022

chrisbobbe commented Aug 18, 2022 •

edited

Loading

gnprice Aug 30, 2022

chrisbobbe Aug 31, 2022

gnprice Aug 31, 2022

gnprice commented Aug 31, 2022

chrisbobbe commented Sep 1, 2022 •

edited

Loading

chrisbobbe commented Sep 2, 2022

gnprice commented Sep 2, 2022

gnprice Sep 2, 2022

gnprice Sep 2, 2022

chrisbobbe Sep 2, 2022

gnprice Sep 2, 2022

chrisbobbe commented Sep 2, 2022

gnprice commented Sep 2, 2022

alya commented Sep 11, 2022 •

edited

Loading

		<IconRight size={kRightArrowIconSize} color={BRAND_COLOR} />
		<IconRight size={kRightArrowIconSize} color={themeData.color} />

settings: Add option to mark read on scroll only in conversation views #5469

settings: Add option to mark read on scroll only in conversation views #5469

Conversation

chrisbobbe commented Aug 18, 2022 • edited Loading

chrisbobbe commented Aug 18, 2022

chrisbobbe commented Aug 18, 2022 • edited Loading

gnprice Aug 30, 2022

Choose a reason for hiding this comment

chrisbobbe Aug 31, 2022

Choose a reason for hiding this comment

gnprice Aug 31, 2022

Choose a reason for hiding this comment

gnprice commented Aug 31, 2022

chrisbobbe commented Sep 1, 2022 • edited Loading

chrisbobbe commented Sep 2, 2022

gnprice commented Sep 2, 2022

gnprice Sep 2, 2022

Choose a reason for hiding this comment

gnprice Sep 2, 2022

Choose a reason for hiding this comment

chrisbobbe Sep 2, 2022

Choose a reason for hiding this comment

gnprice Sep 2, 2022

Choose a reason for hiding this comment

chrisbobbe commented Sep 2, 2022

gnprice commented Sep 2, 2022

alya commented Sep 11, 2022 • edited Loading

chrisbobbe commented Aug 18, 2022 •

edited

Loading

chrisbobbe commented Aug 18, 2022 •

edited

Loading

chrisbobbe commented Sep 1, 2022 •

edited

Loading

alya commented Sep 11, 2022 •

edited

Loading