home screen: Add ability to mark messages in bulk, mute topics. #4194

agrawal-d · 2020-07-17T10:47:16Z

agrawal-d · 2020-07-17T11:03:06Z

src/unread/UnreadCards.js

+      // eslint-disable-next-line no-unused-vars
+      handleTopicSelect,


This comment is present here because although handleTopicSelect is unused, I don't want it going into restProps.

Makes sense. Let's put that in the code comment, though. 😉

agrawal-d · 2020-07-21T16:53:51Z

todo (Done)

~~error handling~~
~~make ui more readable~~
~~handle hardware back button~~

Adds the 'check-circle' MaterialIcon to our icons list, to be used later as a checkbox status indicator.

While icon buttons are great for conveying purpose without text, it can sometimes be confusing what they actually do. Most apps allow long-pressing them to show a toast with their textual equivalent. This commit adds the ability to pass a 'title' prop to 'NavButton'. On long pressing it, a toast is shown with this title string.

Implements a new UI to select multiple topics in the home tab screen with the ability to mute them or mark them as read. Fixes: zulip#3175

agrawal-d · 2020-07-22T07:56:18Z

I've removed the [wip] marker.

chrisbobbe

Cool, this will be really handy!

A few comments below.

Another thought I just had is, do we also want streams to be selectable? If we do, we'll have to think about how we want to treat topics for a stream that has been selected. My first thought was that selecting a stream should auto-select all of its visible topics. But that's no good: for the muting case, muting a stream isn't the same thing as muting all of its topics, and it's definitely not the same thing as muting all of its topics that currently have unread messages. Best, I guess, to not touch the selected/deselected state of its topics, and maybe disable selecting/deselecting the topics (maybe).

chrisbobbe · 2020-08-05T19:21:21Z

src/main/HomeTab.js

+    const color = BRAND_COLOR;
+    const countText = _.intl.formatMessage(
+      {
+        id: '{count} topics selected',


This needs to have an entry in static/translations/messages_en.json.

chrisbobbe · 2020-08-05T19:42:11Z

src/main/HomeTab.js

+    const apiPromises = [];
+
+    if (type === 'mute') {
+      showToast(_('Muting selected topics'));


I think it would be good to announce the success of the action in a toast, once it's succeeded, as something like "Muted selected topics".

It's good to tell the user that their request has been acknowledged, as this "Muting..." / "Marking..." toast does. Unfortunately, our toast library doesn't handle the case where we have multiple toasts in quick succession. One way it might handle that is to move any still-present toasts upwards, to make room for a new one. But it doesn't; ah, well.

Still, even if it did, I think there's a better way to tell the user that their request has been acknowledged, and using fewer words. How about using the component's state to track whether the request is in progress, being sure to set it appropriately when the request starts, and when it fails or succeeds. Then, we can show the loading state graphically through the duration of the request, perhaps by disabling the button they pressed. (This would have the nice benefit of making sure they don't accidentally duplicate the request, which could lead to confusing behavior or bugs.)

chrisbobbe · 2020-08-05T21:16:38Z

src/main/HomeTab.js

+        <View style={commonStyles.navWrapper}>
+          <Label text={countText} style={styles.selectionCountText} />
+        </View>
+        <NavButton


It's a little confusing to use a "nav" button here, where the action isn't to navigate somewhere, but to execute an action on selected topics.

Still, it is in the nav bar, and there's not an obvious other place it could go.

chrisbobbe · 2020-08-05T21:30:46Z

src/main/HomeTab.js

+          title={_('Mute')}
+        />
+        <NavButton
+          name="check"


Hmm, I'm not sure a checkmark really conveys the meaning "mark as read". A checkmark is also used to indicate selecting a topic on the same screen, for one thing.

It looks like Gmail uses an open envelope icon, in both the mobile app (iOS) and in the browser. The situation there is slightly different, though; email threads appear whether they've been read or not, and the button changes to a closed envelope when you select only threads that have been read. That's a bit of extra context that helps solidify the meaning of the open envelope (that and the fact that their product is email).

Still, I think an open envelope icon might be recognizable enough, in our app, or at least more so than a checkmark? What do you think?

edit: Huh, though, looks like the web app uses fa-book for "mark as read" in at least one place:

chrisbobbe · 2020-08-05T21:51:15Z

src/streams/TopicItem.js

+      );
+    }
+
+    if (inBulkSelectionMode) {


The empty circle is just a bit bigger than the circular checkmark icon; they should be exactly the same size, I think.

I wonder if there's a pair of icons that are meant to complement each other? Like this and this, although that particular pair uses a rounded square, and it seems unavailable in the latest version of Font Awesome, hmm.

Maybe this and this?

If we can find two icons, from the same icon set, that are explicitly meant to complement each other, then we don't even have to think about inconsistencies between them.

Not all the icons present online are actually present in the RN library. For example, the empty circle icon is missing from most icon sets while it's present in the online list.

chrisbobbe · 2020-08-05T21:56:09Z

src/main/HomeTab.js

+  handleBackPress = (): boolean => {
+    if (this.state.bulkSelection !== null) {
+      this.setState({ bulkSelection: null });
+      return true;


What's the meaning of returning true or false from this function? That would be good to answer in a code comment.

chrisbobbe · 2020-08-05T22:01:21Z

src/main/HomeTab.js

+class HomeTab extends PureComponent<Props, State> {
+  static contextType = TranslationContext;
+  backHandler;
+  context: GetText;


Let's have context directly follow static contextType. From the JSDoc on GetText in src/types.js:

* To use, put these two lines at the top of a React component's body: * * static contextType = TranslationContext; * context: GetText;

Then a blank line after those, to set them apart.

For backHandler, it looks like it's just sitting there; is there a type annotation we can give it?

agrawal-d requested review from gnprice and chrisbobbe July 17, 2020 10:51

agrawal-d force-pushed the bulk branch from 3979271 to a320c04 Compare July 17, 2020 10:58

agrawal-d commented Jul 17, 2020

View reviewed changes

agrawal-d changed the title ~~home screen: Add ability to mark messages in bulk, mute topics.~~ [wip] home screen: Add ability to mark messages in bulk, mute topics. Jul 21, 2020

agrawal-d added 3 commits July 21, 2020 23:53

icons: Add 'check-circle' material icon.

c8273b8

Adds the 'check-circle' MaterialIcon to our icons list, to be used later as a checkbox status indicator.

home tab: Implement bulk mode for muting and marking topics as read.

dd09113

Implements a new UI to select multiple topics in the home tab screen with the ability to mute them or mark them as read. Fixes: zulip#3175

agrawal-d force-pushed the bulk branch from a320c04 to dd09113 Compare July 21, 2020 18:24

agrawal-d changed the title ~~[wip] home screen: Add ability to mark messages in bulk, mute topics.~~ home screen: Add ability to mark messages in bulk, mute topics. Jul 22, 2020

chrisbobbe requested changes Aug 5, 2020

View reviewed changes

AkashDhiman mentioned this pull request Apr 10, 2021

Add mark as read option in TopicActionSheet and Modify TopicActionSheet to use stream_id. #4635

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

home screen: Add ability to mark messages in bulk, mute topics. #4194

home screen: Add ability to mark messages in bulk, mute topics. #4194

Uh oh!

agrawal-d commented Jul 17, 2020 •

edited

Loading

Uh oh!

agrawal-d Jul 17, 2020

Uh oh!

chrisbobbe Aug 5, 2020

Uh oh!

agrawal-d commented Jul 21, 2020 •

edited

Loading

Uh oh!

agrawal-d commented Jul 22, 2020

Uh oh!

chrisbobbe left a comment

Uh oh!

chrisbobbe Aug 5, 2020

Uh oh!

chrisbobbe Aug 5, 2020 •

edited

Loading

Uh oh!

chrisbobbe Aug 5, 2020

Uh oh!

chrisbobbe Aug 5, 2020

Uh oh!

chrisbobbe Aug 5, 2020 •

edited

Loading

Uh oh!

agrawal-d Aug 7, 2020

Uh oh!

chrisbobbe Aug 5, 2020

Uh oh!

chrisbobbe Aug 5, 2020

Uh oh!

Uh oh!

		// eslint-disable-next-line no-unused-vars
		handleTopicSelect,

Uh oh!

home screen: Add ability to mark messages in bulk, mute topics. #4194

Are you sure you want to change the base?

home screen: Add ability to mark messages in bulk, mute topics. #4194

Uh oh!

Conversation

agrawal-d commented Jul 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

agrawal-d commented Jul 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

agrawal-d commented Jul 22, 2020

Uh oh!

chrisbobbe left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chrisbobbe Aug 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chrisbobbe Aug 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

agrawal-d commented Jul 17, 2020 •

edited

Loading

agrawal-d commented Jul 21, 2020 •

edited

Loading

chrisbobbe Aug 5, 2020 •

edited

Loading

chrisbobbe Aug 5, 2020 •

edited

Loading