From a39ff830e83c21d4d3312912890e583db5959010 Mon Sep 17 00:00:00 2001 From: Joffrey JAFFEUX Date: Mon, 9 Oct 2023 21:12:50 +0200 Subject: [PATCH] UX: makes avatar non interactive in thread participants list (#23847) It was slightly surprising to have a user card show when click on a thread item list. More over this commit does: - moves chat/user-avatar to chat-user-avatar and converts it to gjs - moves chat/thread/participants to chat-thread-participants - rewrite the `toggleCheckIfPossible` modifier to only be applied when selecting messages, it prevents the click event to collide with the click of avatars in regular messages --- .../components/chat-channel-title.hbs | 2 +- .../chat-composer-message-details.hbs | 2 +- .../chat-message-actions-mobile.gjs | 2 +- .../chat-message-in-reply-to-indicator.hbs | 2 +- .../chat-message-thread-indicator.hbs | 4 +- .../discourse/components/chat-message.gjs | 45 +++++++++----- ...pants.gjs => chat-thread-participants.gjs} | 3 +- .../discourse/components/chat-user-avatar.gjs | 58 +++++++++++++++++++ .../discourse/components/chat-user-info.gjs | 2 +- .../chat/message-creator/user-row.hbs | 2 +- .../chat/message-creator/user-selection.hbs | 2 +- .../components/chat/message/avatar.hbs | 2 +- .../components/chat/thread-list/item.hbs | 5 +- .../discourse/components/chat/user-avatar.hbs | 9 --- .../discourse/components/chat/user-avatar.js | 26 --------- .../common/chat-thread-list-item.scss | 2 + .../chat/spec/fabricators/chat_fabricator.rb | 5 +- .../chat/components/thread_indicator.rb | 2 +- .../chat-thread-participants-test.js | 19 +++--- .../components/chat-user-avatar-test.js | 22 +++++-- 20 files changed, 134 insertions(+), 82 deletions(-) rename plugins/chat/assets/javascripts/discourse/components/{chat/thread/participants.gjs => chat-thread-participants.gjs} (96%) create mode 100644 plugins/chat/assets/javascripts/discourse/components/chat-user-avatar.gjs delete mode 100644 plugins/chat/assets/javascripts/discourse/components/chat/user-avatar.hbs delete mode 100644 plugins/chat/assets/javascripts/discourse/components/chat/user-avatar.js diff --git a/plugins/chat/assets/javascripts/discourse/components/chat-channel-title.hbs b/plugins/chat/assets/javascripts/discourse/components/chat-channel-title.hbs index 1e8958d0bf1..712abf29dbd 100644 --- a/plugins/chat/assets/javascripts/discourse/components/chat-channel-title.hbs +++ b/plugins/chat/assets/javascripts/discourse/components/chat-channel-title.hbs @@ -7,7 +7,7 @@ {{@channel.chatable.users.length}} {{else}} - + {{/if}} {{/if}} diff --git a/plugins/chat/assets/javascripts/discourse/components/chat-composer-message-details.hbs b/plugins/chat/assets/javascripts/discourse/components/chat-composer-message-details.hbs index eac795d3fc8..3415ca32d0f 100644 --- a/plugins/chat/assets/javascripts/discourse/components/chat-composer-message-details.hbs +++ b/plugins/chat/assets/javascripts/discourse/components/chat-composer-message-details.hbs @@ -5,7 +5,7 @@ >
{{d-icon (if @message.editing "pencil-alt" "reply")}} - + {{@message.user.username}} {{replace-emoji (html-safe @message.excerpt)}} diff --git a/plugins/chat/assets/javascripts/discourse/components/chat-message-actions-mobile.gjs b/plugins/chat/assets/javascripts/discourse/components/chat-message-actions-mobile.gjs index d5c572cd350..52c34c1f54c 100644 --- a/plugins/chat/assets/javascripts/discourse/components/chat-message-actions-mobile.gjs +++ b/plugins/chat/assets/javascripts/discourse/components/chat-message-actions-mobile.gjs @@ -10,7 +10,7 @@ import didInsert from "@ember/render-modifiers/modifiers/did-insert"; import concatClass from "discourse/helpers/concat-class"; import DButton from "discourse/components/d-button"; import { on } from "@ember/modifier"; -import ChatUserAvatar from "discourse/plugins/chat/discourse/components/chat/user-avatar"; +import ChatUserAvatar from "discourse/plugins/chat/discourse/components/chat-user-avatar"; import ChatMessageReaction from "discourse/plugins/chat/discourse/components/chat-message-reaction"; import { fn } from "@ember/helper"; import or from "truth-helpers/helpers/or"; diff --git a/plugins/chat/assets/javascripts/discourse/components/chat-message-in-reply-to-indicator.hbs b/plugins/chat/assets/javascripts/discourse/components/chat-message-in-reply-to-indicator.hbs index 716525f3fd1..d414a05390d 100644 --- a/plugins/chat/assets/javascripts/discourse/components/chat-message-in-reply-to-indicator.hbs +++ b/plugins/chat/assets/javascripts/discourse/components/chat-message-in-reply-to-indicator.hbs @@ -9,7 +9,7 @@ {{#if @message.inReplyTo.chatWebhookEvent.emoji}} {{else}} - + {{/if}} diff --git a/plugins/chat/assets/javascripts/discourse/components/chat-message-thread-indicator.hbs b/plugins/chat/assets/javascripts/discourse/components/chat-message-thread-indicator.hbs index 2e16cc3d229..77508ae58d0 100644 --- a/plugins/chat/assets/javascripts/discourse/components/chat-message-thread-indicator.hbs +++ b/plugins/chat/assets/javascripts/discourse/components/chat-message-thread-indicator.hbs @@ -10,7 +10,7 @@ >
- @@ -27,7 +27,7 @@
{{i18n "chat.thread.replies" count=@message.thread.preview.replyCount}}
- +
{{replace-emoji (html-safe @message.thread.preview.lastReplyExcerpt)}}
diff --git a/plugins/chat/assets/javascripts/discourse/components/chat-message.gjs b/plugins/chat/assets/javascripts/discourse/components/chat-message.gjs index 5793692218d..f8ef8cfb2b1 100644 --- a/plugins/chat/assets/javascripts/discourse/components/chat-message.gjs +++ b/plugins/chat/assets/javascripts/discourse/components/chat-message.gjs @@ -32,6 +32,7 @@ import didInsert from "@ember/render-modifiers/modifiers/did-insert"; import didUpdate from "@ember/render-modifiers/modifiers/did-update"; import willDestroy from "@ember/render-modifiers/modifiers/will-destroy"; import ChatOnLongPress from "discourse/plugins/chat/discourse/modifiers/chat/on-long-press"; +import { modifier } from "ember-modifier"; let _chatMessageDecorators = []; @@ -84,7 +85,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}} + {{this.toggleCheckIfPossible}} {{ChatOnLongPress this.onLongPressStart this.onLongPressEnd @@ -200,6 +201,34 @@ export default class ChatMessage extends Component { @optionalService adminTools; + toggleCheckIfPossible = modifier((element) => { + let addedListener = false; + + const handler = () => { + if (!this.pane.selectingMessages) { + return; + } + + if (event.shiftKey) { + this.messageInteractor.bulkSelect(!this.args.message.selected); + return; + } + + this.messageInteractor.select(!this.args.message.selected); + }; + + if (this.pane.selectingMessages) { + element.addEventListener("click", handler, { passive: true }); + addedListener = true; + } + + return () => { + if (addedListener) { + element.removeEventListener("click", handler); + } + }; + }); + constructor() { super(...arguments); this.initMentionedUsers(); @@ -280,20 +309,6 @@ 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(); diff --git a/plugins/chat/assets/javascripts/discourse/components/chat/thread/participants.gjs b/plugins/chat/assets/javascripts/discourse/components/chat-thread-participants.gjs similarity index 96% rename from plugins/chat/assets/javascripts/discourse/components/chat/thread/participants.gjs rename to plugins/chat/assets/javascripts/discourse/components/chat-thread-participants.gjs index 441cba1ed42..ac49c1bd5e9 100644 --- a/plugins/chat/assets/javascripts/discourse/components/chat/thread/participants.gjs +++ b/plugins/chat/assets/javascripts/discourse/components/chat-thread-participants.gjs @@ -1,5 +1,5 @@ import Component from "@glimmer/component"; -import ChatUserAvatar from "discourse/plugins/chat/discourse/components/chat/user-avatar"; +import ChatUserAvatar from "discourse/plugins/chat/discourse/components/chat-user-avatar"; import I18n from "I18n"; export default class ChatThreadParticipants extends Component { @@ -12,6 +12,7 @@ export default class ChatThreadParticipants extends Component { @user={{user}} @avatarSize="tiny" @showPresence={{false}} + @interactive={{false}} /> {{/each}}
diff --git a/plugins/chat/assets/javascripts/discourse/components/chat-user-avatar.gjs b/plugins/chat/assets/javascripts/discourse/components/chat-user-avatar.gjs new file mode 100644 index 00000000000..d1907d23b42 --- /dev/null +++ b/plugins/chat/assets/javascripts/discourse/components/chat-user-avatar.gjs @@ -0,0 +1,58 @@ +import Component from "@glimmer/component"; +import { inject as service } from "@ember/service"; +import { renderAvatar } from "discourse/helpers/user-avatar"; +import { htmlSafe } from "@ember/template"; +import concatClass from "discourse/helpers/concat-class"; + +export default class ChatUserAvatar extends Component { + + + @service chat; + + get avatar() { + return htmlSafe( + renderAvatar(this.args.user, { imageSize: this.avatarSize }) + ); + } + + get interactive() { + return this.args.interactive ?? true; + } + + get avatarSize() { + return this.args.avatarSize || "tiny"; + } + + get showPresence() { + return this.args.showPresence ?? true; + } + + get isOnline() { + const users = (this.args.chat || this.chat).presenceChannel?.users; + + return ( + this.showPresence && + !!users?.find( + ({ id, username }) => + this.args.user?.id === id || this.args.user?.username === username + ) + ); + } +} diff --git a/plugins/chat/assets/javascripts/discourse/components/chat-user-info.gjs b/plugins/chat/assets/javascripts/discourse/components/chat-user-info.gjs index d4ae7ebc428..b7640700439 100644 --- a/plugins/chat/assets/javascripts/discourse/components/chat-user-info.gjs +++ b/plugins/chat/assets/javascripts/discourse/components/chat-user-info.gjs @@ -1,6 +1,6 @@ import Component from "@glimmer/component"; import { userPath } from "discourse/lib/url"; -import ChatUserAvatar from "discourse/plugins/chat/discourse/components/chat/user-avatar"; +import ChatUserAvatar from "discourse/plugins/chat/discourse/components/chat-user-avatar"; import ChatUserDisplayName from "discourse/plugins/chat/discourse/components/chat-user-display-name"; export default class ChatUserInfo extends Component { diff --git a/plugins/chat/assets/javascripts/discourse/components/chat/message-creator/user-row.hbs b/plugins/chat/assets/javascripts/discourse/components/chat/message-creator/user-row.hbs index 91785854dba..640fd839332 100644 --- a/plugins/chat/assets/javascripts/discourse/components/chat/message-creator/user-row.hbs +++ b/plugins/chat/assets/javascripts/discourse/components/chat/message-creator/user-row.hbs @@ -1,4 +1,4 @@ - + {{#if (gt @content.tracking.unreadCount 0)}} diff --git a/plugins/chat/assets/javascripts/discourse/components/chat/message-creator/user-selection.hbs b/plugins/chat/assets/javascripts/discourse/components/chat/message-creator/user-selection.hbs index a5bf850b37e..ef9cca035f6 100644 --- a/plugins/chat/assets/javascripts/discourse/components/chat/message-creator/user-selection.hbs +++ b/plugins/chat/assets/javascripts/discourse/components/chat/message-creator/user-selection.hbs @@ -1,4 +1,4 @@ - + {{@selection.model.username}} diff --git a/plugins/chat/assets/javascripts/discourse/components/chat/message/avatar.hbs b/plugins/chat/assets/javascripts/discourse/components/chat/message/avatar.hbs index c393efa893d..1d2e9676fbf 100644 --- a/plugins/chat/assets/javascripts/discourse/components/chat/message/avatar.hbs +++ b/plugins/chat/assets/javascripts/discourse/components/chat/message/avatar.hbs @@ -2,6 +2,6 @@ {{#if @message.chatWebhookEvent.emoji}} {{else}} - + {{/if}}
\ No newline at end of file diff --git a/plugins/chat/assets/javascripts/discourse/components/chat/thread-list/item.hbs b/plugins/chat/assets/javascripts/discourse/components/chat/thread-list/item.hbs index 6424753a6fe..07c18da2323 100644 --- a/plugins/chat/assets/javascripts/discourse/components/chat/thread-list/item.hbs +++ b/plugins/chat/assets/javascripts/discourse/components/chat/thread-list/item.hbs @@ -29,11 +29,12 @@
- - -
- {{avatar @user imageSize=this.avatarSize}} -
-
\ No newline at end of file diff --git a/plugins/chat/assets/javascripts/discourse/components/chat/user-avatar.js b/plugins/chat/assets/javascripts/discourse/components/chat/user-avatar.js deleted file mode 100644 index b4a616d78c9..00000000000 --- a/plugins/chat/assets/javascripts/discourse/components/chat/user-avatar.js +++ /dev/null @@ -1,26 +0,0 @@ -import Component from "@glimmer/component"; -import { inject as service } from "@ember/service"; - -export default class ChatUserAvatar extends Component { - @service chat; - - get avatarSize() { - return this.args.avatarSize || "tiny"; - } - - get showPresence() { - return this.args.showPresence ?? true; - } - - get isOnline() { - const users = (this.args.chat || this.chat).presenceChannel?.users; - - return ( - this.showPresence && - !!users?.find( - ({ id, username }) => - this.args.user?.id === id || this.args.user?.username === username - ) - ); - } -} diff --git a/plugins/chat/assets/stylesheets/common/chat-thread-list-item.scss b/plugins/chat/assets/stylesheets/common/chat-thread-list-item.scss index d282a171500..68604427481 100644 --- a/plugins/chat/assets/stylesheets/common/chat-thread-list-item.scss +++ b/plugins/chat/assets/stylesheets/common/chat-thread-list-item.scss @@ -75,6 +75,8 @@ align-items: center; gap: 0.5rem; .chat-user-avatar { + cursor: pointer; + &__container { padding: 0; } diff --git a/plugins/chat/spec/fabricators/chat_fabricator.rb b/plugins/chat/spec/fabricators/chat_fabricator.rb index 59b7c3bac86..14b1d9ea0ae 100644 --- a/plugins/chat/spec/fabricators/chat_fabricator.rb +++ b/plugins/chat/spec/fabricators/chat_fabricator.rb @@ -63,7 +63,7 @@ end Fabricator(:chat_message_without_service, class_name: "Chat::Message") do user chat_channel - message { Faker::Lorem.paragraph_by_chars(number: 500).gsub("...", "…") } + message { Faker::Lorem.paragraph_by_chars(number: 500).gsub("...", "…").gsub("..", "…") } after_build { |message, attrs| message.cook } after_create { |message, attrs| message.create_mentions } @@ -90,7 +90,8 @@ Fabricator(:chat_message_with_service, class_name: "Chat::CreateMessage") do chat_channel_id: channel.id, guardian: user.guardian, message: - transients[:message] || Faker::Lorem.paragraph_by_chars(number: 500).gsub("...", "…"), + transients[:message] || + Faker::Lorem.paragraph_by_chars(number: 500).gsub("...", "…").gsub("..", "…"), thread_id: transients[:thread]&.id, in_reply_to_id: transients[:in_reply_to]&.id, upload_ids: transients[:upload_ids], diff --git a/plugins/chat/spec/system/page_objects/chat/components/thread_indicator.rb b/plugins/chat/spec/system/page_objects/chat/components/thread_indicator.rb index 7eab3824b68..5f6a69c2308 100644 --- a/plugins/chat/spec/system/page_objects/chat/components/thread_indicator.rb +++ b/plugins/chat/spec/system/page_objects/chat/components/thread_indicator.rb @@ -33,7 +33,7 @@ module PageObjects def has_participant?(user) find(@context).has_css?( - ".chat-thread-participants__avatar-group .chat-user-avatar .chat-user-avatar__container[data-user-card=\"#{user.username}\"] img", + ".chat-thread-participants__avatar-group .chat-user-avatar[data-username=\"#{user.username}\"] img", ) end diff --git a/plugins/chat/test/javascripts/components/chat-thread-participants-test.js b/plugins/chat/test/javascripts/components/chat-thread-participants-test.js index a3632b1cb71..c83d512e372 100644 --- a/plugins/chat/test/javascripts/components/chat-thread-participants-test.js +++ b/plugins/chat/test/javascripts/components/chat-thread-participants-test.js @@ -5,18 +5,18 @@ import { render } from "@ember/test-helpers"; import { module, test } from "qunit"; module( - "Discourse Chat | Component | ", + "Discourse Chat | Component | ", function (hooks) { setupRenderingTest(hooks); test("no participants", async function (assert) { this.thread = fabricators.thread(); - await render(hbs``); + await render(hbs``); assert.dom(".chat-thread-participants").doesNotExist(); }); - test("includeOriginalMessageUser=true", async function (assert) { + test("@includeOriginalMessageUser=true", async function (assert) { const orignalMessageUser = fabricators.user({ username: "bob" }); this.thread = fabricators.thread({ original_message: fabricators.message({ user: orignalMessageUser }), @@ -29,13 +29,12 @@ module( }), }); - await render(hbs``); + await render(hbs``); - assert.dom('.chat-user-avatar [data-user-card="bob"]').exists(); - assert.dom('.chat-user-avatar [data-user-card="alice"]').exists(); + assert.dom(".chat-user-avatar[data-username]").exists({ count: 2 }); }); - test("includeOriginalMessageUser=false", async function (assert) { + test("@includeOriginalMessageUser=false", async function (assert) { const orignalMessageUser = fabricators.user({ username: "bob" }); this.thread = fabricators.thread({ original_message: fabricators.message({ user: orignalMessageUser }), @@ -49,11 +48,11 @@ module( }); await render( - hbs`` + hbs`` ); - assert.dom('.chat-user-avatar [data-user-card="bob"]').doesNotExist(); - assert.dom('.chat-user-avatar [data-user-card="alice"]').exists(); + assert.dom('.chat-user-avatar[data-username="bob"]').doesNotExist(); + assert.dom('.chat-user-avatar[data-username="alice"]').exists(); }); } ); diff --git a/plugins/chat/test/javascripts/components/chat-user-avatar-test.js b/plugins/chat/test/javascripts/components/chat-user-avatar-test.js index 342ec3d3aae..f306e7c7b4a 100644 --- a/plugins/chat/test/javascripts/components/chat-user-avatar-test.js +++ b/plugins/chat/test/javascripts/components/chat-user-avatar-test.js @@ -13,15 +13,15 @@ function containerSelector(user, options = {}) { return `.chat-user-avatar${onlineSelector} .chat-user-avatar__container[data-user-card=${user.username}] .avatar[title=${user.username}]`; } -module("Discourse Chat | Component | ", function (hooks) { +module("Discourse Chat | Component | ", function (hooks) { setupRenderingTest(hooks); - test("user is not online", async function (assert) { + test("when user is not online", async function (assert) { this.user = fabricators.user(); this.chat = { presenceChannel: { users: [] } }; await render( - hbs`` + hbs`` ); assert.dom(containerSelector(this.user, { online: false })).exists(); @@ -34,22 +34,32 @@ module("Discourse Chat | Component | ", function (hooks) { }; await render( - hbs`` + hbs`` ); assert.dom(containerSelector(this.user, { online: true })).exists(); }); - test("showPresence=false", async function (assert) { + test("@showPresence=false", async function (assert) { this.user = fabricators.user(); this.chat = { presenceChannel: { users: [{ id: this.user.id }] }, }; await render( - hbs`` + hbs`` ); assert.dom(containerSelector(this.user, { online: false })).exists(); }); + + test("@interactive=true", async function (assert) { + this.user = fabricators.user(); + + await render( + hbs`` + ); + + assert.dom(".clickable").doesNotExist(); + }); });