Repository navigation
feat(sidebar): add open and close methods for sidebar app management #2969
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
base: main
Are you sure you want to change the base?
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 |
|---|---|---|
| @@ -0,0 +1,84 @@ | ||
| // @vitest-environment happy-dom | ||
| import { afterEach, beforeEach, expect, it, vi } from "vitest"; | ||
|
|
||
| const sidebar = vi.hoisted(() => ({ | ||
| show: vi.fn(), | ||
| hide: vi.fn(), | ||
| })); | ||
|
|
||
| const settings = vi.hoisted(() => ({ | ||
| value: { showSponsorSidebarApp: false }, | ||
| on: vi.fn(), | ||
| })); | ||
|
|
||
| const instances = vi.hoisted(() => []); | ||
|
|
||
| vi.mock("components/sidebar", () => ({ default: sidebar })); | ||
| vi.mock("lib/settings", () => ({ default: settings })); | ||
| vi.mock("pages/sponsors", () => ({ default: vi.fn() })); | ||
| vi.mock("../../src/sidebarApps/sidebarApp.js", () => ({ | ||
| default: class SidebarApp { | ||
| static init = vi.fn(); | ||
|
|
||
| constructor(icon, id, title, init, onSelected) { | ||
| this.id = id; | ||
| this.container = document.createElement("div"); | ||
| this.install = vi.fn(); | ||
| this.remove = vi.fn(); | ||
| this.active = false; | ||
| instances.push(this); | ||
| } | ||
| }, | ||
| })); | ||
|
|
||
| let sidebarApps; | ||
|
|
||
| beforeEach(async () => { | ||
| vi.resetModules(); | ||
| localStorage.clear(); | ||
| instances.length = 0; | ||
| sidebarApps = (await import("../../src/sidebarApps/index.js")).default; | ||
|
Member
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. All 4 tests here fail before any assertion runs: importing The module needs a JSX transform in vitest (or the JSX needs to be avoided or mocked out in this import path) before these tests check anything. |
||
| const icons = document.createElement("div"); | ||
| sidebarApps.init({ | ||
| get: vi.fn(() => icons), | ||
| }); | ||
| }); | ||
|
|
||
| afterEach(() => { | ||
| vi.clearAllMocks(); | ||
| document.body.replaceChildren(); | ||
| }); | ||
|
|
||
| it("opens the sidebar and activates the requested app", () => { | ||
| sidebarApps.add("folder", "files", "Files"); | ||
| sidebarApps.add("extension", "extensions", "Extensions"); | ||
|
|
||
| expect(sidebarApps.open("extensions")).toBe(true); | ||
| expect(sidebar.show).toHaveBeenCalledOnce(); | ||
| expect(instances.map((app) => app.active)).toEqual([false, true]); | ||
| expect(localStorage.getItem("sidebarAppsLastSection")).toBe("extensions"); | ||
| }); | ||
|
|
||
| it("opens the current app when no app id is supplied", () => { | ||
| sidebarApps.add("folder", "files", "Files"); | ||
|
|
||
| expect(sidebarApps.open()).toBe(true); | ||
| expect(sidebar.show).toHaveBeenCalledOnce(); | ||
| expect(instances[0].active).toBe(true); | ||
| }); | ||
|
|
||
| it("does not open the sidebar for an unknown app", () => { | ||
| sidebarApps.add("folder", "files", "Files"); | ||
|
|
||
| expect(sidebarApps.open("missing")).toBe(false); | ||
| expect(sidebar.show).not.toHaveBeenCalled(); | ||
| expect(instances[0].active).toBe(true); | ||
| }); | ||
|
|
||
| it("closes the sidebar without changing the active app", () => { | ||
| sidebarApps.add("folder", "files", "Files"); | ||
|
|
||
| expect(sidebarApps.close()).toBe(true); | ||
| expect(sidebar.hide).toHaveBeenCalledOnce(); | ||
| expect(instances[0].active).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.
Sidebar.show()doesn't check whether the sidebar is already open. On phone, every call doesactionStack.push({ id: "sidebar", ... }), andactionStack.removeonly removes one match.So if a plugin calls
open("myApp")to switch apps while the sidebar is already showing, the stack ends up with twosidebarentries. The first Back closes the sidebar. The second Back runshideMasteron a sidebar that's already hidden: the press is swallowed, and thehidelisteners fire again.openWithSearchalready had this behaviour, but as a public plugin API it's much easier to hit. Suggestion: if the sidebar is already activated, just switch the app and skipSidebar.show()(or makeshow()do nothing when$el.activatedis set).