From 8203bdfbc96f5c4027a2525e9d98deae6e862db2 Mon Sep 17 00:00:00 2001 From: Kris Date: Tue, 3 Dec 2024 13:30:15 -0500 Subject: [PATCH] UX: move topic summary from DMenu to DModal (#992) Co-authored-by: Keegan George --- .../modal/ai-summary-modal.gjs} | 170 +++++++----------- .../ai-summary-trigger.gjs | 30 ++++ .../summarization/common/ai-summary.scss | 60 ++++--- .../summarization/desktop/ai-summary.scss | 56 +++--- .../summarization/mobile/ai-summary.scss | 3 - plugin.rb | 1 - .../page_objects/components/ai_summary_box.rb | 8 +- .../summarization/topic_summarization_spec.rb | 2 +- .../acceptance/topic-summary-test.js | 19 +- 9 files changed, 171 insertions(+), 178 deletions(-) rename assets/javascripts/discourse/{connectors/topic-map-expanded-after/ai-summary-box.gjs => components/modal/ai-summary-modal.gjs} (57%) create mode 100644 assets/javascripts/discourse/connectors/topic-map-expanded-after/ai-summary-trigger.gjs delete mode 100644 assets/stylesheets/modules/summarization/mobile/ai-summary.scss diff --git a/assets/javascripts/discourse/connectors/topic-map-expanded-after/ai-summary-box.gjs b/assets/javascripts/discourse/components/modal/ai-summary-modal.gjs similarity index 57% rename from assets/javascripts/discourse/connectors/topic-map-expanded-after/ai-summary-box.gjs rename to assets/javascripts/discourse/components/modal/ai-summary-modal.gjs index 83683aa4..53934704 100644 --- a/assets/javascripts/discourse/connectors/topic-map-expanded-after/ai-summary-box.gjs +++ b/assets/javascripts/discourse/components/modal/ai-summary-modal.gjs @@ -7,26 +7,29 @@ import didUpdate from "@ember/render-modifiers/modifiers/did-update"; import willDestroy from "@ember/render-modifiers/modifiers/will-destroy"; import { cancel, later } from "@ember/runloop"; import { service } from "@ember/service"; +import { not } from "truth-helpers"; import CookText from "discourse/components/cook-text"; import DButton from "discourse/components/d-button"; +import DModal from "discourse/components/d-modal"; import concatClass from "discourse/helpers/concat-class"; +import htmlClass from "discourse/helpers/html-class"; import { ajax } from "discourse/lib/ajax"; import { shortDateNoYear } from "discourse/lib/formatter"; import dIcon from "discourse-common/helpers/d-icon"; import i18n from "discourse-common/helpers/i18n"; import { bind } from "discourse-common/utils/decorators"; import I18n from "discourse-i18n"; -import DMenu from "float-kit/components/d-menu"; import DTooltip from "float-kit/components/d-tooltip"; import AiSummarySkeleton from "../../components/ai-summary-skeleton"; const STREAMED_TEXT_SPEED = 15; -export default class AiSummaryBox extends Component { +export default class AiSummaryModal extends Component { @service siteSettings; @service messageBus; @service currentUser; @service site; + @service modal; @tracked text = ""; @tracked summarizedOn = null; @@ -68,11 +71,11 @@ export default class AiSummaryBox extends Component { } get topRepliesSummaryEnabled() { - return this.args.outletArgs.postStream.summary; + return this.args.model.postStream.summary; } get topicId() { - return this.args.outletArgs.topic.id; + return this.args.model.topic.id; } get baseSummarizationURL() { @@ -80,13 +83,8 @@ export default class AiSummaryBox extends Component { } @bind - subscribe(unsubscribe, [topicId]) { - const sameTopicId = this.args.outletArgs.topic.id === topicId; - - if (unsubscribe && this._channel && !sameTopicId) { - this.unsubscribe(); - } - const channel = `/discourse-ai/summaries/topic/${this.args.outletArgs.topic.id}`; + subscribe() { + const channel = `/discourse-ai/summaries/topic/${this.args.model.topic.id}`; this._channel = channel; this.messageBus.subscribe(channel, this._updateSummary); } @@ -206,100 +204,66 @@ export default class AiSummaryBox extends Component { } @action - async onClose() { - await this.dMenu.close(); - this.unsubscribe(); + handleClose() { + this.modal.triggerElement = null; // prevent refocus of trigger, which changes scroll position + this.args.closeModal(); } } diff --git a/assets/javascripts/discourse/connectors/topic-map-expanded-after/ai-summary-trigger.gjs b/assets/javascripts/discourse/connectors/topic-map-expanded-after/ai-summary-trigger.gjs new file mode 100644 index 00000000..fe27fdf2 --- /dev/null +++ b/assets/javascripts/discourse/connectors/topic-map-expanded-after/ai-summary-trigger.gjs @@ -0,0 +1,30 @@ +import Component from "@glimmer/component"; +import { action } from "@ember/object"; +import { service } from "@ember/service"; +import DButton from "discourse/components/d-button"; +import AiSummaryModal from "../../components/modal/ai-summary-modal"; + +export default class AiSummaryTrigger extends Component { + @service modal; + + @action + openAiSummaryModal() { + this.modal.show(AiSummaryModal, { + model: { + topic: this.args.outletArgs.topic, + postStream: this.args.outletArgs.postStream, + }, + }); + } + + +} diff --git a/assets/stylesheets/modules/summarization/common/ai-summary.scss b/assets/stylesheets/modules/summarization/common/ai-summary.scss index 3bd13803..02221ec3 100644 --- a/assets/stylesheets/modules/summarization/common/ai-summary.scss +++ b/assets/stylesheets/modules/summarization/common/ai-summary.scss @@ -24,25 +24,9 @@ } } } - - .topic-map__additional-contents { - .ai-summarization-button { - padding-block: 0.5em; - display: flex; - gap: 0.5em; - - button span { - white-space: nowrap; - } - } - } } -.topic-map__ai-summary-content { - .ai-summary-container { - width: 100vw; - } - +.ai-summary-modal { .ai-summary { &__list { list-style: none; @@ -174,14 +158,6 @@ margin: 0; } - .summarized-on p { - display: flex; - align-items: center; - justify-content: flex-end; - gap: 0.25em; - margin-bottom: 0; - } - .outdated-summary { display: flex; flex-direction: column; @@ -193,6 +169,40 @@ color: var(--primary-medium); } } + + .d-modal__footer { + display: grid; + gap: 0; + grid-template-areas: "summarized regenerate" " outdated regenerate"; + grid-template-columns: 1fr auto; + @include breakpoint(mobile-large) { + gap: 0.25em 0.5em; + grid-template-areas: "summarized summarized" "regenerate outdated"; + } + + p { + margin: 0; + } + + .fk-d-tooltip__trigger { + vertical-align: text-top; + } + + .summary-outdated { + color: var(--primary-high); + font-size: var(--font-down-1); + line-height: var(--line-height-medium); + } + + .summarized-on { + grid-area: summarized; + } + + button { + grid-area: regenerate; + justify-self: start; + } + } } @keyframes appear { diff --git a/assets/stylesheets/modules/summarization/desktop/ai-summary.scss b/assets/stylesheets/modules/summarization/desktop/ai-summary.scss index 30b20e2f..9026771c 100644 --- a/assets/stylesheets/modules/summarization/desktop/ai-summary.scss +++ b/assets/stylesheets/modules/summarization/desktop/ai-summary.scss @@ -1,39 +1,27 @@ -.topic-map { - .ai-summarization-button { - .fk-d-menu { - position: fixed; - top: calc(var(--header-offset) + 1rem) !important; - left: unset !important; - right: 1rem !important; - width: 470px; - max-width: 470px !important; //overruling JS - max-height: calc( - 100vh - var(--header-offset) - 3rem - var(--composer-height, 0px) - ); +html.scrollable-modal { + overflow: auto; // overrides core .modal-open class scroll lock +} - .ai-summary__header, - .ai-summary-box { - padding: 0.75em 1rem; - box-sizing: border-box; - } +.ai-summary-modal { + .d-modal__container { + position: fixed; + top: var(--header-offset); + margin-top: 1em; + right: 1em; + width: 100vw; + max-width: 30em; + max-height: calc( + 100vh - var(--header-offset) - 3rem - var(--composer-height, 0px) + ); - .ai-summary { - &__header { - position: sticky; - top: 0; - display: flex; - justify-content: space-between; - align-items: center; - padding-block: 0.5rem; - padding-right: 0.5rem; - background: var(--secondary); - border-bottom: 1px solid var(--primary-low); + box-shadow: var(--shadow-menu-panel); + } - h3 { - margin: 0; - } - } - } - } + .fullscreen-composer & { + display: none; } } + +.ai-summary-modal + .d-modal__backdrop { + display: none; +} diff --git a/assets/stylesheets/modules/summarization/mobile/ai-summary.scss b/assets/stylesheets/modules/summarization/mobile/ai-summary.scss deleted file mode 100644 index e9092b13..00000000 --- a/assets/stylesheets/modules/summarization/mobile/ai-summary.scss +++ /dev/null @@ -1,3 +0,0 @@ -.ai-summary-box { - padding: 0.75em 1rem; -} diff --git a/plugin.rb b/plugin.rb index 0a511b32..9cc3a8c2 100644 --- a/plugin.rb +++ b/plugin.rb @@ -20,7 +20,6 @@ register_asset "stylesheets/modules/ai-helper/common/ai-helper.scss" register_asset "stylesheets/modules/ai-helper/desktop/ai-helper-fk-modals.scss", :desktop register_asset "stylesheets/modules/ai-helper/mobile/ai-helper.scss", :mobile -register_asset "stylesheets/modules/summarization/mobile/ai-summary.scss", :mobile register_asset "stylesheets/modules/summarization/common/ai-summary.scss" register_asset "stylesheets/modules/summarization/desktop/ai-summary.scss", :desktop diff --git a/spec/system/page_objects/components/ai_summary_box.rb b/spec/system/page_objects/components/ai_summary_box.rb index c31e1742..73bb5ee4 100644 --- a/spec/system/page_objects/components/ai_summary_box.rb +++ b/spec/system/page_objects/components/ai_summary_box.rb @@ -2,16 +2,16 @@ module PageObjects module Components - class AiSummaryBox < PageObjects::Components::Base - SUMMARY_BUTTON_SELECTOR = ".ai-summarization-button button" - SUMMARY_CONTAINER_SELECTOR = ".ai-summary-container" + class AiSummaryTrigger < PageObjects::Components::Base + SUMMARY_BUTTON_SELECTOR = ".ai-summarization-button" + SUMMARY_CONTAINER_SELECTOR = ".ai-summary-modal" def click_summarize find(SUMMARY_BUTTON_SELECTOR).click end def click_regenerate_summary - find("#{SUMMARY_CONTAINER_SELECTOR} .outdated-summary button").click + find("#{SUMMARY_CONTAINER_SELECTOR} .d-modal__footer button").click end def has_summary?(summary) diff --git a/spec/system/summarization/topic_summarization_spec.rb b/spec/system/summarization/topic_summarization_spec.rb index f9ef67b4..36caea2e 100644 --- a/spec/system/summarization/topic_summarization_spec.rb +++ b/spec/system/summarization/topic_summarization_spec.rb @@ -14,7 +14,7 @@ RSpec.describe "Summarize a topic ", type: :system do end let(:summarization_result) { "This is a summary" } let(:topic_page) { PageObjects::Pages::Topic.new } - let(:summary_box) { PageObjects::Components::AiSummaryBox.new } + let(:summary_box) { PageObjects::Components::AiSummaryTrigger.new } before do group.add(current_user) diff --git a/test/javascripts/acceptance/topic-summary-test.js b/test/javascripts/acceptance/topic-summary-test.js index 9f9195c0..d9e5dd29 100644 --- a/test/javascripts/acceptance/topic-summary-test.js +++ b/test/javascripts/acceptance/topic-summary-test.js @@ -22,7 +22,12 @@ acceptance("Topic - Summary", function (needs) { }); server.get("/discourse-ai/summarization/t/1", () => { - return helper.response({}); + return helper.response({ + ai_topic_summary: { + summarized_text: "This a", + }, + done: false, + }); }); }); @@ -39,7 +44,7 @@ acceptance("Topic - Summary", function (needs) { ai_topic_summary: { summarized_text: partialSummary }, }); - await click(".ai-topic-summarization"); + await click(".ai-summarization-button"); assert .dom(".ai-summary-box .generated-summary p") @@ -63,7 +68,7 @@ acceptance("Topic - Summary", function (needs) { .hasText(finalSummary, "Updates the summary with a final result"); assert - .dom(".ai-summary-box .summarized-on") + .dom(".ai-summary-modal .summarized-on") .exists("summary metadata exists"); }); @@ -76,7 +81,7 @@ acceptance("Topic - Summary", function (needs) { ai_topic_summary: { summarized_text: partialSummary }, }); - await click(".ai-topic-summarization"); + await click(".ai-summarization-button"); const finalSummaryCooked = "In this post, bianca said some stuff."; const finalSummaryResult = "In this post, bianca said some stuff."; @@ -128,20 +133,20 @@ acceptance("Topic - Summary - Anon", function (needs) { test("displays cached summary immediately", async function (assert) { await visit("/t/-/1"); - await click(".ai-topic-summarization"); + await click(".ai-summarization-button"); assert .dom(".ai-summary-box .generated-summary p") .hasText(finalSummary, "Updates the summary with the result"); assert - .dom(".ai-summary-box .summarized-on") + .dom(".ai-summary-modal .summarized-on") .exists("summary metadata exists"); }); test("clicking outside of summary should not close the summary box", async function (assert) { await visit("/t/-/1"); - await click(".ai-topic-summarization"); + await click(".ai-summarization-button"); await click("#main-outlet-wrapper"); assert.dom(".ai-summary-box").exists(); });