UX: enhances chat copy features (#23770)

- Allows to copy quotes from mobile
- Allows to copy text of a message from mobile
- Allows to select messages by clicking on it when selection has started

Note this commit is also now using toasts to show a confirmation of copy, and refactors system specs helpers concerning secondary actions.

<!-- NOTE: All pull requests should have tests (rspec in Ruby, qunit in JavaScript). If your code does not include test coverage, please include an explanation of why it was omitted. -->
This commit is contained in:
Joffrey JAFFEUX 2023-10-04 16:14:37 +02:00 committed by GitHub
parent 24feb20abc
commit 08df8fc1d1
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
20 changed files with 273 additions and 162 deletions

View File

@ -15,6 +15,9 @@
top: 5px;
right: 5px;
z-index: z("max");
display: flex;
flex-direction: column;
gap: 5px 0;
.mobile-view & {
left: 5px;

View File

@ -84,6 +84,7 @@ export default class ChatMessage extends Component {
{{on "mouseenter" this.onMouseEnter passive=true}}
{{on "mouseleave" this.onMouseLeave passive=true}}
{{on "mousemove" this.onMouseMove passive=true}}
{{on "click" this.toggleCheckIfPossible passive=true}}
{{ChatOnLongPress
this.onLongPressStart
this.onLongPressEnd
@ -193,6 +194,7 @@ export default class ChatMessage extends Component {
@service chatThreadPane;
@service chatChannelsManager;
@service router;
@service toasts;
@tracked isActive = false;
@ -278,10 +280,27 @@ export default class ChatMessage extends Component {
recursiveExpand(this.args.message);
}
@action
toggleCheckIfPossible(event) {
if (!this.pane.selectingMessages) {
return;
}
if (event.shiftKey) {
this.messageInteractor.bulkSelect(!this.args.message.selected);
return;
}
this.messageInteractor.select(!this.args.message.selected);
}
@action
toggleChecked(event) {
event.stopPropagation();
if (event.shiftKey) {
this.messageInteractor.bulkSelect(event.target.checked);
return;
}
this.messageInteractor.select(event.target.checked);

View File

@ -6,23 +6,62 @@ import { isTesting } from "discourse-common/config/environment";
import { popupAjaxError } from "discourse/lib/ajax-error";
import { inject as service } from "@ember/service";
import { bind } from "discourse-common/utils/decorators";
import { tracked } from "@glimmer/tracking";
import ChatModalMoveMessageToChannel from "discourse/plugins/chat/discourse/components/chat/modal/move-message-to-channel";
import DButton from "discourse/components/d-button";
import not from "truth-helpers/helpers/not";
import I18n from "I18n";
export default class ChatSelectionManager extends Component {
<template>
<div
class="chat-selection-management"
data-last-copy-successful={{this.lastCopySuccessful}}
>
<div class="chat-selection-management__buttons">
<DButton
@icon="quote-left"
@label="chat.selection.quote_selection"
@disabled={{not this.anyMessagesSelected}}
@action={{this.quoteMessages}}
id="chat-quote-btn"
/>
<DButton
@icon="copy"
@label="chat.selection.copy"
@disabled={{not this.anyMessagesSelected}}
@action={{this.copyMessages}}
id="chat-copy-btn"
/>
{{#if this.enableMove}}
<DButton
@icon="sign-out-alt"
@label="chat.selection.move_selection_to_channel"
@disabled={{not this.anyMessagesSelected}}
@action={{this.openMoveMessageModal}}
id="chat-move-to-channel-btn"
/>
{{/if}}
<DButton
@icon="times"
@label="chat.selection.cancel"
@action={{@pane.cancelSelecting}}
id="chat-cancel-selection-btn"
class="btn-secondary cancel-btn"
/>
</div>
</div>
</template>
@service("composer") topicComposer;
@service router;
@service modal;
@service site;
@service toasts;
@service("chat-api") api;
// NOTE: showCopySuccess is used to display the message which animates
// after a delay. The on-animation-end helper is not really usable in
// system specs because it fires straight away, so we use lastCopySuccessful
// with a data attr instead so it's not instantly mutated.
@tracked showCopySuccess = false;
@tracked lastCopySuccessful = false;
get enableMove() {
return this.args.enableMove ?? false;
}
@ -96,16 +135,17 @@ export default class ChatSelectionManager extends Component {
@action
async copyMessages() {
try {
this.lastCopySuccessful = false;
this.showCopySuccess = false;
if (!isTesting()) {
// clipboard API throws errors in tests
await clipboardCopyAsync(this.generateQuote);
}
this.showCopySuccess = true;
this.lastCopySuccessful = true;
this.toasts.success({
duration: 3000,
data: {
message: I18n.t("chat.quote.copy_success"),
},
});
}
} catch (error) {
popupAjaxError(error);
}

View File

@ -1,51 +0,0 @@
<div
class="chat-selection-management"
data-last-copy-successful={{this.lastCopySuccessful}}
>
<div class="chat-selection-management__buttons">
<DButton
@icon="quote-left"
@label="chat.selection.quote_selection"
@disabled={{not this.anyMessagesSelected}}
@action={{this.quoteMessages}}
id="chat-quote-btn"
/>
{{#if this.site.desktopView}}
<DButton
@icon="copy"
@label="chat.selection.copy"
@disabled={{not this.anyMessagesSelected}}
@action={{this.copyMessages}}
id="chat-copy-btn"
/>
{{/if}}
{{#if this.enableMove}}
<DButton
@icon="sign-out-alt"
@label="chat.selection.move_selection_to_channel"
@disabled={{not this.anyMessagesSelected}}
@action={{this.openMoveMessageModal}}
id="chat-move-to-channel-btn"
/>
{{/if}}
<DButton
@icon="times"
@label="chat.selection.cancel"
@action={{@pane.cancelSelecting}}
id="chat-cancel-selection-btn"
class="btn-secondary cancel-btn"
/>
</div>
{{#if this.showCopySuccess}}
<span
class="chat-selection-management__copy-success"
{{chat/on-animation-end (fn (mut this.showCopySuccess) false)}}
>
{{i18n "chat.quote.copy_success"}}
</span>
{{/if}}
</div>

View File

@ -45,6 +45,7 @@ export default class ChatMessageInteractor {
@service router;
@service modal;
@service capabilities;
@service toasts;
@tracked message = null;
@tracked context = null;
@ -166,6 +167,14 @@ export default class ChatMessageInteractor {
icon: "link",
});
if (this.site.mobileView) {
buttons.push({
id: "copyText",
name: I18n.t("chat.copy_text"),
icon: "clipboard",
});
}
if (this.canEdit) {
buttons.push({
id: "edit",
@ -237,6 +246,14 @@ export default class ChatMessageInteractor {
}
}
copyText() {
clipboardCopy(this.message.message);
this.toasts.success({
duration: 3000,
data: { message: I18n.t("chat.text_copied") },
});
}
copyLink() {
const { protocol, host } = window.location;
const channelId = this.message.channel.id;
@ -251,6 +268,10 @@ export default class ChatMessageInteractor {
url = url.indexOf("/") === 0 ? protocol + "//" + host + url : url;
clipboardCopy(url);
this.toasts.success({
duration: 3000,
data: { message: I18n.t("chat.link_copied") },
});
}
@action

View File

@ -29,6 +29,8 @@ en:
chat_users_count: "Chat users"
chat:
text_copied: Text copied to clipboard
link_copied: Link copied to clipboard
deleted_chat_username: deleted
dates:
time_tiny: "h:mm"
@ -241,6 +243,7 @@ en:
reply: "Reply"
edit: "Edit"
copy_link: "Copy link"
copy_text: "Copy text"
rebake_message: "Rebuild HTML"
retry_staged_message:
title: "Network error"

View File

@ -18,6 +18,7 @@ register_asset "stylesheets/mobile/index.scss", :mobile
register_svg_icon "comments"
register_svg_icon "comment-slash"
register_svg_icon "lock"
register_svg_icon "clipboard"
register_svg_icon "file-audio"
register_svg_icon "file-video"
register_svg_icon "file-image"

View File

@ -6,8 +6,8 @@ RSpec.describe "Chat message - channel", type: :system do
fab!(:message_1) { Fabricate(:chat_message, chat_channel: channel_1) }
let(:cdp) { PageObjects::CDP.new }
let(:chat) { PageObjects::Pages::Chat.new }
let(:channel) { PageObjects::Pages::ChatChannel.new }
let(:chat_page) { PageObjects::Pages::Chat.new }
let(:channel_page) { PageObjects::Pages::ChatChannel.new }
before do
chat_system_bootstrap
@ -17,9 +17,9 @@ RSpec.describe "Chat message - channel", type: :system do
context "when hovering a message" do
it "adds an active class" do
chat.visit_channel(channel_1)
chat_page.visit_channel(channel_1)
channel.hover_message(message_1)
channel_page.hover_message(message_1)
expect(page).to have_css(
".chat-channel[data-id='#{channel_1.id}'] .chat-message-container[data-id='#{message_1.id}'].-active",
@ -27,15 +27,39 @@ RSpec.describe "Chat message - channel", type: :system do
end
end
context "when copying text of a message" do
before { cdp.allow_clipboard }
it "[mobile] copies the text of a single message", mobile: true do
chat_page.visit_channel(channel_1)
channel_page.click_composer # ensures we don't block on vibrate due to no action
channel_page.messages.copy_text(message_1)
expect(cdp.read_clipboard.chomp).to eq(message_1.message)
expect(PageObjects::Components::Toasts.new).to have_success(I18n.t("js.chat.text_copied"))
end
end
context "when copying link to a message" do
before { cdp.allow_clipboard }
it "copies the link to the message" do
chat.visit_channel(channel_1)
chat_page.visit_channel(channel_1)
channel.copy_link(message_1)
channel_page.messages.copy_link(message_1)
expect(cdp.read_clipboard).to include("/chat/c/-/#{channel_1.id}/#{message_1.id}")
expect(PageObjects::Components::Toasts.new).to have_success(I18n.t("js.chat.link_copied"))
end
it "[mobile] copies the link to the message", mobile: true do
chat_page.visit_channel(channel_1)
channel_page.messages.copy_link(message_1)
expect(cdp.read_clipboard).to include("/chat/c/-/#{channel_1.id}/#{message_1.id}")
expect(PageObjects::Components::Toasts.new).to have_success(I18n.t("js.chat.link_copied"))
end
context "when the message is part of a thread" do
@ -49,14 +73,26 @@ RSpec.describe "Chat message - channel", type: :system do
)
end
it "copies the link to the message and not to the thread" do
chat.visit_channel(channel_1)
it "copies the link to the message" do
chat_page.visit_channel(channel_1)
channel.copy_link(thread_1.original_message)
channel_page.messages.copy_link(thread_1.original_message)
expect(cdp.read_clipboard).to include(
"/chat/c/-/#{channel_1.id}/#{thread_1.original_message.id}",
)
expect(PageObjects::Components::Toasts.new).to have_success(I18n.t("js.chat.link_copied"))
end
it "[mobile] copies the link to the message", mobile: true do
chat_page.visit_channel(channel_1)
channel_page.messages.copy_link(thread_1.original_message)
expect(cdp.read_clipboard).to include(
"/chat/c/-/#{channel_1.id}/#{thread_1.original_message.id}",
)
expect(PageObjects::Components::Toasts.new).to have_success(I18n.t("js.chat.link_copied"))
end
end
end

View File

@ -8,6 +8,7 @@ RSpec.describe "Chat message - thread", type: :system do
Fabricate(:chat_message, in_reply_to: message_1, use_service: true)
end
let(:cdp) { PageObjects::CDP.new }
let(:chat_page) { PageObjects::Pages::Chat.new }
let(:thread_page) { PageObjects::Pages::ChatThread.new }
@ -29,6 +30,20 @@ RSpec.describe "Chat message - thread", type: :system do
end
end
context "when copying text of a message" do
before { cdp.allow_clipboard }
it "[mobile] copies the text of a single message", mobile: true do
chat_page.visit_thread(thread_message_1.thread)
thread_page.click_composer # ensures we don't block on vibrate due to no action
thread_page.messages.copy_text(thread_message_1)
expect(cdp.read_clipboard.chomp).to eq(thread_message_1.message)
expect(PageObjects::Components::Toasts.new).to have_success(I18n.t("js.chat.text_copied"))
end
end
context "when copying link to a message" do
let(:cdp) { PageObjects::CDP.new }
@ -37,11 +52,23 @@ RSpec.describe "Chat message - thread", type: :system do
it "copies the link to the thread" do
chat_page.visit_thread(thread_message_1.thread)
thread_page.copy_link(thread_message_1)
thread_page.messages.copy_link(thread_message_1)
expect(cdp.read_clipboard).to include(
"/chat/c/-/#{channel_1.id}/t/#{thread_message_1.thread.id}/#{thread_message_1.id}",
)
expect(PageObjects::Components::Toasts.new).to have_success(I18n.t("js.chat.link_copied"))
end
it "[mobile] copies the link to the thread", mobile: true do
chat_page.visit_thread(thread_message_1.thread)
thread_page.messages.copy_link(thread_message_1)
expect(cdp.read_clipboard).to include(
"/chat/c/-/#{channel_1.id}/t/#{thread_message_1.thread.id}/#{thread_message_1.id}",
)
expect(PageObjects::Components::Toasts.new).to have_success(I18n.t("js.chat.link_copied"))
end
end
end

View File

@ -17,12 +17,12 @@ RSpec.describe "Deleted message", type: :system do
context "when deleting a message" do
it "shows as deleted" do
chat_page.visit_channel(channel_1)
channel_page.send_message("aaaaaaaaaaaaaaaaaaaa")
channel_page.send_message
expect(page).to have_css(".-persisted")
last_message = find(".chat-message-container:last-child")
channel_page.delete_message(OpenStruct.new(id: last_message["data-id"]))
channel_page.messages.delete(OpenStruct.new(id: last_message["data-id"]))
expect(channel_page.messages).to have_deleted_message(
OpenStruct.new(id: last_message["data-id"]),
@ -39,7 +39,7 @@ RSpec.describe "Deleted message", type: :system do
.find_by(user: current_user)
.update!(last_read_message_id: message.id)
chat_page.visit_channel(channel_1)
channel_page.delete_message(message)
channel_page.messages.delete(message)
expect(channel_page.messages).to have_deleted_message(message, count: 1)
sidebar_component.click_link(channel_2.name)
expect(channel_page).to have_no_loading_skeleton
@ -67,7 +67,7 @@ RSpec.describe "Deleted message", type: :system do
using_session(:tab_2) do |session|
sign_in(other_user)
chat_page.visit_channel(channel_1)
channel_page.delete_message(message)
channel_page.messages.delete(message)
session.quit
end
@ -88,10 +88,10 @@ RSpec.describe "Deleted message", type: :system do
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)
channel_page.messages.delete(message_1)
channel_page.messages.delete(message_3)
channel_page.messages.delete(message_4)
channel_page.messages.delete(message_6)
expect(channel_page.messages).to have_deleted_messages(message_1, message_6)
expect(channel_page.messages).to have_deleted_message(message_4, count: 2)
@ -147,15 +147,17 @@ RSpec.describe "Deleted message", type: :system do
expect(open_thread.messages).to have_message(thread_id: thread_1.id, id: message_4.id)
expect(open_thread.messages).to have_message(thread_id: thread_1.id, id: message_5.id)
open_thread.send_message
Chat::Publisher.publish_bulk_delete!(
channel_1,
[message_1.id, message_2.id, message_4.id, message_5.id].flatten,
)
expect(channel_page.messages).to have_no_message(id: message_1.id)
expect(channel_page.messages).to have_deleted_message(message_2, count: 2)
expect(open_thread.messages).to have_no_message(thread_id: thread_1.id, id: message_4.id)
expect(open_thread.messages).to have_deleted_message(message_5, count: 2)
expect(open_thread.messages).to have_no_message(thread_id: thread_1.id, id: message_4.id)
expect(channel_page.messages).to have_no_message(id: message_1.id)
end
end
end

View File

@ -19,7 +19,7 @@ RSpec.describe "Flag message", type: :system do
it "allows to flag a message" do
chat.visit_channel(category_channel_1)
channel.flag_message(message_1)
channel.messages.flag(message_1)
expect(page).to have_css(".flag-modal")

View File

@ -96,44 +96,8 @@ module PageObjects
find(".more-buttons").click
end
def flag_message(message)
hover_message(message)
click_more_button
find("[data-value='flag']").click
end
def copy_link(message)
hover_message(message)
click_more_button
find("[data-value='copyLink']").click
end
def flag_message(message)
hover_message(message)
click_more_button
find("[data-value='flag']").click
end
def delete_message(message)
hover_message(message)
click_more_button
find("[data-value='delete']").click
end
def restore_message(message)
hover_message(message)
click_more_button
find("[data-value='restore']").click
end
def open_edit_message(message)
hover_message(message)
click_more_button
find("[data-value='edit']").click
end
def edit_message(message, text = nil)
open_edit_message(message)
messages.edit(message)
send_message(message.message + text) if text
end

View File

@ -118,22 +118,6 @@ module PageObjects
message_by_id(message.id).find(".chat-message-expand").click
end
def copy_link(message)
expand_message_actions(message)
find("[data-value='copyLink']").click
end
def delete_message(message)
expand_message_actions(message)
find("[data-value='delete']").click
end
def restore_message(message)
expand_deleted_message(message)
expand_message_actions(message)
find("[data-value='restore']").click
end
def expand_message_actions(message)
hover_message(message)
click_more_button
@ -155,14 +139,8 @@ module PageObjects
".chat-thread .chat-messages-container .chat-message-container[data-id=\"#{id}\"]"
end
def open_edit_message(message)
hover_message(message)
click_more_button
find("[data-value='edit']").click
end
def edit_message(message, text = nil)
open_edit_message(message)
messages.edit(message)
send_message(message.message + text) if text
end
end

View File

@ -30,6 +30,16 @@ module PageObjects
component.find(".chat-message-expand").click
end
def secondary_action(action)
if page.has_css?("html.mobile-view", wait: 0)
component.click(delay: 0.4)
page.find(".chat-message-actions [data-id=\"#{action}\"]").click
else
open_more_menu
page.find("[data-value='#{action}']").click
end
end
def select(shift: false)
if component[:class].include?("-selectable")
message_selector = component.find(".chat-message-selector")
@ -43,7 +53,7 @@ module PageObjects
end
if page.has_css?("html.mobile-view", wait: 0)
component.click(delay: 0.6)
component.click(delay: 0.4)
page.find(".chat-message-actions [data-id=\"select\"]").click
else
hover

View File

@ -16,6 +16,35 @@ module PageObjects
page.find(context)
end
def flag(message)
find(message).secondary_action("flag")
end
def copy_link(message)
find(message).secondary_action("copyLink")
end
def copy_text(message)
find(message).secondary_action("copyText")
end
def flag(message)
find(message).secondary_action("flag")
end
def delete(message)
find(message).secondary_action("delete")
end
def restore(message)
find(message).expand
find(message).secondary_action("restore")
end
def edit(message)
find(message).secondary_action("edit")
end
def has_action?(action, **args)
message = find(args)
message.open_more_menu

View File

@ -7,7 +7,7 @@ RSpec.describe "Restore message", type: :system do
fab!(:channel_1) { Fabricate(:category_channel) }
let(:chat_page) { PageObjects::Pages::Chat.new }
let(:chat_channel_page) { PageObjects::Pages::ChatChannel.new }
let(:channel_page) { PageObjects::Pages::ChatChannel.new }
before do
chat_system_bootstrap
@ -23,10 +23,10 @@ RSpec.describe "Restore message", type: :system do
sign_in(regular_user)
chat_page.visit_channel(channel_1)
chat_channel_page.delete_message(message_1)
channel_page.messages.delete(message_1)
expect(chat_channel_page.messages).to have_deleted_message(message_1, count: 1)
expect(chat_channel_page.messages).to have_action("restore", id: message_1.id)
expect(channel_page.messages).to have_deleted_message(message_1, count: 1)
expect(channel_page.messages).to have_action("restore", id: message_1.id)
end
it "can't be restored by another user" do
@ -38,12 +38,12 @@ RSpec.describe "Restore message", type: :system do
using_session(:regular_user) do |session|
sign_in(regular_user)
chat_page.visit_channel(channel_1)
chat_channel_page.delete_message(message_1)
channel_page.messages.delete(message_1)
session.quit
end
using_session(:another_user) do |session|
expect(chat_channel_page.messages).to have_no_message(id: message_1.id)
expect(channel_page.messages).to have_no_message(id: message_1.id)
session.quit
end
end
@ -61,14 +61,14 @@ RSpec.describe "Restore message", type: :system do
using_session(:admin_user) do |session|
sign_in(admin_user)
chat_page.visit_channel(channel_1)
chat_channel_page.delete_message(message_1)
channel_page.messages.delete(message_1)
session.quit
end
using_session(:regular_user) do |session|
expect(chat_channel_page.messages).to have_deleted_message(message_1, count: 1)
chat_channel_page.messages.expand(id: message_1.id)
expect(chat_channel_page.messages).to have_no_action("restore", id: message_1.id)
expect(channel_page.messages).to have_deleted_message(message_1, count: 1)
channel_page.messages.expand(id: message_1.id)
expect(channel_page.messages).to have_no_action("restore", id: message_1.id)
session.quit
end
end

View File

@ -168,7 +168,7 @@ describe "Thread list in side panel | full page", type: :system do
sign_in(other_user)
chat_page.visit_thread(thread_1)
expect(side_panel_page).to have_open_thread(thread_1)
thread_page.delete_message(thread_1.original_message)
thread_page.messages.delete(thread_1.original_message)
session.quit
end
@ -192,7 +192,7 @@ describe "Thread list in side panel | full page", type: :system do
channel_page.expand_deleted_message(thread_1.original_message)
channel_page.message_thread_indicator(thread_1.original_message).click
expect(side_panel_page).to have_open_thread(thread_1)
thread_page.restore_message(thread_1.original_message)
thread_page.messages.restore(thread_1.original_message)
session.quit
end

View File

@ -20,7 +20,9 @@ RSpec.describe "Quoting chat message transcripts", type: :system do
messages = Array.wrap(messages)
messages.each { |message| channel_page.messages.select(message) }
channel_page.selection_management.copy
expect(page).to have_css(".chat-selection-management[data-last-copy-successful]")
expect(PageObjects::Components::Toasts.new).to have_success(
I18n.t("js.chat.quote.copy_success"),
)
clip_text = cdp.read_clipboard
expect(clip_text.chomp).to eq(generate_transcript(messages, current_user))
clip_text

View File

@ -112,7 +112,7 @@ describe "Uploading files in chat messages", type: :system do
it "allows deleting uploads" do
chat.visit_channel(channel_1)
channel_page.open_edit_message(message_2)
channel_page.messages.edit(message_2)
find(".chat-composer-upload").hover
find(".chat-composer-upload__remove-btn").click
channel_page.click_send_message
@ -122,7 +122,7 @@ describe "Uploading files in chat messages", type: :system do
it "allows adding more uploads" do
chat.visit_channel(channel_1)
channel_page.open_edit_message(message_2)
channel_page.messages.edit(message_2)
file_path = file_from_fixtures("logo.png", "images").path
attach_file(file_path) do

View File

@ -0,0 +1,27 @@
# frozen_string_literal: true
module PageObjects
module Components
class Toasts < PageObjects::Components::Base
def has_default?(message)
has_css?(".fk-d-default-toast", text: message)
end
def has_success?(message)
has_css?(".fk-d-default-toast.-success", text: message)
end
def has_warning?(message)
has_css?(".fk-d-default-toast.-warning", text: message)
end
def has_info?(message)
has_css?(".fk-d-default-toast.-info", text: message)
end
def has_error?(message)
has_css?(".fk-d-default-toast.-error", text: message)
end
end
end
end