Skip to content

Commit 13c1e61

Browse files
jackrzhanggnprice
authored andcommitted
webview render: Refer to state.flags instead of message.flags.
This is part of fixing #2676. As discussed in the PR thread #2843, the `flags` property on a message object in our Redux store never changes after we first receive the message, and instead we maintain an updated version under the separate top-level state subtree `flags`; but we've been using the message object's stale `flags` for rendering the message in the message list. This change switches to using the up-to-date flags from `state.flags`, via the `getFlags` selector, instead. Now when the user stars or unstars a message, we still don't yet update the message list (that's for a followup commit); but now at least the next message list we render from scratch will reflect the update. In making this change, we get a bit of a head start because we already invoke `getFlags` to populate the `flags` prop of `MessageList` (in its `connect` call), which gets passed down to `renderMessagesAsHtml`. So we just need to use it there, and adapt the layers underneath to take the different data format found in `state.flags`. Of the two spots where we actually use `flags` in message rendering: * `messageTagsAsHtml` gets a bit simpler; it only needs to know whether or not the message is starred, so we just pass it a boolean for that. * `messageDiv` actually uses the flags as a list of strings (for now) to generate HTML data-* attributes, so we make a little helper function for it to just compute that list. [greg: explained more context in commit message]
1 parent 3168ff5 commit 13c1e61

File tree

4 files changed

+40
-9
lines changed

4 files changed

+40
-9
lines changed

src/webview/__tests__/webViewHandleUpdates-test.js

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
import { getInputMessages } from '../webViewHandleUpdates';
2+
import { flagsStateToStringList } from '../html/messageAsHtml';
23

34
describe('getInputMessages', () => {
45
test('missing prev and next props returns no messages', () => {
@@ -57,11 +58,13 @@ describe('getInputMessages', () => {
5758
const prevProps = {
5859
auth: { realm: '' },
5960
messages: [],
61+
flags: { starred: {} },
6062
renderedMessages: [{ key: 0, data: [], message: {} }],
6163
};
6264
const nextProps = {
6365
auth: { realm: '' },
6466
messages: [],
67+
flags: { starred: {} },
6568
renderedMessages: [
6669
{
6770
key: 0,
@@ -103,12 +106,14 @@ describe('getInputMessages', () => {
103106
typingUsers: [],
104107
messages: [],
105108
renderedMessages: [{ key: 0, data: [], message: {} }],
109+
flags: { starred: {} },
106110
};
107111
const nextProps = {
108112
auth: { realm: '' },
109113
fetching: { older: false, newer: true },
110114
typingUsers: [{ id: 10 }],
111115
messages: [],
116+
flags: { starred: {} },
112117
renderedMessages: [
113118
{
114119
key: 0,
@@ -124,3 +129,25 @@ describe('getInputMessages', () => {
124129
expect(messages[0].type).toEqual('content');
125130
});
126131
});
132+
133+
describe('flagsStateToStringList', () => {
134+
const flags = {
135+
read: {
136+
1: true,
137+
2: true,
138+
},
139+
starred: {
140+
1: true,
141+
3: true,
142+
},
143+
mentions: {},
144+
// ...
145+
// the actual store keeps track of many more flags
146+
};
147+
148+
test("returns a string list of flags for some message, given some FlagsState and the message's id", () => {
149+
expect(flagsStateToStringList(flags, 1)).toEqual(['read', 'starred']);
150+
expect(flagsStateToStringList(flags, 2)).toEqual(['read']);
151+
expect(flagsStateToStringList(flags, 3)).toEqual(['starred']);
152+
});
153+
});

src/webview/html/messageAsHtml.js

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -5,12 +5,17 @@ import { shortTime } from '../../utils/date';
55
import messageTagsAsHtml from './messageTagsAsHtml';
66
import messageReactionListAsHtml from './messageReactionListAsHtml';
77

8-
const messageDiv = (id: number, msgClass: string, flags: Object): string =>
8+
export const flagsStateToStringList = (flags: FlagsState, id: number): string[] =>
9+
Object.keys(flags).filter(key => flags[key][id]);
10+
11+
const messageDiv = (id: number, msgClass: string, flags: FlagsState): string =>
912
template`<div
1013
class="message ${msgClass}"
1114
id="msg-${id}"
1215
data-msg-id="${id}"
13-
$!${flags.map(flag => template`data-${flag}="true" `).join('')}
16+
$!${flagsStateToStringList(flags, id)
17+
.map(flag => template`data-${flag}="true" `)
18+
.join('')}
1419
>`;
1520

1621
const messageSubheader = ({
@@ -66,7 +71,7 @@ const messageBody = ({
6671
timeEdited,
6772
}: {
6873
content: string,
69-
flags: Object,
74+
flags: FlagsState,
7075
id: number,
7176
isOutbox: boolean,
7277
ownEmail: string,
@@ -76,7 +81,7 @@ const messageBody = ({
7681
}) => template`
7782
$!${content}
7883
$!${isOutbox ? '<div class="loading-spinner outbox-spinner"></div>' : ''}
79-
$!${messageTagsAsHtml(flags, timeEdited)}
84+
$!${messageTagsAsHtml(!!flags.starred[id], timeEdited)}
8085
$!${messageReactionListAsHtml(reactions, id, ownEmail, realmEmoji)}
8186
`;
8287

src/webview/html/messageTagsAsHtml.js

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2,8 +2,7 @@
22
import distanceInWordsToNow from 'date-fns/distance_in_words_to_now';
33
import template from './template';
44

5-
export default (flags: Object, timeEdited: ?number): string => {
6-
const isStarred = flags.indexOf('starred') > -1;
5+
export default (isStarred: boolean, timeEdited: ?number): string => {
76
if (timeEdited === undefined && !isStarred) {
87
return '';
98
}
@@ -12,7 +11,7 @@ export default (flags: Object, timeEdited: ?number): string => {
1211

1312
return template`<div class="message-tags">
1413
$!${timeEdited ? template`<span class="message-tag">edited ${editedTime} ago</span>` : ''}
15-
$!${flags.indexOf('starred') > -1 ? '<span class="message-tag">starred</span>' : ''}
14+
$!${isStarred ? '<span class="message-tag">starred</span>' : ''}
1615
</div>
1716
`;
1817
};

src/webview/html/renderMessagesAsHtml.js

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ export default ({
1010
auth,
1111
subscriptions,
1212
realmEmoji,
13+
flags,
1314
renderedMessages,
1415
narrow,
1516
twentyFourHourTime,
@@ -37,8 +38,7 @@ export default ({
3738
fromName: message.sender_full_name,
3839
fromEmail: message.sender_email,
3940
content: message.match_content || message.content,
40-
// $FlowFixMe: about to be fixed by #2922
41-
flags: message.flags || [],
41+
flags,
4242
timestamp: message.timestamp,
4343
avatarUrl: message.avatar_url || getGravatarFromEmail(message.sender_email),
4444
timeEdited: message.last_edit_timestamp,

0 commit comments

Comments
 (0)