UX: groups deleted messages (#21411)

Any continuous series of deleted messages will now be grouped into one single expand button.
This commit is contained in:
Joffrey JAFFEUX 2023-05-05 17:08:33 +02:00 committed by GitHub
parent ae3231e140
commit cb5e5f3e5b
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
13 changed files with 325 additions and 215 deletions

View File

@ -108,7 +108,7 @@ export default class ChatLivePane extends Component {
// Technically we could keep messages to avoid re-fetching them, but
// it's not worth the complexity for now
this.args.channel?.messagesManager?.clearMessages();
this.args.channel?.clearMessages();
if (this._loadedChannelId !== this.args.channel?.id) {
this._unsubscribeToUpdates(this._loadedChannelId);
@ -183,7 +183,7 @@ export default class ChatLivePane extends Component {
results
);
this.args.channel.messages = messages;
this.args.channel.addMessages(messages);
this.args.channel.details = meta;
if (this.requestedTargetMessageId) {
@ -224,8 +224,8 @@ export default class ChatLivePane extends Component {
const loadingMoreKey = `loadingMore${capitalize(direction)}`;
const canLoadMore = loadingPast
? this.#messagesManager.canLoadMorePast
: this.#messagesManager.canLoadMoreFuture;
? this.args.channel?.canLoadMorePast
: this.args.channel?.canLoadMoreFuture;
if (
!canLoadMore ||
@ -276,7 +276,7 @@ export default class ChatLivePane extends Component {
}
this.args.channel.details = meta;
this.#messagesManager.addMessages(messages);
this.args.channel.addMessages(messages);
// Edge case for IOS to avoid blank screens
// and/or scrolling to bottom losing track of scroll position
@ -367,7 +367,7 @@ export default class ChatLivePane extends Component {
@debounce(100)
highlightOrFetchMessage(messageId) {
const message = this.#messagesManager?.findMessage(messageId);
const message = this.args.channel?.findMessage(messageId);
if (message) {
this.scrollToMessage(message.id, {
highlight: true,
@ -388,7 +388,7 @@ export default class ChatLivePane extends Component {
return;
}
const message = this.#messagesManager?.findMessage(messageId);
const message = this.args.channel?.findMessage(messageId);
if (message?.deletedAt && opts.autoExpand) {
message.expanded = true;
}
@ -485,7 +485,7 @@ export default class ChatLivePane extends Component {
return;
}
if (this.#messagesManager?.canLoadMoreFuture) {
if (this.args.channel?.canLoadMoreFuture) {
this._fetchAndScrollToLatest();
} else if (this.args.channel.messages?.length > 0) {
this.scrollToMessage(
@ -534,9 +534,9 @@ export default class ChatLivePane extends Component {
}
removeMessage(msgData) {
const message = this.#messagesManager.findMessage(msgData.id);
const message = this.args.channel?.findMessage(msgData.id);
if (message) {
this.#messagesManager.removeMessage(message);
this.args.channel?.removeMessage(message);
}
}
@ -557,7 +557,7 @@ export default class ChatLivePane extends Component {
if (data.chat_message.user.id === this.currentUser.id && data.staged_id) {
const stagedMessage = handleStagedMessage(
this.args.channel,
this.#messagesManager,
this.args.channel.messagesManager,
data
);
if (stagedMessage) {
@ -565,20 +565,19 @@ export default class ChatLivePane extends Component {
}
}
if (this.#messagesManager.canLoadMoreFuture) {
if (this.args.channel?.canLoadMoreFuture) {
// If we can load more messages, we just notice the user of new messages
this.hasNewMessages = true;
} else if (this.#isTowardsBottom()) {
// If we are at the bottom, we append the message and scroll to it
const message = ChatMessage.create(this.args.channel, data.chat_message);
this.#messagesManager.addMessages([message]);
this.args.channel.addMessages([message]);
this.scrollToLatestMessage();
this.updateLastReadMessage();
} else {
// If we are almost at the bottom, we append the message and notice the user
const message = ChatMessage.create(this.args.channel, data.chat_message);
this.#messagesManager.addMessages([message]);
this.args.channel.addMessages([message]);
this.hasNewMessages = true;
}
}
@ -589,10 +588,6 @@ export default class ChatLivePane extends Component {
return this.isDestroying || this.isDestroyed;
}
get #messagesManager() {
return this.args.channel?.messagesManager;
}
@action
onSendMessage(message) {
if (message.editing) {
@ -711,7 +706,7 @@ export default class ChatLivePane extends Component {
}
_onSendError(id, error) {
const stagedMessage = this.#messagesManager.findStagedMessage(id);
const stagedMessage = this.args.channel.findStagedMessage(id);
if (stagedMessage) {
if (error.jqXHR?.responseJSON?.errors?.length) {
// only network errors are retryable

View File

@ -1,202 +1,206 @@
{{! template-lint-disable no-invalid-interactive }}
{{#if (eq @context "channel")}}
<ChatMessageSeparatorDate @message={{@message}} />
<ChatMessageSeparatorNew @message={{@message}} />
{{/if}}
{{#if this.shouldRender}}
{{#if (eq @context "channel")}}
<ChatMessageSeparatorDate @message={{@message}} />
<ChatMessageSeparatorNew @message={{@message}} />
{{/if}}
<div
{{will-destroy this.teardownChatMessage}}
{{did-insert this.decorateCookedMessage}}
{{did-update this.decorateCookedMessage @message.id}}
{{did-update this.decorateCookedMessage @message.version}}
{{on "touchmove" this.handleTouchMove passive=true}}
{{on "touchstart" this.handleTouchStart passive=true}}
{{on "touchend" this.handleTouchEnd passive=true}}
{{on "mouseenter" this.onMouseEnter}}
{{on "mouseleave" this.onMouseLeave}}
{{on "mousemove" this.onMouseMove}}
class={{concat-class
"chat-message-container"
(if this.pane.selectingMessages "selecting-messages")
(if @message.highlighted "highlighted")
}}
data-id={{@message.id}}
data-thread-id={{@message.thread.id}}
{{chat/track-message
(hash
didEnterViewport=(fn @messageDidEnterViewport @message)
didLeaveViewport=(fn @messageDidLeaveViewport @message)
)
}}
>
{{#if this.show}}
{{#if this.pane.selectingMessages}}
<Input
@type="checkbox"
class="chat-message-selector"
@checked={{@message.selected}}
{{on "click" this.toggleChecked}}
/>
{{/if}}
{{#if this.deletedAndCollapsed}}
<div class="chat-message-deleted">
<DButton
@class="btn-flat chat-message-expand"
@action={{this.expand}}
@label="chat.deleted"
<div
{{will-destroy this.teardownChatMessage}}
{{did-insert this.decorateCookedMessage}}
{{did-update this.decorateCookedMessage @message.id}}
{{did-update this.decorateCookedMessage @message.version}}
{{on "touchmove" this.handleTouchMove passive=true}}
{{on "touchstart" this.handleTouchStart passive=true}}
{{on "touchend" this.handleTouchEnd passive=true}}
{{on "mouseenter" this.onMouseEnter}}
{{on "mouseleave" this.onMouseLeave}}
{{on "mousemove" this.onMouseMove}}
class={{concat-class
"chat-message-container"
(if this.pane.selectingMessages "selecting-messages")
(if @message.highlighted "highlighted")
}}
data-id={{@message.id}}
data-thread-id={{@message.thread.id}}
{{chat/track-message
(hash
didEnterViewport=(fn @messageDidEnterViewport @message)
didLeaveViewport=(fn @messageDidLeaveViewport @message)
)
}}
>
{{#if this.show}}
{{#if this.pane.selectingMessages}}
<Input
@type="checkbox"
class="chat-message-selector"
@checked={{@message.selected}}
{{on "click" this.toggleChecked}}
/>
</div>
{{else if this.hiddenAndCollapsed}}
<div class="chat-message-hidden">
<DButton
@class="btn-flat chat-message-expand"
@action={{this.expand}}
@label="chat.hidden"
/>
</div>
{{else}}
<div
class={{concat-class
"chat-message"
(if @message.staged "chat-message-staged")
(if @message.deletedAt "deleted")
(if (and @message.inReplyTo (not this.hideReplyToInfo)) "is-reply")
(if this.showThreadIndicator "is-threaded")
(if this.hideUserInfo "user-info-hidden")
(if @message.error "errored")
(if @message.bookmark "chat-message-bookmarked")
(if (eq @message.id this.chat.activeMessage.model.id) "is-active")
}}
>
{{#unless this.hideReplyToInfo}}
<ChatMessageInReplyToIndicator @message={{@message}} />
{{/unless}}
{{/if}}
{{#if this.hideUserInfo}}
<ChatMessageLeftGutter @message={{@message}} />
{{else}}
<ChatMessageAvatar @message={{@message}} />
{{/if}}
<div class="chat-message-content">
{{#unless this.hideUserInfo}}
<ChatMessageInfo @message={{@message}} />
{{#if this.deletedAndCollapsed}}
<div class="chat-message-deleted">
<DButton
@class="btn-flat chat-message-expand"
@action={{this.expand}}
@translatedLabel={{this.deletedMessageLabel}}
/>
</div>
{{else if this.hiddenAndCollapsed}}
<div class="chat-message-hidden">
<DButton
@class="btn-flat chat-message-expand"
@action={{this.expand}}
@label="chat.hidden"
/>
</div>
{{else}}
<div
class={{concat-class
"chat-message"
(if @message.staged "chat-message-staged")
(if @message.deletedAt "deleted")
(if (and @message.inReplyTo (not this.hideReplyToInfo)) "is-reply")
(if this.showThreadIndicator "is-threaded")
(if this.hideUserInfo "user-info-hidden")
(if @message.error "errored")
(if @message.bookmark "chat-message-bookmarked")
(if (eq @message.id this.chat.activeMessage.model.id) "is-active")
}}
>
{{#unless this.hideReplyToInfo}}
<ChatMessageInReplyToIndicator @message={{@message}} />
{{/unless}}
<ChatMessageText
@cooked={{@message.cooked}}
@uploads={{@message.uploads}}
@edited={{@message.edited}}
>
{{#if @message.reactions.length}}
<div class="chat-message-reaction-list">
{{#if this.reactionLabel}}
<div class="reaction-users-list">
{{replace-emoji this.reactionLabel}}
</div>
{{/if}}
{{#if this.hideUserInfo}}
<ChatMessageLeftGutter @message={{@message}} />
{{else}}
<ChatMessageAvatar @message={{@message}} />
{{/if}}
{{#each @message.reactions as |reaction|}}
<ChatMessageReaction
@reaction={{reaction}}
@onReaction={{this.messageInteractor.react}}
@message={{@message}}
@showTooltip={{true}}
/>
{{/each}}
<div class="chat-message-content">
{{#unless this.hideUserInfo}}
<ChatMessageInfo @message={{@message}} />
{{/unless}}
{{#if this.chat.userCanInteractWithChat}}
{{#unless this.site.mobileView}}
<DButton
@class="chat-message-react-btn"
@action={{this.messageInteractor.openEmojiPicker}}
@icon="discourse-emojis"
@title="chat.react"
@forwardEvent={{true}}
<ChatMessageText
@cooked={{@message.cooked}}
@uploads={{@message.uploads}}
@edited={{@message.edited}}
>
{{#if @message.reactions.length}}
<div class="chat-message-reaction-list">
{{#if this.reactionLabel}}
<div class="reaction-users-list">
{{replace-emoji this.reactionLabel}}
</div>
{{/if}}
{{#each @message.reactions as |reaction|}}
<ChatMessageReaction
@reaction={{reaction}}
@onReaction={{this.messageInteractor.react}}
@message={{@message}}
@showTooltip={{true}}
/>
{{/unless}}
{{/each}}
{{#if this.chat.userCanInteractWithChat}}
{{#unless this.site.mobileView}}
<DButton
@class="chat-message-react-btn"
@action={{this.messageInteractor.openEmojiPicker}}
@icon="discourse-emojis"
@title="chat.react"
@forwardEvent={{true}}
/>
{{/unless}}
{{/if}}
</div>
{{/if}}
</ChatMessageText>
{{#if @message.error}}
<div class="chat-send-error">
{{#if (eq @message.error "network_error")}}
<DButton
class="retry-staged-message-btn"
@action={{fn @resendStagedMessage @message}}
@icon="exclamation-circle"
>
<span class="retry-staged-message-btn__title">
{{i18n "chat.retry_staged_message.title"}}
</span>
<span class="retry-staged-message-btn__action">
{{i18n "chat.retry_staged_message.action"}}
</span>
</DButton>
{{else}}
{{@message.error}}
{{/if}}
</div>
{{/if}}
</ChatMessageText>
{{#if @message.error}}
<div class="chat-send-error">
{{#if (eq @message.error "network_error")}}
<DButton
class="retry-staged-message-btn"
@action={{fn @resendStagedMessage @message}}
@icon="exclamation-circle"
>
<span class="retry-staged-message-btn__title">
{{i18n "chat.retry_staged_message.title"}}
{{#if this.mentionWarning}}
<div class="alert alert-info chat-message-mention-warning">
{{#if this.mentionWarning.invitation_sent}}
{{d-icon "check"}}
<span>
{{i18n
"chat.mention_warning.invitations_sent"
count=this.mentionWarning.without_membership.length
}}
</span>
<span class="retry-staged-message-btn__action">
{{i18n "chat.retry_staged_message.action"}}
</span>
</DButton>
{{else}}
{{@message.error}}
{{/if}}
</div>
{{/if}}
{{else}}
<FlatButton
@class="dismiss-mention-warning"
@title="chat.mention_warning.dismiss"
@action={{this.dismissMentionWarning}}
@icon="times"
/>
{{#if this.mentionWarning}}
<div class="alert alert-info chat-message-mention-warning">
{{#if this.mentionWarning.invitation_sent}}
{{d-icon "check"}}
<span>
{{i18n
"chat.mention_warning.invitations_sent"
count=this.mentionWarning.without_membership.length
}}
</span>
{{else}}
<FlatButton
@class="dismiss-mention-warning"
@title="chat.mention_warning.dismiss"
@action={{this.dismissMentionWarning}}
@icon="times"
/>
{{#if this.mentionWarning.cannot_see}}
<p class="warning-item cannot-see">
{{this.mentionedCannotSeeText}}
</p>
{{/if}}
{{#if this.mentionWarning.cannot_see}}
<p class="warning-item cannot-see">
{{this.mentionedCannotSeeText}}
</p>
{{/if}}
{{#if this.mentionWarning.without_membership}}
<p class="warning-item without-membership">
<span>{{this.mentionedWithoutMembershipText}}</span>
<a
class="invite-link"
href
onclick={{this.inviteMentioned}}
>
{{i18n "chat.mention_warning.invite"}}
</a>
</p>
{{/if}}
{{#if this.mentionWarning.group_mentions_disabled}}
<p class="warning-item">
{{this.groupsWithDisabledMentions}}
</p>
{{/if}}
{{#if this.mentionWarning.without_membership}}
<p class="warning-item without-membership">
<span>{{this.mentionedWithoutMembershipText}}</span>
<a
class="invite-link"
href
onclick={{this.inviteMentioned}}
>
{{i18n "chat.mention_warning.invite"}}
</a>
</p>
{{/if}}
{{#if this.mentionWarning.group_mentions_disabled}}
<p class="warning-item">
{{this.groupsWithDisabledMentions}}
</p>
{{#if this.mentionWarning.groups_with_too_many_members}}
<p
class="warning-item"
>{{this.groupsWithTooManyMembers}}</p>
{{/if}}
{{/if}}
</div>
{{/if}}
</div>
{{#if this.mentionWarning.groups_with_too_many_members}}
<p class="warning-item">{{this.groupsWithTooManyMembers}}</p>
{{/if}}
{{/if}}
</div>
{{#if this.showThreadIndicator}}
<ChatMessageThreadIndicator @message={{@message}} />
{{/if}}
</div>
{{#if this.showThreadIndicator}}
<ChatMessageThreadIndicator @message={{@message}} />
{{/if}}
</div>
{{/if}}
{{/if}}
{{/if}}
</div>
</div>
{{/if}}

View File

@ -69,9 +69,43 @@ export default class ChatMessage extends Component {
return !this.args.message?.expanded;
}
get deletedMessageLabel() {
let count = 1;
const recursiveCount = (message) => {
const previousMessage = message.previousMessage;
if (previousMessage?.deletedAt) {
count++;
recursiveCount(previousMessage);
}
};
recursiveCount(this.args.message);
return I18n.t("chat.deleted", { count });
}
get shouldRender() {
return (
this.args.message.expanded ||
!this.args.message.deletedAt ||
(this.args.message.deletedAt && !this.args.message.nextMessage?.deletedAt)
);
}
@action
expand() {
const recursiveExpand = (message) => {
const previousMessage = message.previousMessage;
if (previousMessage?.deletedAt) {
previousMessage.expanded = true;
recursiveExpand(previousMessage);
}
};
this.args.message.expanded = true;
recursiveExpand(this.args.message);
}
@action

View File

@ -215,9 +215,9 @@ export default class ChatMessageInteractor {
bulkSelect(checked) {
const channel = this.message.channel;
const lastSelectedIndex = channel.findIndexOfMessage(
this.pane.lastSelectedMessage
this.pane.lastSelectedMessage.id
);
const newlySelectedIndex = channel.findIndexOfMessage(this.message);
const newlySelectedIndex = channel.findIndexOfMessage(this.message.id);
const sortedIndices = [lastSelectedIndex, newlySelectedIndex].sort(
(a, b) => a - b
);

View File

@ -12,6 +12,7 @@ export default class ChatMessagesManager {
}
clearMessages() {
this.messages.forEach((message) => (message.manager = null));
this.messages.clear();
this.canLoadMoreFuture = null;
@ -19,6 +20,10 @@ export default class ChatMessagesManager {
}
addMessages(messages = []) {
messages.forEach((message) => {
message.manager = this;
});
this.messages = this.messages
.concat(messages)
.uniqBy("id")
@ -40,4 +45,8 @@ export default class ChatMessagesManager {
(message) => message.staged && message.id === stagedMessageId
);
}
findIndexOfMessage(id) {
return this.messages.findIndex((m) => m.id === id);
}
}

View File

@ -68,14 +68,30 @@ export default class ChatChannel extends RestModel {
threadsManager = new ChatThreadsManager(getOwner(this));
messagesManager = new ChatMessagesManager(getOwner(this));
findIndexOfMessage(message) {
return this.messages.findIndex((m) => m.id === message.id);
findIndexOfMessage(id) {
return this.messagesManager.findIndexOfMessage(id);
}
findStagedMessage(id) {
return this.messagesManager.findStagedMessage(id);
}
findMessage(id) {
return this.messagesManager.findMessage(id);
}
addMessages(messages) {
this.messagesManager.addMessages(messages);
}
clearMessages() {
this.messagesManager.clearMessages();
}
removeMessage(message) {
this.messagesManager.removeMessage(message);
}
get messages() {
return this.messagesManager.messages;
}
@ -88,6 +104,10 @@ export default class ChatChannel extends RestModel {
return this.messagesManager.canLoadMoreFuture;
}
get canLoadMorePast() {
return this.messagesManager.canLoadMorePast;
}
get escapedTitle() {
return escapeExpression(this.title);
}
@ -186,10 +206,10 @@ export default class ChatChannel extends RestModel {
if (message.inReplyTo) {
if (!this.threadingEnabled) {
this.messagesManager.addMessages([message]);
this.addMessages([message]);
}
} else {
this.messagesManager.addMessages([message]);
this.addMessages([message]);
}
}

View File

@ -50,6 +50,8 @@ export default class ChatMessage {
@tracked message;
@tracked thread;
@tracked threadReplyCount;
@tracked manager = null;
@tracked _cooked;
constructor(channel, args = {}) {
@ -193,17 +195,17 @@ export default class ChatMessage {
@cached
get index() {
return this.channel.messages.indexOf(this);
return this.manager?.messages?.indexOf(this);
}
@cached
get previousMessage() {
return this.channel?.messages?.objectAt?.(this.index - 1);
return this.manager?.messages?.objectAt?.(this.index - 1);
}
@cached
get nextMessage() {
return this.channel?.messages?.objectAt?.(this.index + 1);
return this.manager?.messages?.objectAt?.(this.index + 1);
}
incrementVersion() {

View File

@ -7,6 +7,10 @@
color: var(--primary-low-mid);
padding: 0.25em;
.d-button-label {
text-align: left;
}
&:hover {
background: inherit;
color: inherit;

View File

@ -81,7 +81,10 @@ en:
collapse: "Collapse Chat Drawer"
expand: "Expand Chat Drawer"
confirm_flag: "Are you sure you want to flag %{username}'s message?"
deleted: "A message was deleted. [view]"
deleted:
one: "A message was deleted. [view]"
other: "%{count} messages were deleted. [view all]"
hidden: "A message was hidden. [view]"
delete: "Delete"
edited: "edited"

View File

@ -4,7 +4,7 @@ RSpec.describe "Deleted message", type: :system, js: true do
let(:chat_page) { PageObjects::Pages::Chat.new }
let(:channel_page) { PageObjects::Pages::ChatChannel.new }
fab!(:current_user) { Fabricate(:user) }
fab!(:current_user) { Fabricate(:admin) }
fab!(:channel_1) { Fabricate(:category_channel) }
before do
@ -21,7 +21,33 @@ RSpec.describe "Deleted message", type: :system, js: true do
last_message = find(".chat-message-container:last-child")
channel_page.delete_message(OpenStruct.new(id: last_message["data-id"]))
expect(page).to have_content(I18n.t("js.chat.deleted"))
expect(channel_page).to have_deleted_message(
OpenStruct.new(id: last_message["data-id"]),
count: 1,
)
end
end
context "when deleting multiple messages" do
fab!(:message_1) { Fabricate(:chat_message, chat_channel: channel_1) }
fab!(:message_2) { Fabricate(:chat_message, chat_channel: channel_1) }
fab!(:message_3) { Fabricate(:chat_message, chat_channel: channel_1) }
fab!(:message_4) { Fabricate(:chat_message, chat_channel: channel_1) }
fab!(:message_5) { Fabricate(:chat_message, chat_channel: channel_1) }
fab!(:message_6) { Fabricate(:chat_message, chat_channel: channel_1) }
it "groups them" do
chat_page.visit_channel(channel_1)
channel_page.delete_message(message_1)
channel_page.delete_message(message_3)
channel_page.delete_message(message_4)
channel_page.delete_message(message_6)
expect(channel_page).to have_deleted_message(message_1)
expect(channel_page).to have_deleted_message(message_4, count: 2)
expect(channel_page).to have_deleted_message(message_6)
expect(channel_page).to have_no_message(id: message_3.id)
end
end
@ -65,9 +91,9 @@ RSpec.describe "Deleted message", type: :system, js: true do
)
expect(channel_page).to have_no_message(id: message_1.id)
expect(channel_page).to have_no_message(id: message_2.id)
expect(channel_page).to have_deleted_message(message_2, count: 2)
expect(open_thread).to have_no_message(thread_id: thread.id, id: message_4.id)
expect(open_thread).to have_no_message(thread_id: thread.id, id: message_5.id)
expect(open_thread).to have_deleted_message(message_5, count: 2)
end
end
end

View File

@ -87,8 +87,7 @@ RSpec.describe "Move message to channel", type: :system, js: true do
chat.visit_channel(channel_1)
expect(page).to have_no_content(message_1.message)
expect(page).to have_content(I18n.t("js.chat.deleted"))
expect(channel).to have_deleted_message(message_1)
end
end
end

View File

@ -172,6 +172,13 @@ module PageObjects
check_message_presence(exists: false, text: text, id: id)
end
def has_deleted_message?(message, count: 1)
has_css?(
".chat-channel .chat-message-container[data-id=\"#{message.id}\"] .chat-message-deleted",
text: I18n.t("js.chat.deleted", count: count),
)
end
def check_message_presence(exists: true, text: nil, id: nil)
css_method = exists ? :has_css? : :has_no_css?
if text

View File

@ -77,6 +77,13 @@ module PageObjects
def message_by_id_selector(id)
".chat-thread .chat-messages-container .chat-message-container[data-id=\"#{id}\"]"
end
def has_deleted_message?(message, count: 1)
has_css?(
".chat-thread .chat-message-container[data-id=\"#{message.id}\"] .chat-message-deleted",
text: I18n.t("js.chat.deleted", count: count),
)
end
end
end
end