From 326deb366db0f4cffa7b225f735e8917f483d06b Mon Sep 17 00:00:00 2001 From: Brian Holmes <120223836+briangregoryholmes@users.noreply.github.com> Date: Thu, 19 Sep 2024 21:01:38 -0400 Subject: [PATCH] refactor: measure menu (#5741) * init commit * test fix * test fix --- web-common/src/components/menu/README.md | 13 -- .../menu/compositions/SelectMenu.svelte | 61 ------- .../components/menu/core/MenuHeader.svelte | 13 -- web-common/src/components/menu/index.ts | 7 +- .../components/menu/shadcn/SelectMenu.svelte | 42 ----- .../menu/shadcn/SelectMenuContent.svelte | 47 ------ .../menu/triggers/SelectButton.svelte | 66 -------- web-common/src/components/menu/types.ts | 14 -- .../menu/wrappers/SimpleActionMenu.svelte | 66 -------- .../menu/wrappers/WithSelectMenu.svelte | 153 ------------------ .../src/components/select/select-item.svelte | 2 +- .../leaderboard/LeaderboardControls.svelte | 126 +++++++-------- .../state-managers/selectors/measures.ts | 5 + web-local/tests/dashboards/dashboards.spec.ts | 2 +- .../leaderboard-context-column.spec.ts | 2 +- .../dashboards/number-formatting.spec.ts | 8 +- web-local/tests/utils/commonHelpers.ts | 8 +- 17 files changed, 74 insertions(+), 561 deletions(-) delete mode 100644 web-common/src/components/menu/README.md delete mode 100644 web-common/src/components/menu/compositions/SelectMenu.svelte delete mode 100644 web-common/src/components/menu/core/MenuHeader.svelte delete mode 100644 web-common/src/components/menu/shadcn/SelectMenu.svelte delete mode 100644 web-common/src/components/menu/shadcn/SelectMenuContent.svelte delete mode 100644 web-common/src/components/menu/triggers/SelectButton.svelte delete mode 100644 web-common/src/components/menu/types.ts delete mode 100644 web-common/src/components/menu/wrappers/SimpleActionMenu.svelte delete mode 100644 web-common/src/components/menu/wrappers/WithSelectMenu.svelte diff --git a/web-common/src/components/menu/README.md b/web-common/src/components/menu/README.md deleted file mode 100644 index c4f3e925dd8..00000000000 --- a/web-common/src/components/menu/README.md +++ /dev/null @@ -1,13 +0,0 @@ -# Menu components - -This directory contains our menu components. There are three types of components here: - -- `core` – Basic container and logical components for usage. These items do _not_ float on their own; we are mostly using them with `WithTogglableFloatingElement.svelte` component, defined in `src/components/floating-element`. - - `Menu.svelte` – the parent element. This basic component handles keyboard interactions. - - `MenuItem.svelte` – the main child element. Handles selection and rendering of slots. -- `wrappers` – The `With.svelte` collection. Lke a tooltip, these components wrap a DOM element and provide a floating menu component in some way. - - `WithSelectMenu.svelte` – this utilizes `WithFloatingMenu.svelte` and makes it so that you provide the trigger, but the menu attached to the trigger functions like a selector menu. -- `triggers` – Trigger components used in compositions - - `SelectButton.svelte` – a composed trigger button for use in `SimpleSelectMenu.svelte`. -- `compositions` – Convenience components comprised of all of the above, ready-made for usage. - - `SelectMenu.svelte` – this is a full component, combining `SelectButton` and `WithSelectMenu` to create a simple drop-in replacement for `select`. It has other features as well; see the props. diff --git a/web-common/src/components/menu/compositions/SelectMenu.svelte b/web-common/src/components/menu/compositions/SelectMenu.svelte deleted file mode 100644 index e25c709607c..00000000000 --- a/web-common/src/components/menu/compositions/SelectMenu.svelte +++ /dev/null @@ -1,61 +0,0 @@ - - - - - { - if (!multiSelect) selection = event.detail; - dispatch("select", selection); - }} - bind:selection - bind:options - bind:active - let:toggleMenu - let:active -> - - {fixedText} {selection?.main} - - diff --git a/web-common/src/components/menu/core/MenuHeader.svelte b/web-common/src/components/menu/core/MenuHeader.svelte deleted file mode 100644 index 13b4e2ec050..00000000000 --- a/web-common/src/components/menu/core/MenuHeader.svelte +++ /dev/null @@ -1,13 +0,0 @@ -
-
-
- -
-
- -
-
-
diff --git a/web-common/src/components/menu/index.ts b/web-common/src/components/menu/index.ts index 4194ec7882d..ba39a9954d4 100644 --- a/web-common/src/components/menu/index.ts +++ b/web-common/src/components/menu/index.ts @@ -1,11 +1,6 @@ /** See README.md for more information. */ -/** ready-made components */ -export { default as SelectMenu } from "./compositions/SelectMenu.svelte"; + /** core components */ export { default as Divider } from "./core/Divider.svelte"; export { default as Menu } from "./core/Menu.svelte"; export { default as MenuItem } from "./core/MenuItem.svelte"; -/** trigger components */ -export { default as SelectButton } from "./triggers/SelectButton.svelte"; -/** wrapper components */ -export { default as WithSelectMenu } from "./wrappers/WithSelectMenu.svelte"; diff --git a/web-common/src/components/menu/shadcn/SelectMenu.svelte b/web-common/src/components/menu/shadcn/SelectMenu.svelte deleted file mode 100644 index 22731302c53..00000000000 --- a/web-common/src/components/menu/shadcn/SelectMenu.svelte +++ /dev/null @@ -1,42 +0,0 @@ - - - - - - - - - diff --git a/web-common/src/components/menu/shadcn/SelectMenuContent.svelte b/web-common/src/components/menu/shadcn/SelectMenuContent.svelte deleted file mode 100644 index 53ee3fa27f4..00000000000 --- a/web-common/src/components/menu/shadcn/SelectMenuContent.svelte +++ /dev/null @@ -1,47 +0,0 @@ - - - - {#each options as option, i (option.key)} - {@const selected = selections.includes(option.key)} - handleClick(i)} - > -
-
- {option.main} -
- {#if option.description} -

- {option.description} -

- {/if} -
- {option.right || ""} -
- {#if option.divider} - - {/if} - {/each} -
diff --git a/web-common/src/components/menu/triggers/SelectButton.svelte b/web-common/src/components/menu/triggers/SelectButton.svelte deleted file mode 100644 index d89f97a6a7f..00000000000 --- a/web-common/src/components/menu/triggers/SelectButton.svelte +++ /dev/null @@ -1,66 +0,0 @@ - - - diff --git a/web-common/src/components/menu/types.ts b/web-common/src/components/menu/types.ts deleted file mode 100644 index 9ea200414e4..00000000000 --- a/web-common/src/components/menu/types.ts +++ /dev/null @@ -1,14 +0,0 @@ -export type SelectMenuItem = { - // the key of that identifies the item in call - key: string | number; - // the main text - main: string; - // the secondary text below the main text - description?: string; - // text to display to the right of "main" - right?: string; - // will this item be used as a divider? - divider?: boolean; - // will this item be disabled? - disabled?: boolean; -}; diff --git a/web-common/src/components/menu/wrappers/SimpleActionMenu.svelte b/web-common/src/components/menu/wrappers/SimpleActionMenu.svelte deleted file mode 100644 index 7e0a4685f02..00000000000 --- a/web-common/src/components/menu/wrappers/SimpleActionMenu.svelte +++ /dev/null @@ -1,66 +0,0 @@ - - - - - - { - if (active) handleClose(); - }} - on:escape={handleClose} - > - {#each options as { main, right, callback }} - - {main} - - {right || ""} - - - {/each} - - diff --git a/web-common/src/components/menu/wrappers/WithSelectMenu.svelte b/web-common/src/components/menu/wrappers/WithSelectMenu.svelte deleted file mode 100644 index b58215e7cbc..00000000000 --- a/web-common/src/components/menu/wrappers/WithSelectMenu.svelte +++ /dev/null @@ -1,153 +0,0 @@ - - - - - - - { - if (active) handleClose(); - }} - on:escape={handleClose} - > - {#each options as { key, main, description, right, divider = false, disabled = false }, i} - {@const selected = isSelected(selection, key)} - { - temporarilySelectedKey = key; - }} - on:select={createOnClickHandler( - main, - right, - description, - key, - disabled, - i, - handleClose, - )} - {selected} - > - - {#if showCheckJustAfterClick(key) || isAlreadySelectedButNotBeingAnimated(key, selected)} - - {:else} - - {/if} - -
- {main} -
- - {#if description} - {description} - {/if} - - - {right || ""} - -
- {#if divider} - - {/if} - {/each} -
-
diff --git a/web-common/src/components/select/select-item.svelte b/web-common/src/components/select/select-item.svelte index 1b7fe10ae15..30a7cc79f27 100644 --- a/web-common/src/components/select/select-item.svelte +++ b/web-common/src/components/select/select-item.svelte @@ -19,7 +19,7 @@ {disabled} {label} class={cn( - "relative flex w-full cursor-default select-none items-center rounded-sm py-1.5 pl-2 pr-8 text-sm outline-none data-[highlighted]:bg-accent data-[highlighted]:text-accent-foreground data-[disabled]:opacity-50", + "relative flex w-full cursor-pointer select-none items-center justify-center rounded-sm py-1.5 pl-2 pr-8 text-sm outline-none data-[highlighted]:bg-accent data-[highlighted]:text-accent-foreground data-[disabled]:opacity-50", className, )} {...$$restProps} diff --git a/web-common/src/features/dashboards/leaderboard/LeaderboardControls.svelte b/web-common/src/features/dashboards/leaderboard/LeaderboardControls.svelte index 1632c391e0c..4099ccc7612 100644 --- a/web-common/src/features/dashboards/leaderboard/LeaderboardControls.svelte +++ b/web-common/src/features/dashboards/leaderboard/LeaderboardControls.svelte @@ -1,21 +1,23 @@
- {#if measures && options.length && selection} + {#if measures.length && activeLeaderboardMeasure}
- + ({ + value: measure.name ?? "", + label: measure.label ?? measure.name, + }))} + onSelectedChange={(newSelection) => { + if (!newSelection) return; + setLeaderboardMeasureName(newSelection.value); + }} + > + + + + + + {#each measures as measure (measure.name)} + +
+ {measure.label ?? measure.name} +
+ +

+ {measure.description} +

+
+ {/each} +
+
{/if}
diff --git a/web-common/src/features/dashboards/state-managers/selectors/measures.ts b/web-common/src/features/dashboards/state-managers/selectors/measures.ts index 778643fa690..25fcbdb01ac 100644 --- a/web-common/src/features/dashboards/state-managers/selectors/measures.ts +++ b/web-common/src/features/dashboards/state-managers/selectors/measures.ts @@ -14,6 +14,10 @@ export const allMeasures = ({ return measures === undefined ? [] : measures; }; +export const leaderboardMeasureName = ({ dashboard }: DashboardDataSources) => { + return dashboard.leaderboardMeasureName; +}; + export const visibleMeasures = ({ metricsSpecQueryResult, dashboard, @@ -188,4 +192,5 @@ export const measureSelectors = { filteredSimpleMeasures, getFilteredMeasuresAndDimensions, + leaderboardMeasureName, }; diff --git a/web-local/tests/dashboards/dashboards.spec.ts b/web-local/tests/dashboards/dashboards.spec.ts index 1d450eefe3a..2b90918100c 100644 --- a/web-local/tests/dashboards/dashboards.spec.ts +++ b/web-local/tests/dashboards/dashboards.spec.ts @@ -394,7 +394,7 @@ dimensions: await page .getByRole("button", { name: "Select a measure to filter by" }) .click(); - await page.getByRole("menuitem", { name: "Avg Bid Price" }).click(); + await page.getByRole("option", { name: "Avg Bid Price" }).click(); // Check domain and sample value in leaderboard await expect(page.getByText("Domain Name")).toBeVisible(); diff --git a/web-local/tests/dashboards/leaderboard-context-column.spec.ts b/web-local/tests/dashboards/leaderboard-context-column.spec.ts index 302a46e56be..af3aeedfe9b 100644 --- a/web-local/tests/dashboards/leaderboard-context-column.spec.ts +++ b/web-local/tests/dashboards/leaderboard-context-column.spec.ts @@ -43,7 +43,7 @@ test.describe("leaderboard context column", () => { await watcher.updateAndWaitForDashboard(metricsWithValidPercentOfTotal); async function clickMenuItem(itemName: string, wait = true) { - await clickMenuButton(page, itemName); + await clickMenuButton(page, itemName, "option"); if (wait) { await page.getByRole("menu").waitFor({ state: "hidden" }); } diff --git a/web-local/tests/dashboards/number-formatting.spec.ts b/web-local/tests/dashboards/number-formatting.spec.ts index 0ee7e490caf..9151411cc19 100644 --- a/web-local/tests/dashboards/number-formatting.spec.ts +++ b/web-local/tests/dashboards/number-formatting.spec.ts @@ -109,7 +109,7 @@ dimensions: name: "Select a measure to filter by", }); await measuresButton.click(); - await page.getByRole("menuitem", { name: "USD" }).click(); + await page.getByRole("option", { name: "USD" }).click(); await page .getByRole("menu", { name: "Showing USD" }) .waitFor({ state: "hidden" }); @@ -119,7 +119,7 @@ dimensions: page.getByRole("row", { name: "null $98.8k 33%" }), ).toBeVisible(); await measuresButton.click(); - await page.getByRole("menuitem", { name: "percentage" }).click(); + await page.getByRole("option", { name: "percentage" }).click(); await expect(measuresButton).toHaveText("Showing percentage"); await expect( page.getByRole("row", { name: "null 9.9M% 33%" }), @@ -127,7 +127,7 @@ dimensions: // try interval_ms... await measuresButton.click(); - await page.getByRole("menuitem", { name: "interval_ms" }).click(); + await page.getByRole("option", { name: "interval_ms" }).click(); await expect(measuresButton).toHaveText("Showing interval_ms"); // ...and add a time comparison to check deltas await interactWithTimeRangeMenu(page, async () => { @@ -141,7 +141,7 @@ dimensions: // try No Format... await measuresButton.click(); - await page.getByRole("menuitem", { name: "No Format" }).click(); + await page.getByRole("option", { name: "No Format" }).click(); await expect(measuresButton).toHaveText("Showing No Format"); await expect( diff --git a/web-local/tests/utils/commonHelpers.ts b/web-local/tests/utils/commonHelpers.ts index 90827f80e98..51cb7dce59a 100644 --- a/web-local/tests/utils/commonHelpers.ts +++ b/web-local/tests/utils/commonHelpers.ts @@ -24,8 +24,12 @@ export async function clickModalButton(page: Page, text: string) { return page.getByText(text).click(); } -export async function clickMenuButton(page: Page, text: string) { - await page.getByRole("menuitem", { name: text }).click(); +export async function clickMenuButton( + page: Page, + text: string, + role: "menuitem" | "option" = "menuitem", +) { + await page.getByRole(role, { name: text }).click(); } export async function waitForProfiling(