From ccb1861ada774aae8ebf993d8afbe0fedc60af59 Mon Sep 17 00:00:00 2001 From: Joffrey JAFFEUX Date: Tue, 20 Aug 2024 14:37:28 +0200 Subject: [PATCH] DEV: better highlighting of mentions (#28403) This commit improves the hilight-ing of mentions in posts and chat messages. - `@here` and `@all` will generate a `` - bots will generate a `` - current user will generate a `` To achieve this change the following value transformer has been added: "mentions-class". It will be run in posts and chat messages after the mention is rendered. A bug were bots were not considered in mentioned users has also been fixed as part of this PR. --- .../instance-initializers/highlight-users.js | 21 +++++ .../discourse/app/lib/transformer/registry.js | 1 + .../discourse/app/widgets/post-cooked.js | 81 ++++++++++++------- .../transformers/mentions-class-test.js | 13 +++ .../discourse/tests/fixtures/topic.js | 1 + .../stylesheets/common/foundation/mixins.scss | 12 +++ .../discourse/components/chat-message.gjs | 61 ++++++++++---- .../discourse/initializers/chat-setup.js | 19 ----- .../stylesheets/common/chat-message.scss | 5 +- plugins/chat/lib/chat/parsed_mentions.rb | 2 +- .../spec/lib/chat/parsed_mentions_spec.rb | 9 +++ plugins/chat/spec/system/chat_channel_spec.rb | 13 ++- 12 files changed, 161 insertions(+), 77 deletions(-) create mode 100644 app/assets/javascripts/discourse/app/instance-initializers/highlight-users.js create mode 100644 app/assets/javascripts/discourse/tests/acceptance/transformers/mentions-class-test.js diff --git a/app/assets/javascripts/discourse/app/instance-initializers/highlight-users.js b/app/assets/javascripts/discourse/app/instance-initializers/highlight-users.js new file mode 100644 index 00000000000..56c7f43d25a --- /dev/null +++ b/app/assets/javascripts/discourse/app/instance-initializers/highlight-users.js @@ -0,0 +1,21 @@ +import { withPluginApi } from "discourse/lib/plugin-api"; + +export default { + initialize() { + withPluginApi("1.36.0", (api) => { + api.registerValueTransformer("mentions-class", ({ value, context }) => { + const { user } = context; + + if (user.id < 0) { + value.push("--bot"); + } else if (user.id === api.getCurrentUser()?.id) { + value.push("--current"); + } else if (user.username === "here" || user.username === "all") { + value.push("--wide"); + } + + return value; + }); + }); + }, +}; diff --git a/app/assets/javascripts/discourse/app/lib/transformer/registry.js b/app/assets/javascripts/discourse/app/lib/transformer/registry.js index f111349d31a..370ff003b39 100644 --- a/app/assets/javascripts/discourse/app/lib/transformer/registry.js +++ b/app/assets/javascripts/discourse/app/lib/transformer/registry.js @@ -7,6 +7,7 @@ export const VALUE_TRANSFORMERS = Object.freeze([ // use only lowercase names "category-description-text", "category-display-name", + "mentions-class", "header-notifications-avatar-size", "home-logo-href", "home-logo-image-url", diff --git a/app/assets/javascripts/discourse/app/widgets/post-cooked.js b/app/assets/javascripts/discourse/app/widgets/post-cooked.js index f367c8def4b..2eca9e4044b 100644 --- a/app/assets/javascripts/discourse/app/widgets/post-cooked.js +++ b/app/assets/javascripts/discourse/app/widgets/post-cooked.js @@ -4,6 +4,7 @@ import { isValidLink } from "discourse/lib/click-track"; import { number } from "discourse/lib/formatter"; import highlightHTML, { unhighlightHTML } from "discourse/lib/highlight-html"; import highlightSearch from "discourse/lib/highlight-search"; +import { applyValueTransformer } from "discourse/lib/transformer"; import { destroyUserStatusOnMentions, updateUserStatusOnMention, @@ -66,16 +67,15 @@ export default class PostCooked { // todo should be a better way of detecting if it is composer preview this._isInComposerPreview = !this.decoratorHelper; - const cookedDiv = this._computeCooked(); - this.cookedDiv = cookedDiv; + this.cookedDiv = this._computeCooked(); - this._insertQuoteControls(cookedDiv); - this._showLinkCounts(cookedDiv); - this._applySearchHighlight(cookedDiv); - this._initUserStatusOnMentions(); - this._decorateAndAdopt(cookedDiv); + this._insertQuoteControls(this.cookedDiv); + this._showLinkCounts(this.cookedDiv); + this._applySearchHighlight(this.cookedDiv); + this._decorateMentions(); + this._decorateAndAdopt(this.cookedDiv); - return cookedDiv; + return this.cookedDiv; } destroy() { @@ -365,9 +365,7 @@ export default class PostCooked { if ( (this.attrs.firstPost || this.attrs.embeddedPost) && - this.ignoredUsers && - this.ignoredUsers.length > 0 && - this.ignoredUsers.includes(this.attrs.username) + this.ignoredUsers?.includes?.(this.attrs.username) ) { cookedDiv.classList.add("post-ignored"); cookedDiv.innerHTML = I18n.t("post.ignored"); @@ -378,44 +376,65 @@ export default class PostCooked { return cookedDiv; } - _initUserStatusOnMentions() { + _decorateMentions() { if (!this._isInComposerPreview) { - this._trackMentionedUsersStatus(); - this._rerenderUserStatusOnMentions(); + destroyUserStatusOnMentions(); } + + this._extractMentions().forEach(({ mentions, user }) => { + if (!this._isInComposerPreview) { + this._trackMentionedUserStatus(user); + this._rerenderUserStatusOnMentions(mentions, user); + } + + const classes = applyValueTransformer("mentions-class", [], { + user, + }); + + mentions.forEach((mention) => { + mention.classList.add(...classes); + }); + }); } - _rerenderUserStatusOnMentions() { - destroyUserStatusOnMentions(); - this._post()?.mentioned_users?.forEach((user) => - this._rerenderUserStatusOnMention(this.cookedDiv, user) - ); - } - - _rerenderUserStatusOnMention(postElement, user) { - const href = getURL(`/u/${user.username.toLowerCase()}`); - const mentions = postElement.querySelectorAll(`a.mention[href="${href}"]`); - + _rerenderUserStatusOnMentions(mentions, user) { mentions.forEach((mention) => { updateUserStatusOnMention( - getOwnerWithFallback(this._post()), + getOwnerWithFallback(this), mention, user.status ); }); } - _trackMentionedUsersStatus() { - this._post()?.mentioned_users?.forEach((user) => { - user.statusManager?.trackStatus?.(); - user.on?.("status-changed", this, "_rerenderUserStatusOnMentions"); + _rerenderUsersStatusOnMentions() { + this._extractMentions().forEach(({ mentions, user }) => { + this._rerenderUserStatusOnMentions(mentions, user); }); } + _extractMentions() { + return ( + this._post()?.mentioned_users?.map((user) => { + const href = getURL(`/u/${user.username.toLowerCase()}`); + const mentions = this.cookedDiv.querySelectorAll( + `a.mention[href="${href}"]` + ); + + return { user, mentions }; + }) || [] + ); + } + + _trackMentionedUserStatus(user) { + user.statusManager?.trackStatus?.(); + user.on?.("status-changed", this, "_rerenderUsersStatusOnMentions"); + } + _stopTrackingMentionedUsersStatus() { this._post()?.mentioned_users?.forEach((user) => { user.statusManager?.stopTrackingStatus?.(); - user.off?.("status-changed", this, "_rerenderUserStatusOnMentions"); + user.off?.("status-changed", this, "_rerenderUsersStatusOnMentions"); }); } diff --git a/app/assets/javascripts/discourse/tests/acceptance/transformers/mentions-class-test.js b/app/assets/javascripts/discourse/tests/acceptance/transformers/mentions-class-test.js new file mode 100644 index 00000000000..4be9c5f43ab --- /dev/null +++ b/app/assets/javascripts/discourse/tests/acceptance/transformers/mentions-class-test.js @@ -0,0 +1,13 @@ +import { visit } from "@ember/test-helpers"; +import { test } from "qunit"; +import { acceptance } from "discourse/tests/helpers/qunit-helpers"; + +acceptance("mentions-class transformer", function (needs) { + needs.user(); + + test("applying a value transformation", async function (assert) { + await visit("/t/internationalization-localization/280"); + + assert.dom(".mention[href='/u/eviltrout']").hasClass("--current"); + }); +}); diff --git a/app/assets/javascripts/discourse/tests/fixtures/topic.js b/app/assets/javascripts/discourse/tests/fixtures/topic.js index 14ba8c54df9..40c009540b4 100644 --- a/app/assets/javascripts/discourse/tests/fixtures/topic.js +++ b/app/assets/javascripts/discourse/tests/fixtures/topic.js @@ -667,6 +667,7 @@ export default { avatar_template: "/images/avatar.png", uploaded_avatar_id: 5253, created_at: "2013-02-07T14:02:07.869Z", + mentioned_users: [{id: 19, username: "eviltrout"}], cooked: '

\n\n

The problem I see here is that this file is likely two grow and change massively over the next couple months, and tracking these changes in order to keep a localized file up to date is going to be a bitch.

\n\n

I wonder where there is a tool that can compare two yml structures and point out which nodes are missing? That would help keep track of new strings.

\n\n

Re keeping track of changed strings, @eviltrout I found this very interesting: http://stackoverflow.com/questions/4232922/why-do-people-use-plain-english-as-translation-placeholders if plain English placeholders were used, any change in strings would lead to a new node in the yml file, making keeping the translation up to date easier. Maybe worth thinking about in the future.

', post_number: 5, diff --git a/app/assets/stylesheets/common/foundation/mixins.scss b/app/assets/stylesheets/common/foundation/mixins.scss index aed5ce8dc2b..97f570b2d62 100644 --- a/app/assets/stylesheets/common/foundation/mixins.scss +++ b/app/assets/stylesheets/common/foundation/mixins.scss @@ -262,6 +262,18 @@ $hpad: 0.65em; border-radius: 0.6em; text-decoration: none; text-wrap: nowrap; + + &.--bot { + background: var(--highlight-low-or-medium); + } + + &.--wide { + background: var(--highlight); + } + + &.--current { + background: var(--tertiary-low); + } } @mixin nav-active() { diff --git a/plugins/chat/assets/javascripts/discourse/components/chat-message.gjs b/plugins/chat/assets/javascripts/discourse/components/chat-message.gjs index 0e25431bf2f..e4cf9cbc92d 100644 --- a/plugins/chat/assets/javascripts/discourse/components/chat-message.gjs +++ b/plugins/chat/assets/javascripts/discourse/components/chat-message.gjs @@ -15,9 +15,11 @@ import { eq, not } from "truth-helpers"; import DButton from "discourse/components/d-button"; import concatClass from "discourse/helpers/concat-class"; import optionalService from "discourse/lib/optional-service"; +import { applyValueTransformer } from "discourse/lib/transformer"; import { updateUserStatusOnMention } from "discourse/lib/update-user-status-on-mention"; import isZoomed from "discourse/lib/zoom-check"; import discourseDebounce from "discourse-common/lib/debounce"; +import getURL from "discourse-common/lib/get-url"; import discourseLater from "discourse-common/lib/later"; import { bind } from "discourse-common/utils/decorators"; import I18n from "discourse-i18n"; @@ -95,11 +97,6 @@ export default class ChatMessage extends Component { }; }); - constructor() { - super(...arguments); - this.initMentionedUsers(); - } - get pane() { return this.threadContext ? this.chatThreadPane : this.chatChannelPane; } @@ -214,6 +211,8 @@ export default class ChatMessage extends Component { @action didInsertMessage(element) { this.messageContainer = element; + this.initMentionedUsers(); + this.decorateMentions(element); this.debounceDecorateCookedMessage(); this.refreshStatusOnMentions(); } @@ -239,6 +238,46 @@ export default class ChatMessage extends Component { ); } + initMentionedUsers() { + this.args.message.mentionedUsers.forEach((user) => { + if (!user.statusManager.isTrackingStatus()) { + user.statusManager.trackStatus(); + user.on("status-changed", this, "refreshStatusOnMentions"); + } + }); + } + + decorateMentions(cooked) { + if (this.args.message.channel.allowChannelWideMentions) { + const wideMentions = [...cooked.querySelectorAll("span.mention")]; + MENTION_KEYWORDS.forEach((keyword) => { + const mentions = wideMentions.filter((node) => { + return node.textContent.trim() === `@${keyword}`; + }); + + const classes = applyValueTransformer("mentions-class", [], { + user: { username: keyword }, + }); + + mentions.forEach((mention) => { + mention.classList.add(...classes); + }); + }); + } + + this.args.message.mentionedUsers.forEach((user) => { + const href = getURL(`/u/${user.username.toLowerCase()}`); + const mentions = cooked.querySelectorAll(`a.mention[href="${href}"]`); + const classes = applyValueTransformer("mentions-class", [], { + user, + }); + + mentions.forEach((mention) => { + mention.classList.add(...classes); + }); + }); + } + @action decorateCookedMessage(message) { schedule("afterRender", () => { @@ -248,18 +287,6 @@ export default class ChatMessage extends Component { }); } - @action - initMentionedUsers() { - this.args.message.mentionedUsers.forEach((user) => { - if (user.statusManager.isTrackingStatus()) { - return; - } - - user.statusManager.trackStatus(); - user.on("status-changed", this, "refreshStatusOnMentions"); - }); - } - get show() { return ( !this.args.message?.deletedAt || diff --git a/plugins/chat/assets/javascripts/discourse/initializers/chat-setup.js b/plugins/chat/assets/javascripts/discourse/initializers/chat-setup.js index 38a3762036d..7ed4a6f3728 100644 --- a/plugins/chat/assets/javascripts/discourse/initializers/chat-setup.js +++ b/plugins/chat/assets/javascripts/discourse/initializers/chat-setup.js @@ -3,7 +3,6 @@ import { getOwnerWithFallback } from "discourse-common/lib/get-owner"; import { replaceIcon } from "discourse-common/lib/icon-library"; import { bind } from "discourse-common/utils/decorators"; import I18n from "discourse-i18n"; -import { MENTION_KEYWORDS } from "discourse/plugins/chat/discourse/components/chat-message"; import { clearChatComposerButtons } from "discourse/plugins/chat/discourse/lib/chat-composer-buttons"; import ChannelHashtagType from "discourse/plugins/chat/discourse/lib/hashtag-types/channel"; import ChatHeaderIcon from "../components/chat/header/icon"; @@ -164,24 +163,6 @@ export default { document.body.classList.remove("chat-drawer-active"); } }); - - api.decorateChatMessage(function (chatMessage, chatChannel) { - if (!this.currentUser) { - return; - } - - const highlightable = [`@${this.currentUser.username}`]; - if (chatChannel.allowChannelWideMentions) { - highlightable.push(...MENTION_KEYWORDS.map((k) => `@${k}`)); - } - - chatMessage.querySelectorAll(".mention").forEach((node) => { - const mention = node.textContent.trim(); - if (highlightable.includes(mention)) { - node.classList.add("highlighted", "valid-mention"); - } - }); - }); }); }, diff --git a/plugins/chat/assets/stylesheets/common/chat-message.scss b/plugins/chat/assets/stylesheets/common/chat-message.scss index bbc67f79925..c34d4a4f741 100644 --- a/plugins/chat/assets/stylesheets/common/chat-message.scss +++ b/plugins/chat/assets/stylesheets/common/chat-message.scss @@ -70,7 +70,8 @@ width: 100%; } - a.mention { + a.mention, + span.mention.--wide { @include mention; &.highlighted { @@ -79,7 +80,7 @@ } // unlinked, invalid mention - span.mention { + span.mention:not(--wide) { color: var(--primary-high); } diff --git a/plugins/chat/lib/chat/parsed_mentions.rb b/plugins/chat/lib/chat/parsed_mentions.rb index 20b04f1e2c3..645d4f27ee5 100644 --- a/plugins/chat/lib/chat/parsed_mentions.rb +++ b/plugins/chat/lib/chat/parsed_mentions.rb @@ -86,7 +86,7 @@ module Chat end def chat_users - User.distinct.joins(:user_option).real.where(user_options: { chat_enabled: true }) + User.distinct.joins(:user_option).where(user_options: { chat_enabled: true }) end def mentionable_groups diff --git a/plugins/chat/spec/lib/chat/parsed_mentions_spec.rb b/plugins/chat/spec/lib/chat/parsed_mentions_spec.rb index 302258b67b4..a1f1f25dc4b 100644 --- a/plugins/chat/spec/lib/chat/parsed_mentions_spec.rb +++ b/plugins/chat/spec/lib/chat/parsed_mentions_spec.rb @@ -145,6 +145,15 @@ RSpec.describe Chat::ParsedMentions do expect(result).to contain_exactly(channel_member_1.username, channel_member_2.username) end + it "returns bots who were mentioned directly" do + message = create_message("mentioning @system") + + mentions = described_class.new(message) + result = mentions.direct_mentions.pluck(:username) + + expect(result).to contain_exactly(Discourse.system_user.username) + end + it "returns a user when self-mentioning" do message = create_message("Hey @#{channel_member_1.username}", user: channel_member_1) diff --git a/plugins/chat/spec/system/chat_channel_spec.rb b/plugins/chat/spec/system/chat_channel_spec.rb index 3326335d408..dbae005662f 100644 --- a/plugins/chat/spec/system/chat_channel_spec.rb +++ b/plugins/chat/spec/system/chat_channel_spec.rb @@ -176,7 +176,8 @@ RSpec.describe "Chat channel", type: :system do Fabricate( :chat_message, chat_channel: channel_1, - message: "hello @here @all @#{current_user.username} @#{other_user.username} @unexisting", + message: + "hello @here @all @#{current_user.username} @#{other_user.username} @unexisting @system", user: other_user, ) end @@ -191,14 +192,12 @@ RSpec.describe "Chat channel", type: :system do it "highlights the mentions" do chat_page.visit_channel(channel_1) - expect(page).to have_selector(".mention.highlighted.valid-mention", text: "@here") - expect(page).to have_selector(".mention.highlighted.valid-mention", text: "@all") - expect(page).to have_selector( - ".mention.highlighted.valid-mention", - text: "@#{current_user.username}", - ) + expect(page).to have_selector(".mention.--wide", text: "@here") + expect(page).to have_selector(".mention.--wide", text: "@all") + expect(page).to have_selector(".mention.--current", text: "@#{current_user.username}") expect(page).to have_selector(".mention", text: "@#{other_user.username}") expect(page).to have_selector(".mention", text: "@unexisting") + expect(page).to have_selector(".mention.--bot", text: "@system") end it "renders user status on mentions" do