From 7710be0f6e6d64dada453f6ba905a2564e1c32d2 Mon Sep 17 00:00:00 2001
From: Emma Segal-Grossman <hi@emma.cafe>
Date: Mon, 22 Apr 2024 20:02:07 -0400
Subject: [PATCH] More Frontend QA Polish Changes (#1709)

Sort of relevant to #1696

- Improves a number of layout elements at smaller viewport sizes
(specifically, letting button groups wrap onto next lines & ensure
titles can't shrink to 0 width)
- Adds "No page title" to places where there'd normally be a page title
but isn't
- Applies grid styling to page list area to fix overflow issues
- When selecting or loading with a page selected that's farther down on
the page list, the top of the page header could be scrolled away from,
and there'd be no way to scroll back because the area had `overflow:
hidden` applied
- Adds default width and height to image comparer element, so that it
displays correctly when images haven't loaded and doesn't change layout
when images load

---------

Co-authored-by: Henry Wilkinson <henry@wilkinson.graphics>
---
 frontend/src/features/qa/page-list/ui/page.ts |  5 +-
 .../org/archived-item-qa/archived-item-qa.ts  | 38 ++++++++-----
 .../src/pages/org/archived-item-qa/styles.ts  | 10 ++--
 .../org/archived-item-qa/ui/resources.ts      | 51 ++++++++++--------
 .../org/archived-item-qa/ui/screenshots.ts    | 53 +++++++++++--------
 .../src/pages/org/archived-item-qa/ui/text.ts |  2 +-
 6 files changed, 94 insertions(+), 65 deletions(-)

diff --git a/frontend/src/features/qa/page-list/ui/page.ts b/frontend/src/features/qa/page-list/ui/page.ts
index 7460419f..c5b48768 100644
--- a/frontend/src/features/qa/page-list/ui/page.ts
+++ b/frontend/src/features/qa/page-list/ui/page.ts
@@ -1,4 +1,4 @@
-import { localized } from "@lit/localize";
+import { localized, msg } from "@lit/localize";
 import type { SlTooltip } from "@shoelace-style/shoelace";
 import clsx from "clsx";
 import { html, type PropertyValues } from "lit";
@@ -174,7 +174,8 @@ export class QaPage extends TailwindElement {
             </div>
           </sl-tooltip>
           <h5 class="truncate text-sm font-semibold text-black">
-            ${page.title}
+            ${page.title ||
+            html`<span class="opacity-50">${msg("No page title")}</span>`}
           </h5>
           <div class="truncate text-xs leading-4 text-blue-600">
             ${page.url}
diff --git a/frontend/src/pages/org/archived-item-qa/archived-item-qa.ts b/frontend/src/pages/org/archived-item-qa/archived-item-qa.ts
index 2c2ad0d0..d9f44fe1 100644
--- a/frontend/src/pages/org/archived-item-qa/archived-item-qa.ts
+++ b/frontend/src/pages/org/archived-item-qa/archived-item-qa.ts
@@ -326,12 +326,14 @@ export class ArchivedItemQA extends TailwindElement {
     return html`
       ${this.renderHidden()}
 
-      <article class="grid gap-x-6 gap-y-4 md:gap-y-0">
+      <article class="qa-grid grid gap-x-6 gap-y-0">
         <header
-          class="grid--header flex items-center justify-between gap-1 border-b py-2"
+          class="grid--header flex flex-wrap items-center justify-between gap-1 border-b py-2"
         >
           <div class="flex items-center gap-2 overflow-hidden">
-            <h1 class="flex-1 truncate text-base font-semibold leading-tight">
+            <h1
+              class="flex-1 flex-shrink-0 basis-32 truncate text-base font-semibold leading-tight"
+            >
               ${itemName}
             </h1>
             ${when(
@@ -351,7 +353,7 @@ export class ArchivedItemQA extends TailwindElement {
               `,
             )}
           </div>
-          <div>
+          <div class="ml-auto flex">
             <sl-button
               size="small"
               variant="text"
@@ -381,19 +383,23 @@ export class ArchivedItemQA extends TailwindElement {
         </header>
 
         <div
-          class="grid--pageToolbar flex items-center justify-between overflow-hidden border-b py-2"
+          class="grid--pageToolbar flex flex-wrap items-center justify-stretch gap-2 overflow-hidden border-b py-2 @container"
         >
           <h2
-            class="mr-4 truncate text-base font-semibold text-neutral-700"
-            title="${this.page ? this.page.title : nothing}"
+            class="flex-auto flex-shrink-0 flex-grow basis-32 truncate text-base font-semibold text-neutral-700"
+            title="${this.page?.title ?? ""}"
           >
-            ${this.page ? this.page.title || msg("no page title") : nothing}
+            ${this.page?.title ||
+            html`<span class="opacity-50">${msg("No page title")}</span>`}
           </h2>
-          <div class="flex gap-4">
+          <div
+            class="ml-auto flex flex-grow basis-auto flex-wrap justify-between gap-2 @lg:flex-grow-0"
+          >
             <sl-button
               size="small"
               @click=${this.navPrevPage}
               ?disabled=${!prevPage}
+              class="order-1"
             >
               <sl-icon slot="prefix" name="arrow-left"></sl-icon>
               ${msg("Previous Page")}
@@ -403,6 +409,7 @@ export class ArchivedItemQA extends TailwindElement {
                 "Approvals are temporarily disabled during analysis runs.",
               )}
               ?disabled=${!disableReview}
+              class="order-3 mx-auto flex w-full justify-center @lg:order-2 @lg:mx-0 @lg:w-auto"
             >
               <btrix-page-qa-approval
                 .authState=${this.authState}
@@ -420,6 +427,7 @@ export class ArchivedItemQA extends TailwindElement {
               ?disabled=${!nextPage}
               outline
               @click=${this.navNextPage}
+              class="order-2 @lg:order-3"
             >
               <sl-icon slot="suffix" name="arrow-right"></sl-icon>
               ${msg("Next Page")}
@@ -427,8 +435,10 @@ export class ArchivedItemQA extends TailwindElement {
           </div>
         </div>
 
-        <div class="grid--tabGroup flex flex-col">
-          <nav class="my-2 flex gap-2">
+        <div class="grid--tabGroup flex min-w-0 flex-col">
+          <nav
+            class="-mx-3 my-0 flex gap-2 overflow-x-auto px-3 py-2 lg:mx-0 lg:px-0"
+          >
             <btrix-navigation-button
               id="screenshot-tab"
               href=${`${crawlBaseUrl}/review/screenshots?${searchParams.toString()}`}
@@ -475,14 +485,16 @@ export class ArchivedItemQA extends TailwindElement {
           ${this.renderPanelToolbar()} ${this.renderPanel()}
         </div>
 
-        <section class="grid--pageList overflow-hidden">
+        <section
+          class="grid--pageList grid grid-rows-[auto_1fr] *:min-h-0 *:min-w-0"
+        >
           <h2
             class="my-4 text-base font-semibold leading-none text-neutral-800"
           >
             ${msg("Pages")}
           </h2>
           <btrix-qa-page-list
-            class="flex h-full flex-col"
+            class="flex flex-col"
             .qaRunId=${this.qaRunId}
             .itemPageId=${this.itemPageId}
             .pages=${this.pages}
diff --git a/frontend/src/pages/org/archived-item-qa/styles.ts b/frontend/src/pages/org/archived-item-qa/styles.ts
index d0514c3b..8811fdf1 100644
--- a/frontend/src/pages/org/archived-item-qa/styles.ts
+++ b/frontend/src/pages/org/archived-item-qa/styles.ts
@@ -1,25 +1,23 @@
 import { css } from "lit";
 
-import { TWO_COL_SCREEN_MIN_CSS } from "@/components/ui/tab-list";
-
 export const styles = css`
   article > * {
     min-height: 0;
   }
 
-  .grid {
+  .qa-grid {
     grid-template:
       "header"
       "pageToolbar"
-      "tabNav"
       "tabGroup"
       "pageList";
     grid-template-columns: 100%;
     grid-template-rows: repeat(5, max-content);
   }
 
-  @media only screen and (min-width: ${TWO_COL_SCREEN_MIN_CSS}) {
-    .grid {
+  /* Tailwind 'lg' responsive size */
+  @media only screen and (min-width: 1024px) {
+    .qa-grid {
       /* TODO calculate screen space instead of hardcoding */
       height: 100vh;
       /* overflow: hidden; */
diff --git a/frontend/src/pages/org/archived-item-qa/ui/resources.ts b/frontend/src/pages/org/archived-item-qa/ui/resources.ts
index 2d92bd27..b8f26c5e 100644
--- a/frontend/src/pages/org/archived-item-qa/ui/resources.ts
+++ b/frontend/src/pages/org/archived-item-qa/ui/resources.ts
@@ -1,4 +1,5 @@
 import { msg } from "@lit/localize";
+import clsx from "clsx";
 import { html } from "lit";
 
 import type { ReplayData, ResourcesPayload } from "../types";
@@ -22,28 +23,32 @@ function renderDiff(
   ];
   const rows = [
     [
-      html`<span class=${tw`font-semibold capitalize`}
+      html`<span class="font-semibold capitalize"
         >${msg("All Resources")}</span
       >`,
-      html`<span class=${tw`font-semibold`}
+      html`<span class="font-semibold"
         >${crawlResources[TOTAL].good.toLocaleString()}</span
       >`,
-      html`<span class=${tw`font-semibold`}
+      html`<span class="font-semibold"
         >${crawlResources[TOTAL].bad.toLocaleString()}</span
       >`,
       html`<span
-        class="${tw`font-semibold`} ${crawlResources[TOTAL].good !==
-        qaResources[TOTAL].good
-          ? tw`text-danger`
-          : tw`text-neutral-700`}"
+        class="${clsx(
+          "font-semibold",
+          crawlResources[TOTAL].good !== qaResources[TOTAL].good
+            ? "text-danger"
+            : "text-neutral-700",
+        )}"
       >
         ${qaResources[TOTAL].good.toLocaleString()}
       </span>`,
       html`<span
-        class="${tw`font-semibold`} ${crawlResources[TOTAL].bad !==
-        qaResources[TOTAL].bad
-          ? tw`text-danger`
-          : tw`text-neutral-700`}"
+        class="${clsx(
+          "font-semibold",
+          crawlResources[TOTAL].bad !== qaResources[TOTAL].bad
+            ? "text-danger"
+            : "text-neutral-700",
+        )}"
       >
         ${qaResources[TOTAL].bad.toLocaleString()}
       </span>`,
@@ -51,7 +56,7 @@ function renderDiff(
     ...Object.keys(qaResources)
       .filter((key) => key !== TOTAL)
       .map((key) => [
-        html`<span class=${tw`capitalize`}>${key}</span>`,
+        html`<span class="capitalize">${key}</span>`,
         html`${Object.prototype.hasOwnProperty.call(crawlResources, key)
           ? crawlResources[key].good.toLocaleString()
           : 0}`,
@@ -78,35 +83,39 @@ function renderDiff(
   ];
 
   return html`
-    <btrix-data-table .columns=${columns} .rows=${rows}></btrix-data-table>
+    <btrix-data-table
+      class="block"
+      .columns=${columns}
+      .rows=${rows}
+    ></btrix-data-table>
   `;
 }
 
 export function renderResources(crawlData: ReplayData, qaData: ReplayData) {
   const noData = html`<div
-    class=${tw`flex h-full flex-col items-center justify-center gap-2 text-xs text-neutral-500`}
+    class="m-4 flex flex-col items-center justify-center gap-2 text-xs text-neutral-500"
   >
     <sl-icon name="slash-circle"></sl-icon>
     ${msg("Resources data not available")}
   </div>`;
 
   return html`
-    <div class=${tw`flex h-full flex-col outline`}>
-      <div class=${tw`flex-1 overflow-auto overscroll-contain`}>
+    <div class="flex h-full flex-col outline">
+      <div class="flex-1 overflow-auto overscroll-contain">
         ${crawlData && qaData
           ? crawlData.resources && qaData.resources
             ? renderDiff(crawlData.resources, qaData.resources)
             : noData
           : renderSpinner()}
       </div>
-      <footer class=${tw`pt-2 text-xs text-neutral-600`}>
+      <footer class="pt-2 text-xs text-neutral-600">
         <dl>
-          <div class=${tw`flex gap-1`}>
-            <dt class=${tw`font-semibold`}>${msg("Good:")}</dt>
+          <div class="flex gap-1">
+            <dt class="font-semibold">${msg("Good:")}</dt>
             <dd>${msg("Success (2xx) and Redirection (3xx) status codes")}</dd>
           </div>
-          <div class=${tw`flex gap-1`}>
-            <dt class=${tw`font-semibold`}>${msg("Bad:")}</dt>
+          <div class="flex gap-1">
+            <dt class="font-semibold">${msg("Bad:")}</dt>
             <dd>
               ${msg("Client error (4xx) and Server error (5xx) status codes")}
             </dd>
diff --git a/frontend/src/pages/org/archived-item-qa/ui/screenshots.ts b/frontend/src/pages/org/archived-item-qa/ui/screenshots.ts
index 72887dcc..1ed3965f 100644
--- a/frontend/src/pages/org/archived-item-qa/ui/screenshots.ts
+++ b/frontend/src/pages/org/archived-item-qa/ui/screenshots.ts
@@ -1,4 +1,5 @@
 import { msg } from "@lit/localize";
+import clsx from "clsx";
 import { html } from "lit";
 import { guard } from "lit/directives/guard.js";
 import { when } from "lit/directives/when.js";
@@ -7,18 +8,24 @@ import type { ReplayData } from "../types";
 
 import { renderSpinner } from "./spinner";
 
-import { tw } from "@/utils/tailwind";
-
 function image(data: ReplayData) {
   if (!data?.blobUrl) {
     return html`<div
-      class=${tw`flex h-full w-full flex-col items-center justify-center gap-2 text-xs text-neutral-500`}
+      class="flex h-full w-full flex-col items-center justify-center gap-2 text-xs text-neutral-500"
     >
       <sl-icon name="slash-circle"></sl-icon>
       ${msg("Screenshot not available")}
     </div>`;
   }
-  return html` <img class=${tw`h-full w-full`} src=${data.blobUrl} /> `;
+  return html`
+    <img
+      class="h-full w-full"
+      width="1920"
+      height="1080"
+      alt=""
+      src=${data.blobUrl}
+    />
+  `;
 }
 
 export function renderScreenshots(
@@ -27,30 +34,36 @@ export function renderScreenshots(
   splitView: boolean,
 ) {
   const content = html`
-    <div class=${tw`flex${splitView ? "" : tw` justify-between`}`}>
+    <div class=${clsx("flex", !splitView && "justify-between")}>
       <h3
         id="crawlScreenshotHeading"
-        class=${tw`mb-2 font-semibold ${splitView ? tw`flex-1` : "flex-grow-0"}`}
+        class=${clsx(
+          "mb-2 font-semibold",
+          splitView ? "flex-1" : "flex-grow-0",
+        )}
       >
         ${msg("Screenshot during crawl")}
       </h3>
       <h3
         id="qaScreenshotHeading"
-        class=${tw`mb-2 font-semibold ${splitView ? tw`flex-1` : "flex-grow-0"}`}
+        class=${clsx(
+          "mb-2 font-semibold",
+          splitView ? "flex-1" : "flex-grow-0",
+        )}
       >
         ${msg("Screenshot from replay")}
       </h3>
     </div>
     ${splitView
-      ? html` <div class=${tw`flex flex-col gap-2 md:flex-row`}>
+      ? html` <div class="flex flex-col gap-2 md:flex-row">
           <div
-            class=${tw`aspect-video flex-1 overflow-hidden rounded-lg border bg-slate-50`}
+            class="aspect-video flex-1 overflow-hidden rounded-lg border bg-slate-50"
             aria-labelledby="crawlScreenshotHeading"
           >
             ${when(crawlData, image, renderSpinner)}
           </div>
           <div
-            class=${tw`aspect-video flex-1 overflow-hidden rounded-lg border bg-slate-50`}
+            class="aspect-video flex-1 overflow-hidden rounded-lg border bg-slate-50"
             aria-labelledby="qaScreenshotHeading"
           >
             ${when(qaData, image, renderSpinner)}
@@ -58,19 +71,15 @@ export function renderScreenshots(
         </div>`
       : html`
           <div
-            class=${tw`aspect-video overflow-hidden rounded-lg border bg-slate-50`}
+            class="aspect-video overflow-hidden rounded-lg border bg-slate-50"
           >
-            <sl-image-comparer>
-              <img
-                slot="after"
-                src="${crawlData?.blobUrl || ""}"
-                aria-labelledby="crawlScreenshotHeading"
-              />
-              <img
-                slot="before"
-                src="${qaData?.blobUrl || ""}"
-                aria-labelledby="qaScreenshotHeading"
-              />
+            <sl-image-comparer class="h-full w-full">
+              <div slot="after" aria-labelledby="crawlScreenshotHeading">
+                ${when(crawlData, image, renderSpinner)}
+              </div>
+              <div slot="before" aria-labelledby="qaScreenshotHeading">
+                ${when(qaData, image, renderSpinner)}
+              </div>
             </sl-image-comparer>
           </div>
         `}
diff --git a/frontend/src/pages/org/archived-item-qa/ui/text.ts b/frontend/src/pages/org/archived-item-qa/ui/text.ts
index 0c5d2980..99868744 100644
--- a/frontend/src/pages/org/archived-item-qa/ui/text.ts
+++ b/frontend/src/pages/org/archived-item-qa/ui/text.ts
@@ -65,7 +65,7 @@ function renderDiff(
 
 export function renderText(crawlData: ReplayData, qaData: ReplayData) {
   const noData = html`<div
-    class=${tw`flex flex-col items-center justify-center gap-2 text-xs text-neutral-500`}
+    class="m-4 flex flex-col items-center justify-center gap-2 text-xs text-neutral-500"
   >
     <sl-icon name="slash-circle"></sl-icon>
     ${msg("Text data not available")}