From f566ef7d8b35f4df11864d1cbbc560639e6e22ce Mon Sep 17 00:00:00 2001 From: Gert Hengeveld Date: Mon, 4 Sep 2023 16:51:23 +0200 Subject: [PATCH 1/5] Don't show menu when there's only one viewport or browser --- src/components/BrowserSelector.tsx | 59 ++++++++++++------ src/components/ViewportSelector.tsx | 60 +++++++++++++------ .../VisualTests/SnapshotComparison.tsx | 36 ++++------- 3 files changed, 97 insertions(+), 58 deletions(-) diff --git a/src/components/BrowserSelector.tsx b/src/components/BrowserSelector.tsx index ba1ea10e..b1048645 100644 --- a/src/components/BrowserSelector.tsx +++ b/src/components/BrowserSelector.tsx @@ -1,3 +1,5 @@ +import { TooltipNote, WithTooltip } from "@storybook/components"; +import { styled } from "@storybook/theming"; import React from "react"; import { Browser, BrowserInfo, ComparisonResult } from "../gql/graphql"; @@ -17,6 +19,14 @@ const browserIcons = { [Browser.Edge]: , } as const; +const IconWrapper = styled.div({ + height: 16, + margin: "6px 7px", + svg: { + verticalAlign: "top", + }, +}); + type BrowserData = Pick; interface BrowserSelectorProps { @@ -32,28 +42,43 @@ export const BrowserSelector = ({ browserResults, onSelectBrowser, }: BrowserSelectorProps) => { - const links = browserResults - .filter(({ browser }) => browser.key in browserIcons) - .map(({ browser, result }) => ({ - active: selectedBrowser === browser, - id: browser.id, - onClick: () => onSelectBrowser(browser), - right: !isAccepted && result !== ComparisonResult.Equal && , - title: browser.name, - })); - const aggregate = aggregateResult(browserResults.map(({ result }) => result)); if (!aggregate) return null; - const icon = browserIcons[selectedBrowser.key]; + let icon = browserIcons[selectedBrowser.key]; + if (!isAccepted && aggregate !== ComparisonResult.Equal) { + icon = {icon}; + } + + const links = browserResults.map(({ browser, result }) => ({ + active: selectedBrowser === browser, + id: browser.id, + onClick: () => onSelectBrowser(browser), + right: !isAccepted && result !== ComparisonResult.Equal && , + title: browser.name, + })); + return ( - - {isAccepted || aggregate === ComparisonResult.Equal ? ( - icon + + } + > + {links.length === 1 ? ( + {icon} ) : ( - {icon} + + {icon} + + )} - - + ); }; diff --git a/src/components/ViewportSelector.tsx b/src/components/ViewportSelector.tsx index 95115295..4ed67a98 100644 --- a/src/components/ViewportSelector.tsx +++ b/src/components/ViewportSelector.tsx @@ -1,4 +1,6 @@ +import { TooltipNote, WithTooltip } from "@storybook/components"; import { Icon } from "@storybook/design-system"; +import { styled } from "@storybook/theming"; import React from "react"; import { ComparisonResult, ViewportInfo } from "../gql/graphql"; @@ -7,6 +9,15 @@ import { ArrowIcon } from "./icons/ArrowIcon"; import { StatusDot, StatusDotWrapper } from "./StatusDot"; import { TooltipMenu } from "./TooltipMenu"; +const IconWrapper = styled.div(({ theme }) => ({ + height: 14, + margin: "7px 7px", + color: `${theme.color.defaultText}99`, + svg: { + verticalAlign: "top", + }, +})); + type ViewportData = Pick; interface ViewportSelectorProps { @@ -25,26 +36,41 @@ export const ViewportSelector = ({ const aggregate = aggregateResult(viewportResults.map(({ result }) => result)); if (!aggregate) return null; + let icon = ; + if (!isAccepted && aggregate !== ComparisonResult.Equal) { + icon = {icon}; + } + + const links = viewportResults.map(({ viewport, result }) => ({ + id: viewport.id, + title: viewport.name, + right: !isAccepted && result !== ComparisonResult.Equal && , + onClick: () => onSelectViewport(viewport), + active: selectedViewport === viewport, + })); + return ( - ({ - id: viewport.id, - title: viewport.name, - right: !isAccepted && result !== ComparisonResult.Equal && , - onClick: () => onSelectViewport(viewport), - active: selectedViewport === viewport, - }))} + + } > - {isAccepted || aggregate === ComparisonResult.Equal ? ( - + {links.length === 1 ? ( + {icon} ) : ( - - - + + {icon} + {selectedViewport.name} + + )} - {selectedViewport.name} - - + ); }; diff --git a/src/screens/VisualTests/SnapshotComparison.tsx b/src/screens/VisualTests/SnapshotComparison.tsx index d7f38ecc..6c2e7d12 100644 --- a/src/screens/VisualTests/SnapshotComparison.tsx +++ b/src/screens/VisualTests/SnapshotComparison.tsx @@ -58,34 +58,22 @@ export const SnapshotComparison = ({ {viewportResults.length > 0 && ( - } - trigger="hover" - hasChrome={false} - > - - + )} {browserResults.length > 0 && ( - } - trigger="hover" - hasChrome={false} - > - - + )} {selectedComparison?.result === ComparisonResult.Changed && ( From 98647b6e3b48a7143532b2e6b0f1ebd6433391e4 Mon Sep 17 00:00:00 2001 From: Gert Hengeveld Date: Mon, 4 Sep 2023 17:06:26 +0200 Subject: [PATCH 2/5] Rename 'viewport' to 'mode' and update the icon --- ...portSelector.stories.ts => ModeSelector.stories.ts} | 6 +++--- .../{ViewportSelector.tsx => ModeSelector.tsx} | 10 +++++----- src/screens/VisualTests/SnapshotComparison.tsx | 4 ++-- 3 files changed, 10 insertions(+), 10 deletions(-) rename src/components/{ViewportSelector.stories.ts => ModeSelector.stories.ts} (94%) rename src/components/{ViewportSelector.tsx => ModeSelector.tsx} (89%) diff --git a/src/components/ViewportSelector.stories.ts b/src/components/ModeSelector.stories.ts similarity index 94% rename from src/components/ViewportSelector.stories.ts rename to src/components/ModeSelector.stories.ts index 31c77066..e26f8546 100644 --- a/src/components/ViewportSelector.stories.ts +++ b/src/components/ModeSelector.stories.ts @@ -2,7 +2,7 @@ import { action } from "@storybook/addon-actions"; import type { Meta, StoryObj } from "@storybook/react"; import { ComparisonResult } from "../gql/graphql"; -import { ViewportSelector } from "./ViewportSelector"; +import { ModeSelector } from "./ModeSelector"; const viewport800Px = { id: "_800", @@ -14,12 +14,12 @@ const viewport1200Px = { }; const meta = { - component: ViewportSelector, + component: ModeSelector, args: { isAccepted: false, onSelectViewport: action("onSelectViewport"), }, -} satisfies Meta; +} satisfies Meta; export default meta; type Story = StoryObj; diff --git a/src/components/ViewportSelector.tsx b/src/components/ModeSelector.tsx similarity index 89% rename from src/components/ViewportSelector.tsx rename to src/components/ModeSelector.tsx index 4ed67a98..c6aab0a2 100644 --- a/src/components/ViewportSelector.tsx +++ b/src/components/ModeSelector.tsx @@ -20,23 +20,23 @@ const IconWrapper = styled.div(({ theme }) => ({ type ViewportData = Pick; -interface ViewportSelectorProps { +interface ModeSelectorProps { isAccepted: boolean; selectedViewport: ViewportData; onSelectViewport: (viewport: ViewportData) => void; viewportResults: { viewport: ViewportData; result: ComparisonResult }[]; } -export const ViewportSelector = ({ +export const ModeSelector = ({ isAccepted, selectedViewport, viewportResults, onSelectViewport, -}: ViewportSelectorProps) => { +}: ModeSelectorProps) => { const aggregate = aggregateResult(viewportResults.map(({ result }) => result)); if (!aggregate) return null; - let icon = ; + let icon = ; if (!isAccepted && aggregate !== ComparisonResult.Equal) { icon = {icon}; } @@ -57,7 +57,7 @@ export const ViewportSelector = ({ tooltip={ } diff --git a/src/screens/VisualTests/SnapshotComparison.tsx b/src/screens/VisualTests/SnapshotComparison.tsx index 6c2e7d12..cdbc1b7e 100644 --- a/src/screens/VisualTests/SnapshotComparison.tsx +++ b/src/screens/VisualTests/SnapshotComparison.tsx @@ -6,10 +6,10 @@ import { BrowserSelector } from "../../components/BrowserSelector"; import { IconButton } from "../../components/IconButton"; import { ProgressIcon } from "../../components/icons/ProgressIcon"; import { Bar, Col } from "../../components/layout"; +import { ModeSelector } from "../../components/ModeSelector"; import { Placeholder } from "../../components/Placeholder"; import { SnapshotImage } from "../../components/SnapshotImage"; import { TooltipMenu } from "../../components/TooltipMenu"; -import { ViewportSelector } from "../../components/ViewportSelector"; import { ComparisonResult, ReviewTestBatch, @@ -58,7 +58,7 @@ export const SnapshotComparison = ({ {viewportResults.length > 0 && ( - Date: Mon, 4 Sep 2023 19:33:00 +0200 Subject: [PATCH 3/5] Update component API to new naming --- src/components/ModeSelector.stories.ts | 30 +++++++++---------- src/components/ModeSelector.tsx | 28 ++++++++--------- .../VisualTests/SnapshotComparison.tsx | 13 ++++---- src/screens/VisualTests/StoryInfo.tsx | 8 ++--- src/utils/summarizeTests.test.ts | 6 ++-- src/utils/summarizeTests.ts | 4 +-- src/utils/useTests.ts | 10 +++---- 7 files changed, 48 insertions(+), 51 deletions(-) diff --git a/src/components/ModeSelector.stories.ts b/src/components/ModeSelector.stories.ts index e26f8546..eb38dd32 100644 --- a/src/components/ModeSelector.stories.ts +++ b/src/components/ModeSelector.stories.ts @@ -17,7 +17,7 @@ const meta = { component: ModeSelector, args: { isAccepted: false, - onSelectViewport: action("onSelectViewport"), + onSelectMode: action("onSelectMode"), }, } satisfies Meta; @@ -26,8 +26,8 @@ type Story = StoryObj; export const WithSingleViewportChanged: Story = { args: { - selectedViewport: viewport1200Px, - viewportResults: [ + selectedMode: viewport1200Px, + modeResults: [ { viewport: viewport1200Px, result: ComparisonResult.Changed, @@ -39,8 +39,8 @@ export const WithSingleViewportChanged: Story = { export const WithSingleViewportAccepted: Story = { args: { isAccepted: true, - selectedViewport: viewport1200Px, - viewportResults: [ + selectedMode: viewport1200Px, + modeResults: [ { viewport: viewport1200Px, result: ComparisonResult.Changed, @@ -51,8 +51,8 @@ export const WithSingleViewportAccepted: Story = { export const WithSingleViewportEqual: Story = { args: { - selectedViewport: viewport1200Px, - viewportResults: [ + selectedMode: viewport1200Px, + modeResults: [ { viewport: viewport1200Px, result: ComparisonResult.Equal, @@ -63,8 +63,8 @@ export const WithSingleViewportEqual: Story = { export const WithSingleViewportError: Story = { args: { - selectedViewport: viewport1200Px, - viewportResults: [ + selectedMode: viewport1200Px, + modeResults: [ { viewport: viewport1200Px, result: ComparisonResult.CaptureError, @@ -75,8 +75,8 @@ export const WithSingleViewportError: Story = { export const WithManyViewportsEqual: Story = { args: { - selectedViewport: viewport800Px, - viewportResults: [ + selectedMode: viewport800Px, + modeResults: [ { viewport: viewport800Px, result: ComparisonResult.Equal, @@ -91,8 +91,8 @@ export const WithManyViewportsEqual: Story = { export const WithManyViewportsSecondSelected: Story = { args: { - selectedViewport: viewport1200Px, - viewportResults: [ + selectedMode: viewport1200Px, + modeResults: [ { viewport: viewport800Px, result: ComparisonResult.Equal, @@ -107,8 +107,8 @@ export const WithManyViewportsSecondSelected: Story = { export const WithManyViewportsVaried: Story = { args: { - selectedViewport: viewport800Px, - viewportResults: [ + selectedMode: viewport800Px, + modeResults: [ { viewport: viewport800Px, result: ComparisonResult.Equal, diff --git a/src/components/ModeSelector.tsx b/src/components/ModeSelector.tsx index c6aab0a2..4d0d28eb 100644 --- a/src/components/ModeSelector.tsx +++ b/src/components/ModeSelector.tsx @@ -18,22 +18,22 @@ const IconWrapper = styled.div(({ theme }) => ({ }, })); -type ViewportData = Pick; +type ModeData = Pick; interface ModeSelectorProps { isAccepted: boolean; - selectedViewport: ViewportData; - onSelectViewport: (viewport: ViewportData) => void; - viewportResults: { viewport: ViewportData; result: ComparisonResult }[]; + selectedMode: ModeData; + onSelectMode: (viewport: ModeData) => void; + modeResults: { viewport: ModeData; result: ComparisonResult }[]; } export const ModeSelector = ({ isAccepted, - selectedViewport, - viewportResults, - onSelectViewport, + selectedMode, + modeResults, + onSelectMode, }: ModeSelectorProps) => { - const aggregate = aggregateResult(viewportResults.map(({ result }) => result)); + const aggregate = aggregateResult(modeResults.map(({ result }) => result)); if (!aggregate) return null; let icon = ; @@ -41,12 +41,12 @@ export const ModeSelector = ({ icon = {icon}; } - const links = viewportResults.map(({ viewport, result }) => ({ + const links = modeResults.map(({ viewport, result }) => ({ id: viewport.id, title: viewport.name, right: !isAccepted && result !== ComparisonResult.Equal && , - onClick: () => onSelectViewport(viewport), - active: selectedViewport === viewport, + onClick: () => onSelectMode(viewport), + active: selectedMode === viewport, })); return ( @@ -56,9 +56,7 @@ export const ModeSelector = ({ trigger="hover" tooltip={ } > @@ -67,7 +65,7 @@ export const ModeSelector = ({ ) : ( {icon} - {selectedViewport.name} + {selectedMode.name} )} diff --git a/src/screens/VisualTests/SnapshotComparison.tsx b/src/screens/VisualTests/SnapshotComparison.tsx index cdbc1b7e..81f65e0c 100644 --- a/src/screens/VisualTests/SnapshotComparison.tsx +++ b/src/screens/VisualTests/SnapshotComparison.tsx @@ -41,9 +41,8 @@ export const SnapshotComparison = ({ const [diffVisible, setDiffVisible] = useState(true); const [focusVisible, setFocusVisible] = useState(false); - const { selectedTest, selectedComparison, onSelectBrowser, onSelectViewport } = useTests(tests); - const { status, isInProgress, changeCount, browserResults, viewportResults } = - summarizeTests(tests); + const { selectedTest, selectedComparison, onSelectBrowser, onSelectMode } = useTests(tests); + const { status, isInProgress, changeCount, browserResults, modeResults } = summarizeTests(tests); return ( <> @@ -56,13 +55,13 @@ export const SnapshotComparison = ({ ) : ( - {viewportResults.length > 0 && ( + {modeResults.length > 0 && ( )} diff --git a/src/screens/VisualTests/StoryInfo.tsx b/src/screens/VisualTests/StoryInfo.tsx index 63f579f8..a0b72ae9 100644 --- a/src/screens/VisualTests/StoryInfo.tsx +++ b/src/screens/VisualTests/StoryInfo.tsx @@ -35,7 +35,7 @@ export const StoryInfo = ({ isBuildFailed, }: StoryInfoSectionProps) => { // isInProgress means we have tests but they are still unfinished - const { status, isInProgress, changeCount, brokenCount, viewportResults, browserResults } = + const { status, isInProgress, changeCount, brokenCount, modeResults, browserResults } = summarizeTests(tests ?? []); const startedAgo = @@ -105,14 +105,14 @@ export const StoryInfo = ({ />
- {viewportResults.length > 0 && ( + {modeResults.length > 0 && ( - {pluralize("viewport", viewportResults.length, true)} + {pluralize("mode", modeResults.length, true)} {", "} {pluralize("browser", browserResults.length, true)} )} - {viewportResults.length > 0 && " • "} + {modeResults.length > 0 && " • "} {isInProgress && Test in progress...} {!isInProgress && {startedAgo}} diff --git a/src/utils/summarizeTests.test.ts b/src/utils/summarizeTests.test.ts index 7a07885f..bf19fc64 100644 --- a/src/utils/summarizeTests.test.ts +++ b/src/utils/summarizeTests.test.ts @@ -51,7 +51,7 @@ const tests = [ ]; it("Calculates static information correctly", () => { - const { status, isInProgress, changeCount, brokenCount, browserResults, viewportResults } = + const { status, isInProgress, changeCount, brokenCount, browserResults, modeResults } = summarizeTests(tests); expect({ @@ -60,7 +60,7 @@ it("Calculates static information correctly", () => { changeCount, brokenCount, browserResults, - viewportResults, + modeResults, }).toMatchInlineSnapshot(` { "brokenCount": 1, @@ -87,7 +87,7 @@ it("Calculates static information correctly", () => { "changeCount": 1, "isInProgress": true, "status": "IN_PROGRESS", - "viewportResults": [ + "modeResults": [ { "result": "EQUAL", "viewport": { diff --git a/src/utils/summarizeTests.ts b/src/utils/summarizeTests.ts index 4669a95d..0ecdd1a5 100644 --- a/src/utils/summarizeTests.ts +++ b/src/utils/summarizeTests.ts @@ -82,7 +82,7 @@ export function summarizeTests(tests: StoryTestFieldsFragment[]) { browser: browserInfoById[id], result, })); - const viewportResults = Object.entries(resultsByViewport).map(([id, result]) => ({ + const modeResults = Object.entries(resultsByViewport).map(([id, result]) => ({ viewport: viewportInfoById[id], result, })); @@ -93,6 +93,6 @@ export function summarizeTests(tests: StoryTestFieldsFragment[]) { changeCount, brokenCount, browserResults, - viewportResults, + modeResults, }; } diff --git a/src/utils/useTests.ts b/src/utils/useTests.ts index 61e02d5e..c04c4811 100644 --- a/src/utils/useTests.ts +++ b/src/utils/useTests.ts @@ -2,8 +2,8 @@ import { useCallback, useState } from "react"; import { BrowserInfo, StoryTestFieldsFragment, ViewportInfo } from "../gql/graphql"; -type ViewportData = Pick; type BrowserData = Pick; +type ModeData = Pick; /** * Pick a single test+comparison (by viewport+browser) from a set of tests @@ -13,15 +13,15 @@ export function useTests(tests: StoryTestFieldsFragment[]) { const [selectedBrowserId, onSelectBrowserId] = useState( tests[0].comparisons[0].browser.id ); - const [selectedViewportId, onSelectViewportId] = useState( + const [selectedModeId, onSelectModeId] = useState( tests[0].comparisons[0].viewport.id ); const onSelectBrowser = useCallback(({ id }: BrowserData) => onSelectBrowserId(id), []); - const onSelectViewport = useCallback(({ id }: ViewportData) => onSelectViewportId(id), []); + const onSelectMode = useCallback(({ id }: ModeData) => onSelectModeId(id), []); const selectedTest = - tests.find(({ parameters }) => parameters.viewport.id === selectedViewportId) || tests[0]; + tests.find(({ parameters }) => parameters.viewport.id === selectedModeId) || tests[0]; const selectedComparison = selectedTest.comparisons.find(({ browser }) => browser.id === selectedBrowserId) || @@ -31,6 +31,6 @@ export function useTests(tests: StoryTestFieldsFragment[]) { selectedTest, selectedComparison, onSelectBrowser, - onSelectViewport, + onSelectMode, }; } From a1814b13d6d2569cf0c86e74d989c0d01c5cab40 Mon Sep 17 00:00:00 2001 From: Gert Hengeveld Date: Tue, 5 Sep 2023 12:59:16 +0200 Subject: [PATCH 4/5] Avoid duplicate links length check --- src/components/BrowserSelector.tsx | 26 +++++++++++++------------- src/components/ModeSelector.tsx | 26 +++++++++++++------------- 2 files changed, 26 insertions(+), 26 deletions(-) diff --git a/src/components/BrowserSelector.tsx b/src/components/BrowserSelector.tsx index b1048645..d425a5cb 100644 --- a/src/components/BrowserSelector.tsx +++ b/src/components/BrowserSelector.tsx @@ -50,13 +50,15 @@ export const BrowserSelector = ({ icon = {icon}; } - const links = browserResults.map(({ browser, result }) => ({ - active: selectedBrowser === browser, - id: browser.id, - onClick: () => onSelectBrowser(browser), - right: !isAccepted && result !== ComparisonResult.Equal && , - title: browser.name, - })); + const links = + browserResults.length > 1 && + browserResults.map(({ browser, result }) => ({ + active: selectedBrowser === browser, + id: browser.id, + onClick: () => onSelectBrowser(browser), + right: !isAccepted && result !== ComparisonResult.Equal && , + title: browser.name, + })); return ( } > - {links.length === 1 ? ( - {icon} - ) : ( + {links ? ( {icon} + ) : ( + {icon} )} ); diff --git a/src/components/ModeSelector.tsx b/src/components/ModeSelector.tsx index 4d0d28eb..d7f92372 100644 --- a/src/components/ModeSelector.tsx +++ b/src/components/ModeSelector.tsx @@ -41,13 +41,15 @@ export const ModeSelector = ({ icon = {icon}; } - const links = modeResults.map(({ viewport, result }) => ({ - id: viewport.id, - title: viewport.name, - right: !isAccepted && result !== ComparisonResult.Equal && , - onClick: () => onSelectMode(viewport), - active: selectedMode === viewport, - })); + const links = + modeResults.length > 1 && + modeResults.map(({ viewport, result }) => ({ + id: viewport.id, + title: viewport.name, + right: !isAccepted && result !== ComparisonResult.Equal && , + onClick: () => onSelectMode(viewport), + active: selectedMode === viewport, + })); return ( + } > - {links.length === 1 ? ( - {icon} - ) : ( + {links ? ( {icon} {selectedMode.name} + ) : ( + {icon} )} ); From 51f5eed1f1390cda18f614ba72773110944c3394 Mon Sep 17 00:00:00 2001 From: Gert Hengeveld Date: Tue, 5 Sep 2023 13:00:20 +0200 Subject: [PATCH 5/5] Update snapshot --- src/utils/summarizeTests.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/utils/summarizeTests.test.ts b/src/utils/summarizeTests.test.ts index bf19fc64..ea1adbae 100644 --- a/src/utils/summarizeTests.test.ts +++ b/src/utils/summarizeTests.test.ts @@ -86,7 +86,6 @@ it("Calculates static information correctly", () => { ], "changeCount": 1, "isInProgress": true, - "status": "IN_PROGRESS", "modeResults": [ { "result": "EQUAL", @@ -125,6 +124,7 @@ it("Calculates static information correctly", () => { }, }, ], + "status": "IN_PROGRESS", } `); });