Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TopicItem: Show actionsheet on long-press. #4608

Merged
merged 4 commits into from
Apr 8, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion src/ZulipMobile.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import React from 'react';
import { Platform, UIManager } from 'react-native';
import 'react-native-url-polyfill/auto';
import { SafeAreaProvider } from 'react-native-safe-area-context';
import { ActionSheetProvider } from '@expo/react-native-action-sheet';

import RootErrorBoundary from './RootErrorBoundary';
import { BRAND_COLOR } from './styles';
Expand Down Expand Up @@ -52,7 +53,9 @@ export default (): React$Node => (
<TranslationProvider>
<ThemeProvider>
<BackNavigationHandler>
<ZulipNavigationContainer />
<ActionSheetProvider>
<ZulipNavigationContainer />
</ActionSheetProvider>
</BackNavigationHandler>
</ThemeProvider>
</TranslationProvider>
Expand Down
67 changes: 32 additions & 35 deletions src/chat/ChatScreen.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@
import React from 'react';
import { View } from 'react-native';
import { useIsFocused } from '@react-navigation/native';
import { ActionSheetProvider } from '@expo/react-native-action-sheet';

import { useSelector, useDispatch } from '../react-redux';
import type { RouteProp } from '../react-navigation';
Expand Down Expand Up @@ -117,39 +116,37 @@ export default function ChatScreen(props: Props) {
const streamColor = useSelector(state => getStreamColorForNarrow(state, narrow));

return (
<ActionSheetProvider>
<View style={[componentStyles.screen, { backgroundColor }]}>
<KeyboardAvoider style={styles.flexed} behavior="padding">
<ZulipStatusBar backgroundColor={streamColor} />
<ChatNavBar narrow={narrow} editMessage={editMessage} />
<OfflineNotice />
<UnreadNotice narrow={narrow} />
{(() => {
if (!isNarrowValid) {
return <InvalidNarrow narrow={narrow} />;
} else if (fetchError !== null) {
return <FetchError narrow={narrow} error={fetchError} />;
} else if (sayNoMessages) {
return <NoMessages narrow={narrow} />;
} else {
return (
<MessageList
narrow={narrow}
showMessagePlaceholders={showMessagePlaceholders}
startEditMessage={setEditMessage}
/>
);
}
})()}
{showComposeBox && (
<ComposeBox
narrow={narrow}
editMessage={editMessage}
completeEditMessage={() => setEditMessage(null)}
/>
)}
</KeyboardAvoider>
</View>
</ActionSheetProvider>
<View style={[componentStyles.screen, { backgroundColor }]}>
<KeyboardAvoider style={styles.flexed} behavior="padding">
<ZulipStatusBar backgroundColor={streamColor} />
<ChatNavBar narrow={narrow} editMessage={editMessage} />
<OfflineNotice />
<UnreadNotice narrow={narrow} />
{(() => {
if (!isNarrowValid) {
return <InvalidNarrow narrow={narrow} />;
} else if (fetchError !== null) {
return <FetchError narrow={narrow} error={fetchError} />;
} else if (sayNoMessages) {
return <NoMessages narrow={narrow} />;
} else {
return (
<MessageList
narrow={narrow}
showMessagePlaceholders={showMessagePlaceholders}
startEditMessage={setEditMessage}
/>
);
}
})()}
{showComposeBox && (
<ComposeBox
narrow={narrow}
editMessage={editMessage}
completeEditMessage={() => setEditMessage(null)}
/>
)}
</KeyboardAvoider>
</View>
);
}
5 changes: 1 addition & 4 deletions src/lightbox/LightboxScreen.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
/* @flow strict-local */
import React from 'react';
import { View } from 'react-native';
import { ActionSheetProvider } from '@expo/react-native-action-sheet';

import type { Message } from '../types';
import type { RouteProp } from '../react-navigation';
Expand Down Expand Up @@ -30,9 +29,7 @@ export default function LightboxScreen(props: Props) {
return (
<View style={styles.screen}>
<ZulipStatusBar hidden backgroundColor="black" />
<ActionSheetProvider>
<Lightbox src={src} message={message} />
</ActionSheetProvider>
<Lightbox src={src} message={message} />
</View>
);
}
21 changes: 9 additions & 12 deletions src/search/SearchMessagesCard.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@

import React, { PureComponent } from 'react';
import { View } from 'react-native';
import { ActionSheetProvider } from '@expo/react-native-action-sheet';

import type { Message } from '../types';
import { createStyleSheet } from '../styles';
Expand Down Expand Up @@ -49,17 +48,15 @@ export default class SearchMessagesCard extends PureComponent<Props> {

return (
<View style={styles.results}>
<ActionSheetProvider>
<MessageList
initialScrollMessageId={messages[0].id}
messages={messages}
narrow={HOME_NARROW}
htmlPieceDescriptorsForShownMessages={htmlPieceDescriptors}
fetching={SearchMessagesCard.NOT_FETCHING}
showMessagePlaceholders={false}
typingUsers={NULL_ARRAY}
/>
</ActionSheetProvider>
<MessageList
initialScrollMessageId={messages[0].id}
messages={messages}
narrow={HOME_NARROW}
htmlPieceDescriptorsForShownMessages={htmlPieceDescriptors}
fetching={SearchMessagesCard.NOT_FETCHING}
showMessagePlaceholders={false}
typingUsers={NULL_ARRAY}
/>
</View>
);
}
Expand Down
45 changes: 33 additions & 12 deletions src/streams/TopicItem.js
Original file line number Diff line number Diff line change
@@ -1,10 +1,15 @@
/* @flow strict-local */
import React from 'react';
import React, { useContext } from 'react';
import { View } from 'react-native';
import { useActionSheet } from '@expo/react-native-action-sheet';

import styles, { BRAND_COLOR, createStyleSheet } from '../styles';
import { RawLabel, Touchable, UnreadCount } from '../common';
import { showToast } from '../utils/info';
import { showHeaderActionSheet } from '../message/messageActionSheet';
import type { ShowActionSheetWithOptions } from '../message/messageActionSheet';
import { TranslationContext } from '../boot/TranslationProvider';
import { useDispatch, useSelector } from '../react-redux';
import { getAuth, getMute, getFlags, getSubscriptions, getOwnUser } from '../selectors';

const componentStyles = createStyleSheet({
selectedRow: {
Expand All @@ -22,7 +27,7 @@ const componentStyles = createStyleSheet({
});

type Props = $ReadOnly<{|
stream?: string,
stream: string,
name: string,
isMuted?: boolean,
isSelected?: boolean,
Expand All @@ -31,17 +36,33 @@ type Props = $ReadOnly<{|
|}>;

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉

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.

isMuted = false,
isSelected = false,
unreadCount = 0,
onPress,
} = props;
const { name, stream, isMuted = false, isSelected = false, unreadCount = 0, onPress } = props;

const showActionSheetWithOptions: ShowActionSheetWithOptions = useActionSheet()
.showActionSheetWithOptions;
const _ = useContext(TranslationContext);
const dispatch = useDispatch();
const backgroundData = useSelector(state => ({
auth: getAuth(state),
mute: getMute(state),
subscriptions: getSubscriptions(state),
ownUser: getOwnUser(state),
flags: getFlags(state),
}));

return (
<Touchable onPress={() => onPress(stream, name)} onLongPress={() => showToast(name)}>
<Touchable
onPress={() => onPress(stream, name)}
onLongPress={() => {
showHeaderActionSheet({
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@WesleyAC WesleyAC Apr 8, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

showActionSheetWithOptions,
callbacks: { dispatch, _ },
backgroundData,
stream,
topic: name,
});
}}
>
<View
style={[
styles.listItem,
Expand Down
6 changes: 4 additions & 2 deletions src/topics/TopicList.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
import React, { PureComponent } from 'react';
import { FlatList } from 'react-native';

import type { TopicExtended } from '../types';
import type { Stream, TopicExtended } from '../types';
import { createStyleSheet } from '../styles';
import TopicItem from '../streams/TopicItem';
import { LoadingIndicator, SearchEmptyState } from '../common';
Expand All @@ -15,13 +15,14 @@ const styles = createStyleSheet({
});

type Props = $ReadOnly<{|
stream: Stream,
topics: ?(TopicExtended[]),
onPress: (stream: string, topic: string) => void,
|}>;

export default class TopicList extends PureComponent<Props> {
render() {
const { topics, onPress } = this.props;
const { stream, topics, onPress } = this.props;

if (!topics) {
return <LoadingIndicator size={40} />;
Expand All @@ -40,6 +41,7 @@ export default class TopicList extends PureComponent<Props> {
renderItem={({ item }) => (
<TopicItem
name={item.name}
stream={stream.name}
isMuted={item.isMuted}
unreadCount={item.unreadCount}
onPress={onPress}
Expand Down
4 changes: 2 additions & 2 deletions src/topics/TopicListScreen.js
Original file line number Diff line number Diff line change
Expand Up @@ -48,14 +48,14 @@ class TopicListScreen extends PureComponent<Props, State> {
handleFilterChange = (filter: string) => this.setState({ filter });

render() {
const { topics } = this.props;
const { stream, topics } = this.props;
const { filter } = this.state;
const filteredTopics =
topics && topics.filter(topic => topic.name.toLowerCase().includes(filter.toLowerCase()));

return (
<Screen title="Topics" centerContent search searchBarOnChange={this.handleFilterChange}>
<TopicList topics={filteredTopics} onPress={this.handlePress} />
<TopicList stream={stream} topics={filteredTopics} onPress={this.handlePress} />
</Screen>
);
}
Expand Down