UX: move topic summary from DMenu to DModal (#992)

Co-authored-by: Keegan George <kgeorge13@gmail.com>
2024-12-03 13:30:15 -05:00 · 2024-12-03 13:30:15 -05:00 · 8203bdfbc9
parent ce6a2eca21
commit 8203bdfbc9
9 changed files with 171 additions and 178 deletions
--- a/assets/javascripts/discourse/connectors/topic-map-expanded-after/ai-summary-box.gjs
+++ b/assets/javascripts/discourse/connectors/topic-map-expanded-after/ai-summary-box.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();
  }

  <template>
-    {{#if @outletArgs.topic.summarizable}}
-      <div
-        class="ai-summarization-button"
-        {{didInsert this.subscribe}}
-        {{didUpdate this.subscribe @outletArgs.topic.id}}
-        {{willDestroy this.unsubscribe}}
-      >
-        <DMenu
-          @onShow={{this.generateSummary}}
-          @arrow={{false}}
-          @identifier="topic-map__ai-summary"
-          @onRegisterApi={{this.onRegisterApi}}
-          @interactive={{true}}
-          @triggers="click"
-          @placement="left"
-          @modalForMobile={{true}}
-          @groupIdentifier="topic-map"
-          @inline={{true}}
-          @label={{i18n "summary.buttons.generate"}}
-          @title={{i18n "summary.buttons.generate"}}
-          @icon="discourse-sparkles"
-          @triggerClass="ai-topic-summarization"
-          @closeOnClickOutside={{false}}
-        >
-          <:content>
-            <div class="ai-summary-container">
-              <header class="ai-summary__header">
-                <h3>{{i18n "discourse_ai.summarization.topic.title"}}</h3>
-                {{#if this.site.desktopView}}
-                  <DButton
-                    @title="discourse_ai.summarization.topic.close"
-                    @action={{this.onClose}}
-                    @icon="times"
-                    class="btn-transparent ai-summary__close"
-                  />
-                {{/if}}
-              </header>
-
-              <article
-                class={{concatClass
-                  "ai-summary-box"
-                  "streamable-content"
-                  (if this.isStreaming "streaming")
-                }}
-              >
-                {{#if this.loading}}
-                  <AiSummarySkeleton />
-                {{else}}
-                  <div class="generated-summary cooked">
-                    <CookText @rawText={{this.streamedText}} />
-                  </div>
-                  {{#if this.summarizedOn}}
-                    <div class="summarized-on">
-                      <p>
-                        {{i18n "summary.summarized_on" date=this.summarizedOn}}
-                        <DTooltip @placements={{array "top-end"}}>
-                          <:trigger>
-                            {{dIcon "info-circle"}}
-                          </:trigger>
-                          <:content>
-                            {{i18n
-                              "summary.model_used"
-                              model=this.summarizedBy
-                            }}
-                          </:content>
-                        </DTooltip>
-                      </p>
-                      <div class="outdated-summary">
-                        {{#if this.outdated}}
-                          <p>{{this.outdatedSummaryWarningText}}</p>
-                        {{/if}}
-                        {{#if this.canRegenerate}}
-                          <DButton
-                            @label="summary.buttons.regenerate"
-                            @title="summary.buttons.regenerate"
-                            @action={{this.regenerateSummary}}
-                            @icon="sync"
-                          />
-                        {{/if}}
-                      </div>
-                    </div>
-                  {{/if}}
-                {{/if}}
-              </article>
-            </div>
-          </:content>
-        </DMenu>
-      </div>
-    {{/if}}
+    <DModal
+      @title={{i18n "discourse_ai.summarization.topic.title"}}
+      @closeModal={{this.handleClose}}
+      @bodyClass="ai-summary-modal__body"
+      class="ai-summary-modal"
+      {{didInsert this.subscribe @model.topic.id}}
+      {{didUpdate this.subscribe @model.topic.id}}
+      {{willDestroy this.unsubscribe}}
+      @hideFooter={{not this.summarizedOn}}
+    >
+      <:body>
+        {{htmlClass "scrollable-modal"}}
+        <div class="ai-summary-container" {{didInsert this.generateSummary}}>
+          <article
+            class={{concatClass
+              "ai-summary-box"
+              "streamable-content"
+              (if this.isStreaming "streaming")
+            }}
+          >
+            {{#if this.loading}}
+              <AiSummarySkeleton />
+            {{else}}
+              <div class="generated-summary cooked">
+                <CookText @rawText={{this.streamedText}} />
+              </div>
+            {{/if}}
+          </article>
+        </div>
+      </:body>
+      <:footer>
+        <p class="summarized-on">
+          {{i18n "summary.summarized_on" date=this.summarizedOn}}
+          <DTooltip @placements={{array "top-end"}}>
+            <:trigger>
+              {{dIcon "circle-info"}}
+            </:trigger>
+            <:content>
+              {{i18n "summary.model_used" model=this.summarizedBy}}
+            </:content>
+          </DTooltip>
+        </p>
+        {{#if this.outdated}}
+          <p class="summary-outdated">{{this.outdatedSummaryWarningText}}</p>
+        {{/if}}
+        {{#if this.canRegenerate}}
+          <DButton
+            @label="summary.buttons.regenerate"
+            @title="summary.buttons.regenerate"
+            @action={{this.regenerateSummary}}
+            @icon="sync"
+          />
+        {{/if}}
+      </:footer>
+    </DModal>
  </template>
 }
--- 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
@ -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,
+      },
+    });
+  }
+
+  <template>
+    {{#if @outletArgs.topic.summarizable}}
+      <DButton
+        @label="summary.buttons.generate"
+        @icon="discourse-sparkles"
+        @action={{this.openAiSummaryModal}}
+        class="ai-summarization-button"
+      />
+    {{/if}}
+  </template>
+}
--- 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 {
--- 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;
+}
--- a/assets/stylesheets/modules/summarization/mobile/ai-summary.scss
+++ b/assets/stylesheets/modules/summarization/mobile/ai-summary.scss
@ -1,3 +0,0 @@
-.ai-summary-box {
-  padding: 0.75em 1rem;
-}
--- 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

--- 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)
--- 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)
--- 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,  <a href='/t/-/1/1'>bianca</a> 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();
  });