Skip to content
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
9 changes: 6 additions & 3 deletions src/chat/narrowsSelectors.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ import {
getOutbox,
} from '../directSelectors';
import { getCaughtUpForNarrow } from '../caughtup/caughtUpSelectors';
import { getAllUsersByEmail, getOwnUser } from '../users/userSelectors';
import { getAllUsersByEmail, getAllUsersById, getOwnUser } from '../users/userSelectors';
import {
isStreamOrTopicNarrow,
emailsOfGroupPmNarrow,
Expand Down Expand Up @@ -149,13 +149,16 @@ export const isNarrowValid: Selector<boolean, Narrow> = createSelector(
(state, narrow) => narrow,
state => getStreams(state),
state => getAllUsersByEmail(state),
(narrow, streams, allUsersByEmail) =>
state => getAllUsersById(state),
(narrow, streams, allUsersByEmail, allUsersById) =>
caseNarrowDefault(
narrow,
{
stream: streamName => streams.find(s => s.name === streamName) !== undefined,
topic: streamName => streams.find(s => s.name === streamName) !== undefined,
pm: emails => emails.every(email => allUsersByEmail.get(email) !== undefined),
pm: (emails, ids) =>
emails.every(email => allUsersByEmail.get(email) !== undefined)
&& ids.every(id => allUsersById.get(id) !== undefined),
},
() => true,
),
Expand Down
10 changes: 2 additions & 8 deletions src/common/GroupAvatar.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ const styles = createStyleSheet({
type Props = $ReadOnly<{|
names: string[],
size: number,
shape: 'rounded' | 'square',
children?: React$Node,
onPress?: () => void,
|}>;
Expand All @@ -43,22 +42,17 @@ export const initialsForGroupIcon = (names: string[]): string => {
*
* @prop names - The name of one or more users, used to extract their initials.
* @prop size - Sets width and height in logical pixels.
* @prop [shape]
* @prop children - If provided, will render inside the component body.
* @prop onPress - Event fired on pressing the component.
*/
export default class GroupAvatar extends PureComponent<Props> {
static defaultProps = {
shape: 'rounded',
};

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

const frameSize = {
height: size,
width: size,
borderRadius: shape === 'rounded' ? size / 8 : 0,
borderRadius: size / 8,
backgroundColor: colorHashFromString(names.join(', ')),
};
const textSize = {
Expand Down
10 changes: 2 additions & 8 deletions src/common/UserAvatar.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ import { AvatarURL } from '../utils/avatar';
type Props = $ReadOnly<{|
avatarUrl: AvatarURL,
size: number,
shape: 'rounded' | 'square',
children?: React$Node,
onPress?: () => void,
|}>;
Expand All @@ -18,18 +17,13 @@ type Props = $ReadOnly<{|
*
* @prop avatarUrl
* @prop size - Sets width and height in logical pixels.
* @prop [shape] - 'rounded' (default) means a square with rounded corners.
* @prop [children] - If provided, will render inside the component body.
* @prop [onPress] - Event fired on pressing the component.
*/
export default class UserAvatar extends PureComponent<Props> {
static defaultProps = {
shape: 'rounded',
};

render() {
const { avatarUrl, children, size, shape, onPress } = this.props;
const borderRadius = shape === 'rounded' ? size / 8 : 0;
const { avatarUrl, children, size, onPress } = this.props;
const borderRadius = size / 8;
const style = {
height: size,
width: size,
Expand Down
60 changes: 47 additions & 13 deletions src/common/UserAvatarWithPresence.js
Original file line number Diff line number Diff line change
@@ -1,11 +1,13 @@
/* @flow strict-local */

import React, { PureComponent } from 'react';
import React, { type ComponentType, PureComponent } from 'react';
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.

user: Add "...ById" variant for UserAvatarWithPresence.

We'll temporarily have two variants of this component -- an
implementation in common but with slightly different interfaces -- as a
way to smoothly migrate from callers using one interface to the other.

The new interface keeps callers from having to know or care about
emails, which we're moving away from.  It also encapsulates the need
to find the user's avatar URL; the caller only has to be concerned
with identifying which user it wants represented, by user ID.  This
improves react-redux hygiene: it means that when that user's avatar
changes, we we no longer have to rerender the calling component,
which fundamentally shouldn't care about the specific avatar URL.

Nit in commit message: "we we"


import { createStyleSheet } from '../styles';
import type { Dispatch } from '../types';
import UserAvatar from './UserAvatar';
import PresenceStatusIndicator from './PresenceStatusIndicator';
import { AvatarURL } from '../utils/avatar';
import { getUserForId } from '../users/userSelectors';
import { connect } from '../react-redux';

const styles = createStyleSheet({
status: {
Expand All @@ -17,33 +19,65 @@ const styles = createStyleSheet({

type Props = $ReadOnly<{|
avatarUrl: AvatarURL,
email: string,
size: number,
shape: 'rounded' | 'square',
onPress?: () => void,
email: string,
|}>;

/**
* Renders a user avatar with a PresenceStatusIndicator in the corner
* A user avatar with a PresenceStatusIndicator in the corner.
*
* Prefer `UserAvatarWithPresenceById` over this component: it does the same
* thing but avoids an email in the component's interface. Once all callers
* have migrated to that version, it'll replace this one.
*
* @prop [avatarUrl]
* @prop [email] - Sender's / user's email address, for the presence dot.
* @prop [size] - Sets width and height in logical pixels.
* @prop [shape] - 'rounded' (default) means a square with rounded corners.
* @prop [onPress] - Event fired on pressing the component.
*/
export default class UserAvatarWithPresence extends PureComponent<Props> {
static defaultProps = {
shape: 'rounded',
};

// The underlying class gets an inexact Props type to express that it's fine
// for it to be passed extra props by the implementation of …ById.
// We don't export it with that type, because we want exact Props types to
// get the most effective type-checking.
class UserAvatarWithPresence extends PureComponent<$ReadOnly<{ ...Props, ... }>> {
render() {
const { avatarUrl, email, size, onPress, shape } = this.props;
const { avatarUrl, email, size, onPress } = this.props;

return (
<UserAvatar avatarUrl={avatarUrl} size={size} onPress={onPress} shape={shape}>
<UserAvatar avatarUrl={avatarUrl} size={size} onPress={onPress}>
<PresenceStatusIndicator style={styles.status} email={email} hideIfOffline />
</UserAvatar>
);
}
}

// Export the class with a tighter constraint on acceptable props, namely
// that the type is an exact object type as usual.
export default (UserAvatarWithPresence: ComponentType<Props>);

/**
* A user avatar with a PresenceStatusIndicator in the corner.
*
* Use this in preference to the default export `UserAvatarWithPresence`.
* We're migrating from that one to this in order to avoid using emails.
*
* @prop [userId]
* @prop [size]
* @prop [onPress]
*/
export const UserAvatarWithPresenceById = connect<{| avatarUrl: AvatarURL, email: string |}, _, _>(
(state, props) => {
const user = getUserForId(state, props.userId);
return { avatarUrl: user.avatar_url, email: user.email };
},
)(
// The type inference embedded in our `connect` type relies on seeing the
// exact Props type for the underlying component, complete with all the
// expected props. Normally that's just how our Props types are in the
// first place, but here we have to provide it explicitly.
/* eslint-disable flowtype/generic-spacing */
(UserAvatarWithPresence: ComponentType<
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.

I think using react-redux's connect here is fine and convenient because we want to create a separate thing with a new interface, instead of modifying the original thing.

But when we settle on using this one and removing the old one, I wonder about trying out React Hooks, in particular, react-redux's useReducer (which is already available with the react-redux version we're on). True, we've gotten a pretty good handle on treating the "inner" and "outer" props in most places, with SelectorProps—but we won't have to do even that, when there's no wrapping component involved. 🙂

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.

Sure, that'd be interesting!

(useSelector, I think.)

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.

(useSelector, I think.)

Ah of course 🤪.

$ReadOnly<{| ...Props, dispatch: Dispatch, userId: number |}>,
>),
);
17 changes: 8 additions & 9 deletions src/compose/ComposeBox.js
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,6 @@ import {
getIsAdmin,
getSession,
getLastMessageTopic,
getActiveUsersByEmail,
getCaughtUpForNarrow,
getStreamInNarrow,
getVideoChatProvider,
Expand All @@ -56,12 +55,12 @@ import {
import { getDraftForNarrow } from '../drafts/draftsSelectors';
import TopicAutocomplete from '../autocomplete/TopicAutocomplete';
import AutocompleteView from '../autocomplete/AutocompleteView';
import { getOwnEmail } from '../users/userSelectors';
import { getActiveUsersById, getOwnUserId } from '../users/userSelectors';

type SelectorProps = {|
auth: Auth,
ownEmail: string,
usersByEmail: Map<string, UserOrBot>,
ownUserId: number,
usersById: Map<number, UserOrBot>,
safeAreaInsets: Dimensions,
isAdmin: boolean,
isAnnouncementOnly: boolean,
Expand Down Expand Up @@ -420,9 +419,9 @@ class ComposeBox extends PureComponent<Props, State> {
render() {
const { isTopicFocused, isMenuExpanded, height, message, topic, selection } = this.state;
const {
ownEmail,
ownUserId,
narrow,
usersByEmail,
usersById,
editMessage,
safeAreaInsets,
isAdmin,
Expand All @@ -441,7 +440,7 @@ class ComposeBox extends PureComponent<Props, State> {
return <AnnouncementOnly />;
}

const placeholder = getComposeInputPlaceholder(narrow, ownEmail, usersByEmail);
const placeholder = getComposeInputPlaceholder(narrow, ownUserId, usersById);
const style = {
paddingBottom: safeAreaInsets.bottom,
backgroundColor: 'hsla(0, 0%, 50%, 0.1)',
Expand Down Expand Up @@ -519,8 +518,8 @@ class ComposeBox extends PureComponent<Props, State> {

export default connect<SelectorProps, _, _>((state, props) => ({
auth: getAuth(state),
ownEmail: getOwnEmail(state),
usersByEmail: getActiveUsersByEmail(state),
ownUserId: getOwnUserId(state),
usersById: getActiveUsersById(state),
safeAreaInsets: getSession(state).safeAreaInsets,
isAdmin: getIsAdmin(state),
isAnnouncementOnly: getIsActiveStreamAnnouncementOnly(state, props.narrow),
Expand Down
14 changes: 7 additions & 7 deletions src/compose/__tests__/getComposeInputPlaceholder-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,12 +11,12 @@ import {
import * as eg from '../../__tests__/lib/exampleData';

describe('getComposeInputPlaceholder', () => {
const usersByEmail = new Map([eg.selfUser, eg.otherUser, eg.thirdUser].map(u => [u.email, u]));
const ownEmail = eg.selfUser.email;
const usersById = new Map([eg.selfUser, eg.otherUser, eg.thirdUser].map(u => [u.user_id, u]));
const ownUserId = eg.selfUser.user_id;

test('returns "Message @ThisPerson" object for person narrow', () => {
const narrow = deepFreeze(pm1to1NarrowFromUser(eg.otherUser));
const placeholder = getComposeInputPlaceholder(narrow, ownEmail, usersByEmail);
const placeholder = getComposeInputPlaceholder(narrow, ownUserId, usersById);
expect(placeholder).toEqual({
text: 'Message {recipient}',
values: { recipient: `@${eg.otherUser.full_name}` },
Expand All @@ -25,25 +25,25 @@ describe('getComposeInputPlaceholder', () => {

test('returns "Jot down something" object for self narrow', () => {
const narrow = deepFreeze(pm1to1NarrowFromUser(eg.selfUser));
const placeholder = getComposeInputPlaceholder(narrow, ownEmail, usersByEmail);
const placeholder = getComposeInputPlaceholder(narrow, ownUserId, usersById);
expect(placeholder).toEqual({ text: 'Jot down something' });
});

test('returns "Message #streamName" for stream narrow', () => {
const narrow = deepFreeze(streamNarrow('Denmark'));
const placeholder = getComposeInputPlaceholder(narrow, ownEmail, usersByEmail);
const placeholder = getComposeInputPlaceholder(narrow, ownUserId, usersById);
expect(placeholder).toEqual({ text: 'Message {recipient}', values: { recipient: '#Denmark' } });
});

test('returns properly for topic narrow', () => {
const narrow = deepFreeze(topicNarrow('Denmark', 'Copenhagen'));
const placeholder = getComposeInputPlaceholder(narrow, ownEmail, usersByEmail);
const placeholder = getComposeInputPlaceholder(narrow, ownUserId, usersById);
expect(placeholder).toEqual({ text: 'Reply' });
});

test('returns "Message group" object for group narrow', () => {
const narrow = deepFreeze(pmNarrowFromUsersUnsafe([eg.otherUser, eg.thirdUser]));
const placeholder = getComposeInputPlaceholder(narrow, ownEmail, usersByEmail);
const placeholder = getComposeInputPlaceholder(narrow, ownUserId, usersById);
expect(placeholder).toEqual({ text: 'Message group' });
});
});
14 changes: 7 additions & 7 deletions src/compose/getComposeInputPlaceholder.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,23 +4,23 @@ import { caseNarrowDefault } from '../utils/narrow';

export default (
narrow: Narrow,
ownEmail: string,
usersByEmail: Map<string, UserOrBot>,
ownUserId: number,
usersById: Map<number, UserOrBot>,
): LocalizableText =>
caseNarrowDefault(
narrow,
{
pm: emails => {
if (emails.length > 1) {
pm: (emails, ids) => {
if (ids.length > 1) {
return { text: 'Message group' };
}
const email = emails[0];
const userId = ids[0];

if (email === ownEmail) {
if (userId === ownUserId) {
return { text: 'Jot down something' };
}

const user = usersByEmail.get(email);
const user = usersById.get(userId);
if (!user) {
return { text: 'Type a message' };
}
Expand Down
18 changes: 9 additions & 9 deletions src/outbox/outboxActions.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import {
} from '../actionConstants';
import { getAuth } from '../selectors';
import * as api from '../api';
import { getAllUsersByEmail, getOwnUser } from '../users/userSelectors';
import { getAllUsersById, getOwnUser } from '../users/userSelectors';
import { getUsersAndWildcards } from '../users/userHelpers';
import { caseNarrowPartial } from '../utils/narrow';
import { BackoffMachine } from '../utils/async';
Expand Down Expand Up @@ -98,13 +98,13 @@ export const sendOutbox = () => async (dispatch: Dispatch, getState: GetState) =
};

// A valid display_recipient with all the thread's users, sorted by ID.
const mapEmailsToUsers = (emails, allUsersByEmail, ownUser) => {
const result = emails.map(email => {
const user = allUsersByEmail.get(email);
const recipientsFromIds = (ids, allUsersById, ownUser) => {
const result = ids.map(id => {
const user = allUsersById.get(id);
if (!user) {
throw new Error('outbox: missing user when preparing to send PM');
}
return { email, id: user.user_id, full_name: user.full_name };
return { id, email: user.email, full_name: user.full_name };
});
if (!result.some(r => r.id === ownUser.user_id)) {
result.push({ email: ownUser.email, id: ownUser.user_id, full_name: ownUser.full_name });
Expand All @@ -120,13 +120,13 @@ type DataFromNarrow = SubsetProperties<

const extractTypeToAndSubjectFromNarrow = (
narrow: Narrow,
allUsersByEmail: Map<string, UserOrBot>,
allUsersById: Map<number, UserOrBot>,
ownUser: UserOrBot,
): DataFromNarrow =>
caseNarrowPartial(narrow, {
pm: emails => ({
pm: (emails, ids) => ({
type: 'private',
display_recipient: mapEmailsToUsers(emails, allUsersByEmail, ownUser),
display_recipient: recipientsFromIds(ids, allUsersById, ownUser),
subject: '',
}),

Expand Down Expand Up @@ -167,7 +167,7 @@ export const addToOutbox = (narrow: Narrow, content: string) => async (
dispatch(
messageSendStart({
isSent: false,
...extractTypeToAndSubjectFromNarrow(narrow, getAllUsersByEmail(state), ownUser),
...extractTypeToAndSubjectFromNarrow(narrow, getAllUsersById(state), ownUser),
markdownContent: content,
content: getContentPreview(content, state),
timestamp: localTime,
Expand Down
8 changes: 4 additions & 4 deletions src/title/Title.js
Original file line number Diff line number Diff line change
Expand Up @@ -30,11 +30,11 @@ export default class Title extends PureComponent<Props> {
allPrivate: () => <TitleSpecial code="private" color={color} />,
stream: () => <TitleStream narrow={narrow} color={color} />,
topic: () => <TitleStream narrow={narrow} color={color} />,
pm: emails =>
emails.length === 1 ? (
<TitlePrivate email={emails[0]} color={color} />
pm: (emails, ids) =>
ids.length === 1 ? (
<TitlePrivate userId={ids[0]} color={color} />
) : (
<TitleGroup narrow={narrow} />
<TitleGroup userIds={ids} />
),
search: () => null,
});
Expand Down
Loading