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.

Sign up for GitHub

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

Jump to bottom

Modify/don't provide 'Reply' option for messages that can't be replied to #3811

Merged
merged 2 commits into from
Feb 4, 2020
Merged

Modify/don't provide 'Reply' option for messages that can't be replied to #3811

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
666b962
message list: Modify "Reply" action when in announcement-only narrow.
agrawal-d Jan 21, 2020
b902d55
message list: Remove "Reply" action when in group PM narrow.
agrawal-d Jan 21, 2020
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
21 changes: 17 additions & 4 deletions src/message/messageActionSheet.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import type { BackgroundData } from '../webview/MessageList';
import {
getNarrowFromMessage,
isHomeNarrow,
isPrivateNarrow,
isPrivateOrGroupNarrow,
isSpecialNarrow,
isTopicNarrow,
} from '../utils/narrow';
Expand Down Expand Up @@ -52,6 +52,12 @@ const reply = ({ message, dispatch, ownEmail }) => {
reply.title = 'Reply';
reply.errorMessage = 'Failed to reply';

const narrowToTopic = ({ message, dispatch, ownEmail }) => {
dispatch(doNarrow(getNarrowFromMessage(message, ownEmail), message.id));
};
narrowToTopic.title = 'Narrow to topic';
narrowToTopic.errorMessage = 'Failed to narrow to topic';

const copyToClipboard = async ({ _, auth, message }) => {
const rawMessage = isAnOutboxMessage(message) /* $FlowFixMe: then really type Outbox */
? message.markdownContent
Expand Down Expand Up @@ -148,6 +154,7 @@ const allButtonsRaw = {
// For messages
addReaction,
reply,
narrowToTopic,
copyToClipboard,
shareMessage,
editMessage,
Expand Down Expand Up @@ -178,6 +185,7 @@ type ConstructSheetParams = {|
backgroundData: BackgroundData,
message: Message | Outbox,
narrow: Narrow,
isAnnouncementOnly: boolean,
|};

export const constructHeaderActionButtons = ({
Expand Down Expand Up @@ -206,9 +214,10 @@ const messageNotDeleted = (message: Message | Outbox): boolean =>
message.content !== '<p>(deleted)</p>';

export const constructMessageActionButtons = ({
backgroundData: { ownEmail, flags },
backgroundData: { ownEmail, flags, isAdmin },
message,
narrow,
isAnnouncementOnly,
}: ConstructSheetParams): ButtonCode[] => {
const buttons = [];
if (message.reactions.length > 0) {
Expand All @@ -217,8 +226,12 @@ export const constructMessageActionButtons = ({
if (!isAnOutboxMessage(message) && messageNotDeleted(message)) {
buttons.push('addReaction');
}
if (!isAnOutboxMessage(message) && !isTopicNarrow(narrow) && !isPrivateNarrow(narrow)) {
buttons.push('reply');
if (!isAnOutboxMessage(message) && !isTopicNarrow(narrow) && !isPrivateOrGroupNarrow(narrow)) {
if (isAnnouncementOnly && !isAdmin) {
buttons.push('narrowToTopic');
} else {
buttons.push('reply');
}
}
Comment on lines +229 to 235
Copy link
Member

Choose a reason for hiding this comment

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

This is too many conditionals in one place 🙂 -- it's more than the reader, in particular the author, can reasonably keep track of in their head.

One symptom of that is that this causes "Narrow to topic" to appear only for non-outbox messages. That doesn't make sense.

This also leaves "Reply" in place when the message is in an announcement-only stream, any time you're in a narrow that isn't specific to the stream. You could be in the all-messages narrow; or starred messages, @-mentions, or a search.

if (messageNotDeleted(message)) {
buttons.push('copyToClipboard');
Expand Down
5 changes: 5 additions & 0 deletions src/webview/MessageList.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ import { getUpdateEvents } from './webViewHandleUpdates';
import { handleMessageListEvent } from './webViewEventHandlers';
import { base64Utf8Encode } from '../utils/encoding';
import * as logging from '../utils/logging';
import { getIsActiveStreamAnnouncementOnly } from '../subscriptions/subscriptionSelectors';

// ESLint doesn't notice how `this.props` escapes, and complains about some
// props not being used here.
Expand All @@ -73,6 +74,7 @@ export type BackgroundData = $ReadOnly<{
mute: MuteState,
ownEmail: string,
ownUserId: number,
isAdmin: boolean,
allImageEmojiById: $ReadOnly<{ [id: string]: ImageEmojiType }>,
twentyFourHourTime: boolean,
subscriptions: Subscription[],
Expand All @@ -87,6 +89,7 @@ type SelectorProps = {|
showMessagePlaceholders: boolean,
theme: ThemeName,
typingUsers: $ReadOnlyArray<User>,
isAnnouncementOnly: boolean,
|};

// TODO get a type for `connectActionSheet` so this gets fully type-checked.
Expand Down Expand Up @@ -340,6 +343,7 @@ export default connect<SelectorProps, _, _>((state, props: OuterProps) => {
mute: getMute(state),
ownEmail: getOwnEmail(state),
ownUserId: getOwnUser(state).user_id,
isAdmin: getOwnUser(state).is_admin,
allImageEmojiById: getAllImageEmojiById(state),
subscriptions: getSubscriptions(state),
twentyFourHourTime: getRealm(state).twentyFourHourTime,
Expand All @@ -357,5 +361,6 @@ export default connect<SelectorProps, _, _>((state, props: OuterProps) => {
: getShowMessagePlaceholders(props.narrow)(state),
theme: getSettings(state).theme,
typingUsers: props.typingUsers || getCurrentTypingUsers(state, props.narrow),
isAnnouncementOnly: getIsActiveStreamAnnouncementOnly(state, props.narrow),
};
})(connectActionSheet(withGetText(MessageList)));
10 changes: 9 additions & 1 deletion src/webview/webViewEventHandlers.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,7 @@ type Props = $ReadOnly<{
messages: $ReadOnlyArray<Message | Outbox>,
narrow: Narrow,
showActionSheetWithOptions: ShowActionSheetWithOptions,
isAnnouncementOnly: boolean,
}>;

const fetchMore = (props: Props, event: MessageListEventScroll) => {
Expand Down Expand Up @@ -180,11 +181,18 @@ const handleLongPress = (
if (!message) {
return;
}
const { dispatch, showActionSheetWithOptions, backgroundData, narrow } = props;
const {
dispatch,
showActionSheetWithOptions,
backgroundData,
narrow,
isAnnouncementOnly,
} = props;
showActionSheet(target === 'header', dispatch, showActionSheetWithOptions, _, {
backgroundData,
message,
narrow,
isAnnouncementOnly,
});
Comment on lines 191 to 196
Copy link
Member

Choose a reason for hiding this comment

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

One bonus of using backgroundData for the plumbing: it saves us from making a longer and longer list here of things we just pass through without change. (That was part of the origin of the backgroundData object in this message-list code.)

};

Expand Down
2 changes: 2 additions & 0 deletions static/translations/messages_en.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@
"Narrow to conversation": "Narrow to conversation",
"Reply": "Reply",
"Add a reaction": "Add a reaction",
"Narrow to topic": "Narrow to topic",
"Copy to clipboard": "Copy to clipboard",
"Link copied to clipboard": "Link copied to clipboard",
"Mute topic": "Mute topic",
Expand Down Expand Up @@ -122,6 +123,7 @@
"Edit message": "Edit message",
"Network request failed": "Network request failed",
"Failed to add reaction": "Failed to add reaction",
"Failed to narrow to topic": "Failed to narrow to topic",
"Failed to reply": "Failed to reply",
"Failed to copy message to clipboard": "Failed to copy message to clipboard",
"Failed to share message": "Failed to share message",
Expand Down