Skip to content
Open
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
1 change: 1 addition & 0 deletions src/common/Icons.js
Original file line number Diff line number Diff line change
Expand Up @@ -83,3 +83,4 @@ export const IconTerminal = makeIcon(Feather, 'terminal');
export const IconMoreHorizontal = makeIcon(Feather, 'more-horizontal');
export const IconEdit = makeIcon(Feather, 'edit');
export const IconPlusSquare = makeIcon(Feather, 'plus-square');
export const IconCheckMarkCircle = makeIcon(MaterialIcon, 'check-circle');
206 changes: 195 additions & 11 deletions src/main/HomeTab.js
Original file line number Diff line number Diff line change
@@ -1,18 +1,29 @@
/* @flow strict-local */

import React, { PureComponent } from 'react';
import { StyleSheet, View } from 'react-native';
import { StyleSheet, View, BackHandler, Alert } from 'react-native';

import type { Dispatch } from '../types';
import { Set } from 'immutable';
import type { Dispatch, Narrow, GetText, Auth, Subscription } from '../types';
import { connect } from '../react-redux';
import { HOME_NARROW, MENTIONED_NARROW, STARRED_NARROW } from '../utils/narrow';
import { TranslationContext } from '../boot/TranslationProvider';
import {
HOME_NARROW,
MENTIONED_NARROW,
STARRED_NARROW,
topicNarrow,
caseNarrowPartial,
} from '../utils/narrow';
import NavButton from '../nav/NavButton';
import NavButtonGeneral from '../nav/NavButtonGeneral';
import UnreadCards from '../unread/UnreadCards';
import { doNarrow, navigateToSearch } from '../actions';
import IconUnreadMentions from '../nav/IconUnreadMentions';
import { BRAND_COLOR } from '../styles';
import { LoadingBanner } from '../common';
import commonStyles, { BRAND_COLOR, NAVBAR_SIZE } from '../styles';
import * as api from '../api';
import { getAuth, getSubscriptionsByName } from '../selectors';
import { LoadingBanner, Label } from '../common';
import { showToast } from '../utils/info';

const styles = StyleSheet.create({
wrapper: {
Expand All @@ -23,18 +34,143 @@ const styles = StyleSheet.create({
justifyContent: 'space-between',
flexDirection: 'row',
},
bulkSelectionNav: {
flexDirection: 'row',
height: NAVBAR_SIZE,
alignItems: 'center',
borderBottomWidth: 1,
borderColor: 'hsla(0, 0%, 50%, 0.25)',
},
selectionCountText: {
textAlign: 'left',
marginLeft: 8,
},
button: {
paddingHorizontal: 8,
paddingVertical: 4,
borderRadius: 18,
marginLeft: 4,
marginRight: 4,
},
});

type SelectorProps = $ReadOnly<{|
auth: Auth,
subscriptionsByName: Map<string, Subscription>,
|}>;

type Props = $ReadOnly<{|
dispatch: Dispatch,
...SelectorProps,
|}>;

class HomeTab extends PureComponent<Props> {
render() {
type State = $ReadOnly<{|
bulkSelection: Set<string> | null,
|}>;

class HomeTab extends PureComponent<Props, State> {
static contextType = TranslationContext;
backHandler;
context: GetText;
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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?

state = {
bulkSelection: null,
};

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

Choose a reason for hiding this comment

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

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

}
return false;
};

componentDidMount() {
this.backHandler = BackHandler.addEventListener('hardwareBackPress', this.handleBackPress);
}

componentWillUnmount() {
this.backHandler.remove();
}

handleTopicSelect = (stream: string, topic: string) => {
const { bulkSelection } = this.state;
const narrow = JSON.stringify(topicNarrow(stream, topic));

if (bulkSelection == null) {
const selection = Set<Narrow>([narrow]);
this.setState({ bulkSelection: selection });
return;
}

if (bulkSelection.has(narrow)) {
this.setState({ bulkSelection: bulkSelection.delete(narrow) });
} else {
this.setState({ bulkSelection: bulkSelection.add(narrow) });
}
};

processBulkCommon = (type: 'mute' | 'read') => {
const { auth, subscriptionsByName } = this.props;
const { bulkSelection } = this.state;
const _ = this.context;
const apiPromises = [];

if (type === 'mute') {
showToast(_('Muting selected topics'));
Copy link
Copy Markdown
Collaborator

@chrisbobbe chrisbobbe Aug 5, 2020

Choose a reason for hiding this comment

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

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

} else {
showToast(_('Marking selected topics as read'));
}

if (bulkSelection == null) {
return;
}

bulkSelection.forEach(narrow => {
const parsedNarrow: Narrow = JSON.parse(narrow);
caseNarrowPartial(parsedNarrow, {
topic: (stream: string, topic: string) => {
if (type === 'mute') {
apiPromises.push(api.muteTopic(auth, stream, topic));
} else {
const subscription = subscriptionsByName.get(stream);
if (subscription === undefined) {
return;
}
apiPromises.push(api.markTopicAsRead(auth, subscription.stream_id, topic));
}
},
});
});

this.setState({ bulkSelection: null });

Promise.all(apiPromises).catch(err => {
const alertTitle =
type === 'mute' ? _('Failed to mute some topics') : _('Failed to mark some topics as read');

Alert.alert(alertTitle, err.message);
});
};

muteBulkSelection = () => {
this.processBulkCommon('mute');
};

readBulkSelection = () => {
this.processBulkCommon('read');
};

cancelBulkSelection = () => {
this.setState({ bulkSelection: null });
};

renderHeader = () => {
const { dispatch } = this.props;
const { bulkSelection } = this.state;
const _ = this.context;

return (
<View style={styles.wrapper}>
if (bulkSelection === null) {
return (
<View style={styles.iconList}>
<NavButton
name="globe"
Expand Down Expand Up @@ -62,11 +198,59 @@ class HomeTab extends PureComponent<Props> {
}}
/>
</View>
);
}

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

Choose a reason for hiding this comment

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

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

defaultMessage: '{count} topics selected',
},
{ count: bulkSelection.size },
);

return (
<View style={styles.bulkSelectionNav}>
<NavButton
name="arrow-left"
color={color}
onPress={this.cancelBulkSelection}
title={_('Cancel')}
/>
<View style={commonStyles.navWrapper}>
<Label text={countText} style={styles.selectionCountText} />
</View>
<NavButton
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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.

name="volume-x"
color={color}
onPress={this.muteBulkSelection}
title={_('Mute')}
/>
<NavButton
name="check"
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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:

color={color}
onPress={this.readBulkSelection}
title={_('Mark as read')}
/>
</View>
);
};

render() {
const { bulkSelection } = this.state;

return (
<View style={styles.wrapper}>
{this.renderHeader()}
<LoadingBanner />
<UnreadCards />
<UnreadCards bulkSelection={bulkSelection} handleTopicSelect={this.handleTopicSelect} />
</View>
);
}
}

export default connect<{||}, _, _>()(HomeTab);
export default connect<SelectorProps, _, _>((state, props) => ({
auth: getAuth(state),
subscriptionsByName: getSubscriptionsByName(state),
}))(HomeTab);
5 changes: 3 additions & 2 deletions src/nav/NavButton.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ type Props = $ReadOnly<{|
style?: TextStyleProp,
name: IconNames,
onPress: () => void,
title?: string,
|}>;

export default class NavButton extends PureComponent<Props> {
Expand All @@ -28,10 +29,10 @@ export default class NavButton extends PureComponent<Props> {
});

render() {
const { name, style, color, onPress } = this.props;
const { name, style, color, onPress, title } = this.props;

return (
<NavButtonGeneral onPress={onPress}>
<NavButtonGeneral onPress={onPress} title={title}>
<Icon style={[this.styles.navButtonIcon, style]} color={color} name={name} />
</NavButtonGeneral>
);
Expand Down
12 changes: 11 additions & 1 deletion src/nav/NavButtonGeneral.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,12 @@ import type { Node as React$Node } from 'react';

import { NAVBAR_SIZE } from '../styles';
import { Touchable } from '../common';
import { showToast } from '../utils/info';

type Props = $ReadOnly<{|
children: React$Node,
onPress: () => void,
title?: string,
|}>;

export default class NavButtonGeneral extends PureComponent<Props> {
Expand All @@ -21,11 +23,19 @@ export default class NavButtonGeneral extends PureComponent<Props> {
},
});

handleLongPress = () => {
const { title } = this.props;

if (title !== undefined) {
showToast(title);
}
};

render() {
const { children, onPress } = this.props;

return (
<Touchable onPress={onPress}>
<Touchable onPress={onPress} onLongPress={this.handleLongPress}>
<View style={this.styles.navButtonFrame}>{children}</View>
</Touchable>
);
Expand Down
41 changes: 37 additions & 4 deletions src/streams/TopicItem.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,9 @@
import React, { PureComponent } from 'react';
import { StyleSheet, View } from 'react-native';

import styles, { BRAND_COLOR } from '../styles';
import styles, { BRAND_COLOR, HALF_COLOR } from '../styles';
import { RawLabel, Touchable, UnreadCount } from '../common';
import { showToast } from '../utils/info';
import { IconCheckMarkCircle } from '../common/Icons';

const componentStyles = StyleSheet.create({
selectedRow: {
Expand All @@ -19,22 +19,38 @@ const componentStyles = StyleSheet.create({
muted: {
opacity: 0.5,
},
checkIcon: {
marginRight: 12,
},
emptyCircle: {
width: 24,
height: 24,
borderRadius: 12,
borderWidth: 1,
borderColor: HALF_COLOR,
marginRight: 12,
},
});

type Props = $ReadOnly<{|
stream: string,
name: string,
isMuted: boolean,
isSelected: boolean,
inBulkSelectionMode: boolean,
isBulkSelected: boolean,
unreadCount: number,
onPress: (topic: string, stream: string) => void,
onLongPress: (topic: string, stream: string) => void,
|}>;

export default class TopicItem extends PureComponent<Props> {
static defaultProps = {
stream: '',
isMuted: false,
isSelected: false,
inBulkSelectionMode: false,
isBulkSelected: false,
unreadCount: 0,
};

Expand All @@ -44,8 +60,24 @@ export default class TopicItem extends PureComponent<Props> {
};

handleLongPress = () => {
const { name } = this.props;
showToast(name);
const { name, stream, onLongPress } = this.props;
onLongPress(stream, name);
};

renderBulkSelectionIcon = () => {
const { isBulkSelected, inBulkSelectionMode } = this.props;

if (isBulkSelected) {
return (
<IconCheckMarkCircle style={componentStyles.checkIcon} size={24} color={BRAND_COLOR} />
);
}

if (inBulkSelectionMode) {
Copy link
Copy Markdown
Collaborator

@chrisbobbe chrisbobbe Aug 5, 2020

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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.

return <View style={componentStyles.emptyCircle} />;
}

return null;
};

render() {
Expand All @@ -60,6 +92,7 @@ export default class TopicItem extends PureComponent<Props> {
isMuted && componentStyles.muted,
]}
>
{this.renderBulkSelectionIcon()}
<RawLabel
style={[componentStyles.label, isSelected && componentStyles.selectedText]}
text={name}
Expand Down
1 change: 1 addition & 0 deletions src/topics/TopicList.js
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ export default class TopicList extends PureComponent<Props> {
isMuted={item.isMuted}
unreadCount={item.unreadCount}
onPress={onPress}
onLongPress={() => {}}
/>
)}
/>
Expand Down
Loading