TopicItem: Show actionsheet on long-press. #4608

WesleyAC · 2021-04-06T09:29:44Z

No description provided.

chrisbobbe

Thanks, @WesleyAC, this looks good! See some small comments below; also, here's a screenshot on iOS because I happened to test on iOS:

chrisbobbe · 2021-04-06T17:40:19Z

src/main/HomeScreen.js

-              NavigationService.dispatch(navigateToSearch());
-            }}
-          />
+      <ActionSheetProvider>


I think one part of my revision of #4468 that we'd like to keep is to have just one ActionSheetProvider for the whole app. It's currently

a4ff91f ZulipMobile [nfc]: Use just one ActionSheetProvider, here.

If that sounds good to you, you could rewrite or cherry-pick it—I don't expect #4468 as a whole to be mergeable for a little while; there are some changes I still need to make. But IIUC that part is fine. 🙂

(I find that a -b flag, to ignore whitespace changes, helps in viewing the kind of changes made in that commit: as in git lsp -b a4ff91f4c (where git lsp is Greg's alias, copied to my config, for git log --stat -p).)

I tried this (both independently while I was making this PR, and just now cherrypicking the commit you linked), but it appears to break actionsheets on Android (at least, the ones in the webview and lightbox). I didn't really want to debug that, which is why I ended up just adding another ActionSheetProvider.

Do you have any idea what's going on there?

(Answered in chat, here.)

chrisbobbe · 2021-04-06T17:56:25Z

src/streams/TopicItem.js

 type Props = $ReadOnly<{|
  stream?: string,
  name: string,
  isMuted?: boolean,
  isSelected?: boolean,
  unreadCount?: number,
  onPress: (topic: string, stream: string) => void,
+  dispatch: Dispatch,


nit: We tend to keep an empty line above the dispatch and ...SelectorProps pair, to separate props that the inner component receives automatically from the wrapper component created by connect.

But also, I see a further simplification: when the component is a function component (i.e., one where we can use React Hooks), we usually reach for the useSelector hook instead of connect. This means one less component in the component tree, which helps with reading the component stack React Native provides when there's an error in a component's render method. Also, our type-wrapper for useSelector is much simpler than that for connect, and I think this has made it easier for me to get valid, type-checked code with useSelector. I also think it's just generally easier to read. 🙂

I'm thinking something like this:

import { useSelector } from '../react-redux'; // ... export default function TopicItem(props: Props) { // ... const backgroundData = useSelector(state => ({ auth: getAuth(state), mute: getMute(state), subscriptions: getSubscriptions(state), ownUser: getOwnUser(state), flags: getFlags(state), })); // ... }

Oh, I didn't know about useSelector, that's very nice :) Done!

WesleyAC · 2021-04-07T19:57:16Z

@chrisbobbe FYI, this is ready for another review :)

gnprice

Thanks @WesleyAC ! A couple of other comments below.

gnprice · 2021-04-07T23:49:16Z

src/ZulipMobile.js

-        </SafeAreaProvider>
-      </StoreProvider>
-    </CompatibilityChecker>
+    <ActionSheetProvider>


Adding this should come as its own commit, before the commit that adds the particular new use of the provider. That makes it easier to see what each of the changes is doing; it also makes it easier to pin down where a given change in behavior happened, if there's later some bug that seems like it could be related to these changes.

gnprice · 2021-04-07T23:50:23Z

src/ZulipMobile.js

-        </SafeAreaProvider>
-      </StoreProvider>
-    </CompatibilityChecker>
+    <ActionSheetProvider>


It feels a bit odd to be inserting one of these at the top when we also have some at lower points in the tree. But if having both causes things to break (as you mentioned elsewhere), this can be a reasonable workaround.

I do wonder, though, if anything can go wrong as a consequence of having more than one of them nested inside each other. Have you tested out the existing action sheets after this change?

gnprice · 2021-04-07T23:55:11Z

src/streams/TopicItem.js

+    <Touchable
+      onPress={() => onPress(stream, name)}
+      onLongPress={() => {
+        showHeaderActionSheet({


One bit which should be a commit of its own, but would be good to include in this PR: this name is now increasingly inapt and it'd be good to rename this to something more accurate. 🙂 There's no particular "header" involved here (like the "recipient header" in the message list, which was the original use of this), and conversely when we do have a recipient header but it's for a PM, we don't use this.

Perhaps... showStreamOrTopicActionSheet? Maybe also a bit of jsdoc, saying something like: show an action sheet with actions for a whole stream or topic.

Currently this is actually only a TopicActionSheet, there's no way (that I know of) to call it with just a stream and no topic.

I agree that we should rename it, but I'd prefer to punt on that until we figure out if it will in fact turn into a StreamOrTopicActionSheet, or if we're going to have it turn into a TopicActionSheet, and we'll grow a StreamActionSheet as well.

I suspect it'll be the latter, but I don't think I know for sure right now, so I'd rather wait to rename until I'm more sure what this will end up looking like, to save us the effort of possibly renaming it twice.

WesleyAC · 2021-04-08T07:04:14Z

@chrisbobbe This should be ready for another review. I also fixed the bug that @AkashDhiman reported on CZO.

I don't see a reason why we'd need multiple `ActionSheetProviders`, wrapping more or less tightly the code that opens each action sheet. The examples in the docs have it wrap around the whole app [1]. I tested the message action sheet and the lightbox action sheet on iOS and Android and didn't notice any differences from the previous behavior. [1] https://github.com/expo/react-native-action-sheet

This will be required for actionsheets to work properly.

It will be needed for the actionsheet, and will cause bugs for it to not exist.

chrisbobbe

Thanks, @WesleyAC! Merging. Thanks for testing and incorporating the ActionSheetProvider change; I'm glad not to have to do #4468 without that, as it helps me avoid rebase conflicts: #4468 (comment). 🙂

chrisbobbe · 2021-04-08T19:02:20Z

src/streams/TopicItem.js

@@ -31,14 +31,7 @@ type Props = $ReadOnly<{|
 |}>;

 export default function TopicItem(props: Props) {
-  const {
-    name,
-    stream = '',


🎉

The empty string at stream can't have been up to anything good. I'm curious what TopicItem's caller in UnreadCards is trying to accomplish by passing the empty string, but that's probably for other work outside this PR.

WesleyAC requested review from gnprice and chrisbobbe April 6, 2021 09:29

chrisbobbe reviewed Apr 6, 2021

View reviewed changes

WesleyAC force-pushed the topicitem-longpress-actionsheet branch 2 times, most recently from 0509909 to b3b4ba8 Compare April 7, 2021 13:59

gnprice reviewed Apr 8, 2021

View reviewed changes

WesleyAC force-pushed the topicitem-longpress-actionsheet branch from b3b4ba8 to 3caef6b Compare April 8, 2021 06:42

WesleyAC force-pushed the topicitem-longpress-actionsheet branch from d9f68e1 to d193f78 Compare April 8, 2021 07:17

chrisbobbe and others added 4 commits April 8, 2021 14:53

TopicList [nfc]: Pass stream name into TopicItem

a3fab39

This will be required for actionsheets to work properly.

TopicItem [nfc]: Make stream name required.

c93db0f

It will be needed for the actionsheet, and will cause bugs for it to not exist.

TopicItem: Show actionsheet on long-press.

9634872

chrisbobbe approved these changes Apr 8, 2021

View reviewed changes

chrisbobbe force-pushed the topicitem-longpress-actionsheet branch from d193f78 to 9634872 Compare April 8, 2021 19:29

chrisbobbe merged commit 9634872 into zulip:master Apr 8, 2021

WesleyAC mentioned this pull request Apr 17, 2021

There's no obvious way to unmute a topic #3473

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TopicItem: Show actionsheet on long-press. #4608

TopicItem: Show actionsheet on long-press. #4608

WesleyAC commented Apr 6, 2021

chrisbobbe left a comment •

edited

Loading

chrisbobbe Apr 6, 2021

chrisbobbe Apr 6, 2021 •

edited

Loading

WesleyAC Apr 7, 2021

chrisbobbe Apr 8, 2021

chrisbobbe Apr 6, 2021

WesleyAC Apr 7, 2021

WesleyAC commented Apr 7, 2021

gnprice left a comment

gnprice Apr 7, 2021

gnprice Apr 7, 2021

gnprice Apr 7, 2021

WesleyAC Apr 8, 2021 •

edited

Loading

WesleyAC commented Apr 8, 2021

chrisbobbe left a comment •

edited

Loading

chrisbobbe Apr 8, 2021

TopicItem: Show actionsheet on long-press. #4608

TopicItem: Show actionsheet on long-press. #4608

Conversation

WesleyAC commented Apr 6, 2021

chrisbobbe left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbobbe Apr 6, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WesleyAC commented Apr 7, 2021

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WesleyAC Apr 8, 2021 • edited Loading

Choose a reason for hiding this comment

WesleyAC commented Apr 8, 2021

chrisbobbe left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbobbe left a comment •

edited

Loading

chrisbobbe Apr 6, 2021 •

edited

Loading

WesleyAC Apr 8, 2021 •

edited

Loading

chrisbobbe left a comment •

edited

Loading