-
Notifications
You must be signed in to change notification settings - Fork 55
Unify plugin browse action buttons #994
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,7 +6,6 @@ import { | |
| ResourceBrowseGrid, | ||
| ResourceCollectionViewport, | ||
| ResourceInstallControl, | ||
| ResourceInstalledControl, | ||
| ResourceListState, | ||
| ResourceSortMenu, | ||
| ResourceToolbar, | ||
|
|
@@ -193,11 +192,12 @@ function BrowseCard({ | |
| ) : undefined; | ||
| const headerAction = | ||
| installedPluginId !== null ? ( | ||
| <ResourceInstalledControl | ||
| <ResourceInstallControl | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🚨 This visual signal conflicts with the accessible label, tooltip, and action. The shared
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🚨 The browser test confirmed that each installed card shows
|
||
| accessibleLabel={`Uninstall ${entry.displayName}`} | ||
| pending={uninstall.isPending} | ||
| presentation="icon" | ||
| tooltip={`Uninstall ${entry.displayName}`} | ||
| className="border-success/40 bg-success/15 text-[color:color-mix(in_oklab,var(--success)_72%,var(--ink))] hover:border-success/55 hover:bg-success/25 hover:text-[color:color-mix(in_oklab,var(--success)_72%,var(--ink))] focus-visible:border-success/55 focus-visible:bg-success/25 focus-visible:text-[color:color-mix(in_oklab,var(--success)_72%,var(--ink))]" | ||
| onAction={() => setConfirmingUninstall(true)} | ||
| /> | ||
| ) : ( | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,29 +1,90 @@ | ||
| // @vitest-environment jsdom | ||
|
|
||
| import { act, cleanup, render } from "@testing-library/react"; | ||
| import { createElement } from "react"; | ||
| import { afterEach, vi } from "vitest"; | ||
| import { describe, expect, it } from "vitest"; | ||
| import { | ||
| getTabStripChevronEdgeClass, | ||
| getTabStripChevronVisibilityClass, | ||
| SecondaryPanelTabStrip, | ||
| SECONDARY_PANEL_TAB_STRIP_FADE_TONE, | ||
| } from "./SecondaryPanelTabStrip"; | ||
|
|
||
| afterEach(() => { | ||
| cleanup(); | ||
| vi.unstubAllGlobals(); | ||
| }); | ||
|
|
||
| describe("secondary panel tab-strip edge fades", () => { | ||
| it("uses one opaque themed edge fade and no second caret gradient", () => { | ||
| it("uses the themed edge fade without overlay scroll controls", () => { | ||
| expect(SECONDARY_PANEL_TAB_STRIP_FADE_TONE).toBe("sidebar"); | ||
| expect(getTabStripChevronEdgeClass("left")).toBe( | ||
| "left-0 justify-start", | ||
| ); | ||
| expect(getTabStripChevronEdgeClass("right")).toBe( | ||
| "right-0 justify-end", | ||
| ); | ||
| }); | ||
|
|
||
| it("keeps an available scroll control visible without requiring hover", () => { | ||
| const visibleClass = getTabStripChevronVisibilityClass(true); | ||
| it("observes the intrinsic tab row so async title changes refresh overflow", () => { | ||
| const observed: Element[] = []; | ||
| let resizeCallback: ResizeObserverCallback | undefined; | ||
| vi.stubGlobal( | ||
| "ResizeObserver", | ||
| class { | ||
| constructor(callback: ResizeObserverCallback) { | ||
| resizeCallback = callback; | ||
| } | ||
| observe(element: Element) { | ||
| observed.push(element); | ||
| } | ||
| disconnect() {} | ||
| }, | ||
| ); | ||
|
|
||
| const { container } = render( | ||
| createElement(SecondaryPanelTabStrip, { | ||
| fileTabs: [ | ||
| { | ||
| id: "browser", | ||
| filename: "Browser", | ||
| isActive: true, | ||
| isPinned: false, | ||
| leadingVisual: null, | ||
| statusLabel: null, | ||
| onSelect: vi.fn(), | ||
| onClose: vi.fn(), | ||
| }, | ||
| ], | ||
| onReorderTab: vi.fn(), | ||
| usesDesktopChrome: false, | ||
| }), | ||
| ); | ||
|
|
||
| expect(visibleClass).toContain("pointer-events-auto"); | ||
| expect(visibleClass).toContain("opacity-100"); | ||
| expect(visibleClass).not.toContain("hover:"); | ||
| expect(getTabStripChevronVisibilityClass(false)).toBe( | ||
| "pointer-events-none opacity-0", | ||
| const viewport = container.querySelector(".no-scrollbar"); | ||
| const content = container.querySelector( | ||
| "[data-secondary-panel-tab-content]", | ||
| ); | ||
| expect(content).not.toBeNull(); | ||
| expect(observed).toContain(viewport); | ||
| expect(observed).toContain(content); | ||
| expect(resizeCallback).toBeDefined(); | ||
| expect(container.querySelectorAll("[data-overflow-fade]")).toHaveLength(2); | ||
| expect( | ||
| container | ||
| .querySelector("[data-overflow-fade='left']") | ||
| ?.classList.contains("w-6"), | ||
| ).toBe(true); | ||
| expect( | ||
| container.querySelector('[aria-label="Scroll tabs left"]'), | ||
| ).toBeNull(); | ||
| expect( | ||
| container.querySelector('[aria-label="Scroll tabs right"]'), | ||
| ).toBeNull(); | ||
|
|
||
| const rightFade = container.querySelector("[data-overflow-fade='right']"); | ||
| expect(rightFade?.classList.contains("opacity-0")).toBe(true); | ||
| Object.defineProperties(viewport!, { | ||
| clientWidth: { configurable: true, value: 120 }, | ||
| scrollWidth: { configurable: true, value: 240 }, | ||
| scrollLeft: { configurable: true, value: 0, writable: true }, | ||
| }); | ||
| act(() => { | ||
| resizeCallback?.([], {} as ResizeObserver); | ||
| }); | ||
| expect(rightFade?.classList.contains("opacity-100")).toBe(true); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚨
slopcop/review— The installed state now depends on color alone.Both branches show the same Download icon. Only the green color identifies the installed state. This design also presents an uninstall action as a download action.
Use a distinct installed icon. Preserve a destructive hover or focus signal before the confirmation dialog.