From e1ba4c6b73b9a26ccfd43499c223ce865bdf79a3 Mon Sep 17 00:00:00 2001 From: Alan Guo Xiang Tan Date: Wed, 31 May 2023 21:58:45 +0900 Subject: [PATCH] PERF: Client side triggering multiple requests when opening composer (#21823) What is the problem? There are two problems being fixed here: 1. When opening the composer, we are seeing multiple requests made to the `/composer_messages` endpoint. This is due to our use of the `transitionend` event on the `#reply-control` element. The event is fired once for each transition event and the `#reply-control` element has multiple transition events. 2. System tests have animations disabled so the `transitionend` event does not fire at all. What is the solution? Instead of relying on the `transitionend` event, we can instead just observer the `composerState` property of the `ComposerBody` component and trigger the `composer:opened` appEvent with a delay that is similar to the transition duration used for the `ComposerBody` component. --- .../discourse/app/components/composer-body.js | 13 ------------- .../javascripts/discourse/app/models/composer.js | 7 +++++++ .../tests/acceptance/composer-messages-test.js | 3 --- app/assets/stylesheets/common/base/compose.scss | 3 +++ .../composer/dont_feed_the_trolls_popup_spec.rb | 4 ---- 5 files changed, 10 insertions(+), 20 deletions(-) diff --git a/app/assets/javascripts/discourse/app/components/composer-body.js b/app/assets/javascripts/discourse/app/components/composer-body.js index 2d9414dec42..cc8b9edce3b 100644 --- a/app/assets/javascripts/discourse/app/components/composer-body.js +++ b/app/assets/javascripts/discourse/app/components/composer-body.js @@ -170,20 +170,7 @@ export default Component.extend(KeyEnterEscape, { didInsertElement() { this._super(...arguments); - this.setupComposerResizeEvents(); - - const triggerOpen = () => { - if (this.get("composer.composeState") === Composer.OPEN) { - this.appEvents.trigger("composer:opened"); - } - }; - triggerOpen(); - - this.element.addEventListener("transitionend", () => { - triggerOpen(); - }); - positioningWorkaround(this.element); }, diff --git a/app/assets/javascripts/discourse/app/models/composer.js b/app/assets/javascripts/discourse/app/models/composer.js index 2f744fc2889..367f4c77e27 100644 --- a/app/assets/javascripts/discourse/app/models/composer.js +++ b/app/assets/javascripts/discourse/app/models/composer.js @@ -5,6 +5,7 @@ import discourseComputed, { observes, on, } from "discourse-common/utils/decorators"; +import discourseLater from "discourse-common/lib/later"; import { emailValid, escapeExpression, @@ -225,6 +226,12 @@ const Composer = RestModel.extend({ if (this.composeState === OPEN) { this.set("composerOpened", oldOpen || new Date()); elem.classList.add("composer-open"); + + // If the duration changes here, it must also be changed in `stylesheets/common/base/compose.scss` which is where + // the transition duration is defined. A delay is added here as we only want to fire the event after the transition ends. + discourseLater(() => { + this.appEvents.trigger("composer:opened"); + }, 250); } else { if (oldOpen) { const oldTotal = this.composerTotalOpened || 0; diff --git a/app/assets/javascripts/discourse/tests/acceptance/composer-messages-test.js b/app/assets/javascripts/discourse/tests/acceptance/composer-messages-test.js index aab3fdc86bb..774a9dbbb16 100644 --- a/app/assets/javascripts/discourse/tests/acceptance/composer-messages-test.js +++ b/app/assets/javascripts/discourse/tests/acceptance/composer-messages-test.js @@ -138,9 +138,6 @@ acceptance("Composer - Messages - Duplicate links", function (needs) { await visit("/t/internationalization-localization/280"); await click("button.create"); - // Work around the lack of CSS transitions in the test env - query("#reply-control").dispatchEvent(new Event("transitionend")); - assert .dom(".composer-popup") .doesNotExist("composer warning is not shown by default"); diff --git a/app/assets/stylesheets/common/base/compose.scss b/app/assets/stylesheets/common/base/compose.scss index f556c5b26d1..e990d3c44ef 100644 --- a/app/assets/stylesheets/common/base/compose.scss +++ b/app/assets/stylesheets/common/base/compose.scss @@ -23,8 +23,11 @@ html.composer-open { min-width: 0; } z-index: z("composer", "content"); + + // If the transition duration is changed here, it must also be changed in the `composeStateChanged` function of models/composer.js transition: height 250ms ease, background 250ms ease, transform 250ms ease, max-width 250ms ease, padding-bottom 250ms ease; + background-color: var(--secondary); box-shadow: shadow("composer"); diff --git a/spec/system/composer/dont_feed_the_trolls_popup_spec.rb b/spec/system/composer/dont_feed_the_trolls_popup_spec.rb index 4bb039c6123..ece0cc65b6a 100644 --- a/spec/system/composer/dont_feed_the_trolls_popup_spec.rb +++ b/spec/system/composer/dont_feed_the_trolls_popup_spec.rb @@ -12,10 +12,6 @@ describe "Composer don't feed the trolls popup", type: :system, js: true do before { sign_in user } it "shows a popup when about to reply to a troll" do - skip( - "TGX: This does not work when Capybara.disable_animation is set to true. We're in the midst of fixing this.", - ) - SiteSetting.educate_until_posts = 0 topic_page.visit_topic(topic)