From e91fa0ef80ed56f13ba349e2afe9d7a5a2658a3c Mon Sep 17 00:00:00 2001 From: Jarek Radosz Date: Thu, 10 Aug 2023 16:41:46 +0200 Subject: [PATCH] DEV: Convert user-tips functions into a service (#23032) The original motivation for this change was to avoid mutating imported modules (by stubbing imported functions in tests) Other than that it's a good practice to place code like this in services, especially (although not the case here) if it requires access to other services or controller. --- .../app/components/bootstrap-mode-notice.hbs | 1 - .../app/components/bootstrap-mode-notice.js | 7 +- .../discourse/app/components/user-tip.js | 62 +++--- .../discourse/app/lib/user-tips.js | 176 ---------------- .../javascripts/discourse/app/models/user.js | 19 +- .../discourse/app/services/user-tips.js | 196 ++++++++++++++++++ .../discourse/app/widgets/header.js | 7 +- .../javascripts/discourse/app/widgets/post.js | 7 +- .../discourse/app/widgets/widget.js | 3 +- .../tests/acceptance/user-tips-test.js | 16 -- .../discourse/tests/unit/models/user-test.js | 13 +- 11 files changed, 251 insertions(+), 256 deletions(-) delete mode 100644 app/assets/javascripts/discourse/app/lib/user-tips.js create mode 100644 app/assets/javascripts/discourse/app/services/user-tips.js diff --git a/app/assets/javascripts/discourse/app/components/bootstrap-mode-notice.hbs b/app/assets/javascripts/discourse/app/components/bootstrap-mode-notice.hbs index 3078dc4c160..8c1aab46f29 100644 --- a/app/assets/javascripts/discourse/app/components/bootstrap-mode-notice.hbs +++ b/app/assets/javascripts/discourse/app/components/bootstrap-mode-notice.hbs @@ -2,7 +2,6 @@ class="btn-default bootstrap-mode" @label="bootstrap_mode" @action={{this.routeToAdminGuide}} - {{did-insert this.setupUserTip}} > {{#if this.showUserTip}} diff --git a/app/assets/javascripts/discourse/app/components/bootstrap-mode-notice.js b/app/assets/javascripts/discourse/app/components/bootstrap-mode-notice.js index 128aff7fab0..7d986c7ef7e 100644 --- a/app/assets/javascripts/discourse/app/components/bootstrap-mode-notice.js +++ b/app/assets/javascripts/discourse/app/components/bootstrap-mode-notice.js @@ -10,12 +10,7 @@ export default class BootstrapModeNotice extends Component { @service currentUser; @service siteSettings; - @tracked showUserTip = false; - - @action - setupUserTip() { - this.showUserTip = this.currentUser?.canSeeUserTip("admin_guide"); - } + @tracked showUserTip = this.currentUser?.canSeeUserTip("admin_guide"); @action routeToAdminGuide() { diff --git a/app/assets/javascripts/discourse/app/components/user-tip.js b/app/assets/javascripts/discourse/app/components/user-tip.js index 9684bf54618..fdf78b8e0cc 100644 --- a/app/assets/javascripts/discourse/app/components/user-tip.js +++ b/app/assets/javascripts/discourse/app/components/user-tip.js @@ -1,12 +1,15 @@ -import { action } from "@ember/object"; -import { schedule } from "@ember/runloop"; -import { inject as service } from "@ember/service"; import Component from "@glimmer/component"; -import { hideUserTip } from "discourse/lib/user-tips"; +import { action } from "@ember/object"; +import { inject as service } from "@ember/service"; import I18n from "I18n"; export default class UserTip extends Component { @service currentUser; + @service userTips; + + willDestroy() { + this.userTips.hideTip(this.args.id); + } @action showUserTip(element) { @@ -14,36 +17,29 @@ export default class UserTip extends Component { return; } - schedule("afterRender", () => { - const { - id, - selector, - content, - placement, - buttonLabel, - buttonIcon, - onDismiss, - } = this.args; - element = element.parentElement; + const { + id, + selector, + content, + placement, + buttonLabel, + buttonIcon, + onDismiss, + } = this.args; + element = element.parentElement; - this.currentUser.showUserTip({ - id, - titleText: I18n.t(`user_tips.${id}.title`), - contentHtml: content, - contentText: I18n.t(`user_tips.${id}.content`), - buttonLabel, - buttonIcon, - reference: - (selector && element.parentElement.querySelector(selector)) || - element, - appendTo: element.parentElement, - placement, - onDismiss, - }); + this.currentUser.showUserTip({ + id, + titleText: I18n.t(`user_tips.${id}.title`), + contentHtml: content, + contentText: I18n.t(`user_tips.${id}.content`), + buttonLabel, + buttonIcon, + reference: + (selector && element.parentElement.querySelector(selector)) || element, + appendTo: element.parentElement, + placement, + onDismiss, }); } - - willDestroy() { - hideUserTip(this.args.id); - } } diff --git a/app/assets/javascripts/discourse/app/lib/user-tips.js b/app/assets/javascripts/discourse/app/lib/user-tips.js deleted file mode 100644 index 6a58bb06229..00000000000 --- a/app/assets/javascripts/discourse/app/lib/user-tips.js +++ /dev/null @@ -1,176 +0,0 @@ -import { isTesting } from "discourse-common/config/environment"; -import { iconHTML } from "discourse-common/lib/icon-library"; -import I18n from "I18n"; -import { escape } from "pretty-text/sanitizer"; -import tippy from "tippy.js"; -import isElementInViewport from "discourse/lib/is-element-in-viewport"; - -const TIPPY_DELAY = 500; - -const instancesMap = {}; -window.instancesMap = instancesMap; - -function destroyInstance(instance) { - if (instance.showTimeout) { - clearTimeout(instance.showTimeout); - instance.showTimeout = null; - } - - if (instance.destroyTimeout) { - clearTimeout(instance.destroyTimeout); - instance.destroyTimeout = null; - } - - instance.destroy(); -} - -function cancelDestroyInstance(instance) { - if (instance.destroyTimeout) { - clearTimeout(instance.destroyTimeout); - instance.destroyTimeout = null; - } -} - -function showInstance(instance) { - if (isTesting()) { - instance.show(); - } else if (!instance.showTimeout) { - instance.showTimeout = setTimeout(() => { - instance.showTimeout = null; - if (!instance.state.isDestroyed) { - instance.show(); - } - }, TIPPY_DELAY); - } -} - -function hideInstance(instance) { - clearTimeout(instance.showTimeout); - instance.showTimeout = null; - instance.hide(); -} - -export function showUserTip(options) { - // Find if a similar instance has been scheduled for destroying recently - // and cancel that - let instance = instancesMap[options.id]; - if (instance) { - if (instance.reference === options.reference) { - return cancelDestroyInstance(instance); - } else { - destroyInstance(instance); - delete instancesMap[options.id]; - } - } - - if (!options.reference) { - return; - } - - let buttonText = escape(I18n.t(options.buttonLabel || "user_tips.button")); - if (options.buttonIcon) { - buttonText = `${iconHTML(options.buttonIcon)} ${buttonText}`; - } - - instancesMap[options.id] = tippy(options.reference, { - hideOnClick: false, - trigger: "manual", - theme: "user-tip", - zIndex: "", // reset z-index to use inherited value from the parent - duration: TIPPY_DELAY, - - arrow: iconHTML("tippy-rounded-arrow"), - placement: options.placement, - appendTo: options.appendTo, - - interactive: true, // for buttons in content - allowHTML: true, - - content: - options.content || - `
-
${escape(options.titleText)}
-
${ - options.contentHtml || escape(options.contentText) - }
-
- -
-
`, - - onCreate(tippyInstance) { - // Used to set correct z-index property on root tippy element - tippyInstance.popper.classList.add("user-tip"); - - tippyInstance.popper - .querySelector(".btn") - .addEventListener("click", (event) => { - options.onDismiss?.(); - event.preventDefault(); - }); - }, - }); - - showNextUserTip(); -} - -export function hideUserTip(userTipId, force = false) { - // Tippy instances are not destroyed immediately because sometimes there - // user tip is recreated immediately. This happens when Ember components - // are re-rendered because a parent component has changed - - const instance = instancesMap[userTipId]; - if (!instance) { - return; - } - - if (force) { - destroyInstance(instance); - delete instancesMap[userTipId]; - showNextUserTip(); - } else if (!instance.destroyTimeout) { - instance.destroyTimeout = setTimeout(() => { - destroyInstance(instancesMap[userTipId]); - delete instancesMap[userTipId]; - showNextUserTip(); - }, TIPPY_DELAY); - } -} - -export function hideAllUserTips() { - Object.keys(instancesMap).forEach((userTipId) => { - destroyInstance(instancesMap[userTipId]); - delete instancesMap[userTipId]; - }); -} - -export function showNextUserTip() { - const instances = Object.values(instancesMap); - - // Return early if a user tip is already visible and it is in viewport - if ( - instances.find( - (instance) => - instance.state.isVisible && isElementInViewport(instance.reference) - ) - ) { - return; - } - - // Otherwise, try to find a user tip in the viewport - const idx = instances.findIndex((instance) => - isElementInViewport(instance.reference) - ); - - // If no instance was found, select first user tip - const newInstance = instances[idx === -1 ? 0 : idx]; - - // Show only selected instance and hide all the other ones - instances.forEach((instance) => { - if (instance === newInstance) { - showInstance(instance); - } else { - hideInstance(instance); - } - }); -} diff --git a/app/assets/javascripts/discourse/app/models/user.js b/app/assets/javascripts/discourse/app/models/user.js index e684d61fa02..ac345894269 100644 --- a/app/assets/javascripts/discourse/app/models/user.js +++ b/app/assets/javascripts/discourse/app/models/user.js @@ -36,11 +36,6 @@ import Evented from "@ember/object/evented"; import { cancel } from "@ember/runloop"; import discourseLater from "discourse-common/lib/later"; import { isTesting } from "discourse-common/config/environment"; -import { - hideUserTip, - showNextUserTip, - showUserTip, -} from "discourse/lib/user-tips"; import { dependentKeyCompat } from "@ember/object/compat"; export const SECOND_FACTOR_METHODS = { @@ -1180,7 +1175,7 @@ const User = RestModel.extend({ if (!userTips[id]) { if (!isTesting()) { // eslint-disable-next-line no-console - console.warn("Cannot show user tip with type =", id); + console.warn("Cannot show user tip with id", id); } return false; } @@ -1195,7 +1190,7 @@ const User = RestModel.extend({ showUserTip(options) { if (this.canSeeUserTip(options.id)) { - showUserTip({ + this.userTips.showTip({ ...options, onDismiss: () => { options.onDismiss?.(); @@ -1214,13 +1209,13 @@ const User = RestModel.extend({ // Empty userTipId means all user tips. if (!userTips[userTipId]) { // eslint-disable-next-line no-console - console.warn("Cannot hide user tip with type =", userTipId); + console.warn("Cannot hide user tip with id", userTipId); return; } // Hide user tips and maybe show the next one. - hideUserTip(userTipId, true); - showNextUserTip(); + this.userTips.hideTip(userTipId, true); + this.userTips.showNextTip(); // Update list of seen user tips. let seenUserTips = this.user_option?.seen_popups || []; @@ -1353,6 +1348,10 @@ User.reopenClass(Singleton, { create(args) { args = args || {}; this.deleteStatusTrackingFields(args); + + const owner = getOwner(this); + args.userTips = owner.lookup("service:user-tips"); + return this._super(args); }, diff --git a/app/assets/javascripts/discourse/app/services/user-tips.js b/app/assets/javascripts/discourse/app/services/user-tips.js new file mode 100644 index 00000000000..9de6f7e8b8d --- /dev/null +++ b/app/assets/javascripts/discourse/app/services/user-tips.js @@ -0,0 +1,196 @@ +import Service from "@ember/service"; +import { isTesting } from "discourse-common/config/environment"; +import { iconHTML } from "discourse-common/lib/icon-library"; +import I18n from "I18n"; +import { escape } from "pretty-text/sanitizer"; +import tippy from "tippy.js"; +import isElementInViewport from "discourse/lib/is-element-in-viewport"; +import discourseLater from "discourse-common/lib/later"; +import { cancel } from "@ember/runloop"; + +const TIPPY_DELAY = 500; + +export default class UserTips extends Service { + #instances = new Map(); + + /** + * @param {Object} options + * @param {Integer} options.id + * @param {Element} options.reference + * @param {string} [options.buttonLabel] + * @param {string} [options.buttonIcon] + * @param {string} [options.placement] + * @param {Element} [options.appendTo] + * @param {string} [options.content] + * @param {string} [options.contentText] + * @param {string} [options.titleText] + * @param {function} [options.onDismiss] + */ + showTip(options) { + // Find if a similar instance has been scheduled for destroying recently + // and cancel that + const instance = this.#instances.get(options.id); + + if (instance) { + if (instance.reference === options.reference) { + return this.#cancelDestroyInstance(instance); + } else { + this.#destroyInstance(instance); + this.#instances.delete(options.id); + } + } + + if (!options.reference) { + return; + } + + let buttonText = escape(I18n.t(options.buttonLabel || "user_tips.button")); + if (options.buttonIcon) { + buttonText = `${iconHTML(options.buttonIcon)} ${buttonText}`; + } + + this.#instances.set( + options.id, + tippy(options.reference, { + hideOnClick: false, + trigger: "manual", + theme: "user-tip", + zIndex: "", // reset z-index to use inherited value from the parent + duration: TIPPY_DELAY, + + arrow: iconHTML("tippy-rounded-arrow"), + placement: options.placement, + appendTo: options.appendTo, + + interactive: true, // for buttons in content + allowHTML: true, + + content: + options.content || + `
+
${escape(options.titleText)}
+
${ + options.contentHtml || escape(options.contentText) + }
+
+ +
+
`, + + onCreate(tippyInstance) { + // Used to set correct z-index property on root tippy element + tippyInstance.popper.classList.add("user-tip"); + + tippyInstance.popper + .querySelector(".btn") + .addEventListener("click", (event) => { + options.onDismiss?.(); + event.preventDefault(); + }); + }, + }) + ); + + this.showNextTip(); + } + + hideTip(userTipId, force = false) { + // Tippy instances are not destroyed immediately because sometimes there + // user tip is recreated immediately. This happens when Ember components + // are re-rendered because a parent component has changed + + const instance = this.#instances.get(userTipId); + if (!instance) { + return; + } + + if (force) { + this.#destroyInstance(instance); + this.#instances.delete(userTipId); + this.showNextTip(); + } else if (!instance.destroyTimer) { + instance.destroyTimer = discourseLater(() => { + this.#destroyInstance(this.#instances.get(userTipId)); + this.#instances.delete(userTipId); + this.showNextTip(); + }, TIPPY_DELAY); + } + } + + hideAll() { + for (const [id, tip] of this.#instances.entries()) { + this.#destroyInstance(tip); + this.#instances.delete(id); + } + } + + showNextTip() { + // Return early if a user tip is already visible and it is in viewport + for (const tip of this.#instances.values()) { + if (tip.state.isVisible && isElementInViewport(tip.reference)) { + return; + } + } + + // Otherwise, try to find a user tip in the viewport + let visibleTip; + for (const tip of this.#instances.values()) { + if (isElementInViewport(tip.reference)) { + visibleTip = tip; + break; + } + } + + // If no instance was found, select first user tip + const newTip = visibleTip || this.#instances.values().next(); + + // Show only selected instance and hide all the other ones + for (const tip of this.#instances.values()) { + if (tip === newTip) { + this.#showInstance(tip); + } else { + this.#hideInstance(tip); + } + } + } + + #destroyInstance(instance) { + if (instance.showTimer) { + cancel(instance.showTimer); + instance.showTimer = null; + } + + if (instance.destroyTimer) { + cancel(instance.destroyTimer); + instance.destroyTimer = null; + } + + instance.destroy(); + } + + #cancelDestroyInstance(instance) { + if (instance.destroyTimer) { + cancel(instance.destroyTimer); + instance.destroyTimer = null; + } + } + + #showInstance(instance) { + if (isTesting()) { + instance.show(); + } else if (!instance.showTimer) { + instance.showTimer = discourseLater(() => { + instance.showTimer = null; + if (!instance.state.isDestroyed) { + instance.show(); + } + }, TIPPY_DELAY); + } + } + + #hideInstance(instance) { + cancel(instance.showTimer); + instance.showTimer = null; + instance.hide(); + } +} diff --git a/app/assets/javascripts/discourse/app/widgets/header.js b/app/assets/javascripts/discourse/app/widgets/header.js index 2c6605f2c8c..eb4d1c34310 100644 --- a/app/assets/javascripts/discourse/app/widgets/header.js +++ b/app/assets/javascripts/discourse/app/widgets/header.js @@ -12,7 +12,6 @@ import { wantsNewWindow } from "discourse/lib/intercept-click"; import { logSearchLinkClick } from "discourse/lib/search"; import RenderGlimmer from "discourse/widgets/render-glimmer"; import { hbs } from "ember-cli-htmlbars"; -import { hideUserTip } from "discourse/lib/user-tips"; import { SEARCH_BUTTON_ID } from "discourse/components/search-menu"; let _extraHeaderIcons = []; @@ -47,6 +46,8 @@ export const dropdown = { }; createWidget("header-notifications", { + services: ["user-tips"], + settings: { avatarSize: "medium", }, @@ -169,11 +170,11 @@ createWidget("header-notifications", { }, destroy() { - hideUserTip("first_notification"); + this.userTips.hideTip("first_notification"); }, willRerenderWidget() { - hideUserTip("first_notification"); + this.userTips.hideTip("first_notification"); }, }); diff --git a/app/assets/javascripts/discourse/app/widgets/post.js b/app/assets/javascripts/discourse/app/widgets/post.js index d88ff170991..446f1c0d6e6 100644 --- a/app/assets/javascripts/discourse/app/widgets/post.js +++ b/app/assets/javascripts/discourse/app/widgets/post.js @@ -21,7 +21,6 @@ import { relativeAgeMediumSpan } from "discourse/lib/formatter"; import { transformBasicPost } from "discourse/lib/transform-post"; import autoGroupFlairForUser from "discourse/lib/avatar-flair"; import { nativeShare } from "discourse/lib/pwa-utils"; -import { hideUserTip } from "discourse/lib/user-tips"; import ShareTopicModal from "discourse/components/modal/share-topic"; import { getOwner } from "@ember/application"; @@ -871,7 +870,7 @@ export function addPostClassesCallback(callback) { export default createWidget("post", { buildKey: (attrs) => `post-${attrs.id}`, - services: ["dialog"], + services: ["dialog", "user-tips"], shadowTree: true, buildAttributes(attrs) { @@ -1004,10 +1003,10 @@ export default createWidget("post", { }, destroy() { - hideUserTip("post_menu"); + this.userTips.hideTip("post_menu"); }, willRerenderWidget() { - hideUserTip("post_menu"); + this.userTips.hideTip("post_menu"); }, }); diff --git a/app/assets/javascripts/discourse/app/widgets/widget.js b/app/assets/javascripts/discourse/app/widgets/widget.js index 8e226c46020..5c840d21649 100644 --- a/app/assets/javascripts/discourse/app/widgets/widget.js +++ b/app/assets/javascripts/discourse/app/widgets/widget.js @@ -26,6 +26,7 @@ import { h } from "virtual-dom"; import { isProduction } from "discourse-common/config/environment"; import { consolePrefix } from "discourse/lib/source-identifier"; import { getOwner, setOwner } from "@ember/application"; +import { camelize } from "@ember/string"; const _registry = {}; @@ -161,7 +162,7 @@ export default class Widget { // We can inject services into widgets by passing a `services` parameter on creation (this.services || []).forEach((s) => { - this[s] = register.lookup(`service:${s}`); + this[camelize(s)] = register.lookup(`service:${s}`); }); this.init(this.attrs); diff --git a/app/assets/javascripts/discourse/tests/acceptance/user-tips-test.js b/app/assets/javascripts/discourse/tests/acceptance/user-tips-test.js index cd450ff5107..37adc83a17a 100644 --- a/app/assets/javascripts/discourse/tests/acceptance/user-tips-test.js +++ b/app/assets/javascripts/discourse/tests/acceptance/user-tips-test.js @@ -1,5 +1,4 @@ import { visit } from "@ember/test-helpers"; -import { hideAllUserTips } from "discourse/lib/user-tips"; import { acceptance, query } from "discourse/tests/helpers/qunit-helpers"; import I18n from "I18n"; import { test } from "qunit"; @@ -8,9 +7,6 @@ acceptance("User Tips - first_notification", function (needs) { needs.user({ new_personal_messages_notifications_count: 1 }); needs.site({ user_tips: { first_notification: 1 } }); - needs.hooks.beforeEach(() => hideAllUserTips()); - needs.hooks.afterEach(() => hideAllUserTips()); - test("Shows first notification user tip", async function (assert) { this.siteSettings.enable_user_tips = true; @@ -26,9 +22,6 @@ acceptance("User Tips - topic_timeline", function (needs) { needs.user(); needs.site({ user_tips: { topic_timeline: 2 } }); - needs.hooks.beforeEach(() => hideAllUserTips()); - needs.hooks.afterEach(() => hideAllUserTips()); - test("Shows topic timeline user tip", async function (assert) { this.siteSettings.enable_user_tips = true; @@ -44,9 +37,6 @@ acceptance("User Tips - post_menu", function (needs) { needs.user(); needs.site({ user_tips: { post_menu: 3 } }); - needs.hooks.beforeEach(() => hideAllUserTips()); - needs.hooks.afterEach(() => hideAllUserTips()); - test("Shows post menu user tip", async function (assert) { this.siteSettings.enable_user_tips = true; @@ -62,9 +52,6 @@ acceptance("User Tips - topic_notification_levels", function (needs) { needs.user(); needs.site({ user_tips: { topic_notification_levels: 4 } }); - needs.hooks.beforeEach(() => hideAllUserTips()); - needs.hooks.afterEach(() => hideAllUserTips()); - test("Shows topic notification levels user tip", async function (assert) { this.siteSettings.enable_user_tips = true; @@ -81,9 +68,6 @@ acceptance("User Tips - suggested_topics", function (needs) { needs.user(); needs.site({ user_tips: { suggested_topics: 5 } }); - needs.hooks.beforeEach(() => hideAllUserTips()); - needs.hooks.afterEach(() => hideAllUserTips()); - test("Shows suggested topics user tip", async function (assert) { this.siteSettings.enable_user_tips = true; diff --git a/app/assets/javascripts/discourse/tests/unit/models/user-test.js b/app/assets/javascripts/discourse/tests/unit/models/user-test.js index 47eb9fc1983..c9ec4e4641b 100644 --- a/app/assets/javascripts/discourse/tests/unit/models/user-test.js +++ b/app/assets/javascripts/discourse/tests/unit/models/user-test.js @@ -6,7 +6,6 @@ import { settled } from "@ember/test-helpers"; import User from "discourse/models/user"; import pretender, { response } from "discourse/tests/helpers/create-pretender"; import { getOwner } from "discourse-common/lib/get-owner"; -import * as userTips from "discourse/lib/user-tips"; module("Unit | Model | user", function (hooks) { setupTest(hooks); @@ -224,15 +223,17 @@ module("Unit | Model | user", function (hooks) { test("hideUserTipForever() can hide the user tip", async function (assert) { const site = getOwner(this).lookup("service:site"); - site.set("user_tips", { first_notification: 1 }); const store = getOwner(this).lookup("service:store"); + const userTips = getOwner(this).lookup("service:user-tips"); + + site.set("user_tips", { first_notification: 1 }); const user = store.createRecord("user", { username: "eviltrout" }); - const hideSpy = sinon.spy(userTips, "hideUserTip"); - const showNextSpy = sinon.spy(userTips, "showNextUserTip"); + const hideSpy = sinon.spy(userTips, "hideTip"); + const showNextSpy = sinon.spy(userTips, "showNextTip"); await user.hideUserTipForever("first_notification"); - assert.ok(hideSpy.calledWith("first_notification")); - assert.ok(showNextSpy.calledWith()); + assert.true(hideSpy.calledWith("first_notification")); + assert.true(showNextSpy.calledWith()); }); });