Skip to content
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

#9222 Add policy for hiding the sidebar logo #9224

Merged
merged 12 commits into from
Oct 3, 2024
71 changes: 66 additions & 5 deletions src/hooks/useTheme.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,21 +22,31 @@ import { initialTheme } from "@/themes/themeStore";
import { type AsyncState } from "@/types/sliceTypes";
import { themeStorage } from "@/themes/themeUtils";
import { activateTheme } from "@/background/messenger/api";
import { readManagedStorageByKey } from "@/store/enterprise/managedStorage";

afterEach(() => {
jest.clearAllMocks();
});

jest.mock("@/hooks/useAsyncExternalStore");
jest.mock("@/background/messenger/api");
jest.mock("@/store/enterprise/managedStorage");

describe("useTheme", () => {
beforeEach(() => {
beforeEach(async () => {
jest
.mocked(useAsyncExternalStore)
.mockReturnValue({ data: initialTheme, isLoading: false } as AsyncState);
// eslint-disable-next-line no-restricted-syntax -- this func requires a parameter
jest.mocked(readManagedStorageByKey).mockResolvedValue(undefined);
});

test("calls useAsyncExternalStore and gets current theme state", async () => {
const { result: themeResult } = renderHook(() => useTheme());
const { result: themeResult, waitForNextUpdate } = renderHook(() =>
useTheme(),
);

await waitForNextUpdate();

expect(useAsyncExternalStore).toHaveBeenNthCalledWith(
1,
Expand All @@ -57,20 +67,71 @@ describe("useTheme", () => {
});
});

it("calls activateTheme after loading is done and it hasn't been called recently", () => {
it("calls activateTheme after loading is done and it hasn't been called recently", async () => {
jest.useFakeTimers();

jest.mocked(useAsyncExternalStore).mockReturnValue({
data: { ...initialTheme, lastFetched: Date.now() },
isLoading: false,
} as AsyncState);

renderHook(() => useTheme());
let result = renderHook(() => useTheme());
await result.waitForNextUpdate();
expect(activateTheme).not.toHaveBeenCalled();

jest.advanceTimersByTime(125_000);

renderHook(() => useTheme());
result = renderHook(() => useTheme());
await result.waitForNextUpdate();
expect(activateTheme).toHaveBeenCalledOnce();
});

it("overrides showSidebarLogo when hideSidebarLogo is true in managed storage", async () => {
jest.mocked(readManagedStorageByKey).mockResolvedValue(true);

const { result, waitForNextUpdate } = renderHook(() => useTheme());

await waitForNextUpdate();

expect(result.current.activeTheme.showSidebarLogo).toBe(false);
});

it("uses default activeTheme when hideSidebarLogo is false in managed storage", async () => {
jest.mocked(readManagedStorageByKey).mockResolvedValue(false);

const { result, waitForNextUpdate } = renderHook(() => useTheme());

await waitForNextUpdate();

expect(result.current.activeTheme.showSidebarLogo).toBe(
initialTheme.showSidebarLogo,
);
});

it("uses default activeTheme when hideSidebarLogo is undefined in managed storage", async () => {
// eslint-disable-next-line no-restricted-syntax -- this func requires a parameter
jest.mocked(readManagedStorageByKey).mockResolvedValue(undefined);

const { result, waitForNextUpdate } = renderHook(() => useTheme());

await waitForNextUpdate();

expect(result.current.activeTheme.showSidebarLogo).toBe(
initialTheme.showSidebarLogo,
);
});

it("uses default activeTheme when an error occurs while fetching from managed storage", async () => {
jest
.mocked(readManagedStorageByKey)
.mockRejectedValue(new Error("Managed storage error"));

const { result, waitForNextUpdate } = renderHook(() => useTheme());

await waitForNextUpdate();

expect(result.current.activeTheme.showSidebarLogo).toBe(
initialTheme.showSidebarLogo,
);
});
});
23 changes: 18 additions & 5 deletions src/hooks/useTheme.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@ import {
import { initialTheme } from "@/themes/themeStore";
import useAsyncExternalStore from "@/hooks/useAsyncExternalStore";
import { activateTheme } from "@/background/messenger/api";
import useAsyncState from "@/hooks/useAsyncState";
import { readManagedStorageByKey } from "@/store/enterprise/managedStorage";

const themeStorageSubscribe = (callback: () => void) => {
const abortController = new AbortController();
Expand All @@ -42,10 +44,13 @@ const themeStorageSubscribe = (callback: () => void) => {
function useTheme(): { activeTheme: ThemeAssets; isLoading: boolean } {
// The active theme is fetched with `getActiveTheme` in the background script and cached in the themeStorage,
// This hook subscribes to changes in themeStorage to retrieve the latest current activeTheme
const { data: cachedTheme, isLoading } = useAsyncExternalStore(
themeStorageSubscribe,
themeStorage.get,
);
const { data: cachedTheme, isLoading: isCachedThemeLoading } =
useAsyncExternalStore(themeStorageSubscribe, themeStorage.get);

const { data: hideSidebarLogo, isLoading: isManagedStorageLoading } =
Copy link
Contributor

@twschiller twschiller Oct 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NIT: instead of using useAsyncState with readManagedStorageByKey, we might consider using useManagedStorageState (and standardizing that hook return value use theAsyncState shape)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TIL we have a useManagedStorageState hook. Since we're releasing tomorrow, let's merge this as-is.

@mnholtz before you move on to something else, can you open up a follow-up PR to leverage useManageStorageState, including the enhancement to return AsyncState?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ticket here: #9227

useAsyncState(async () => readManagedStorageByKey("hideSidebarLogo"), []);

const isLoading = isManagedStorageLoading || isCachedThemeLoading;

useEffect(() => {
if (
Expand All @@ -70,7 +75,15 @@ function useTheme(): { activeTheme: ThemeAssets; isLoading: boolean } {
setThemeFavicon(activeTheme.themeName);
}, [activeTheme, cachedTheme, isLoading]);

return { activeTheme, isLoading };
return {
activeTheme: {
...activeTheme,
// There is a managed storage policy to hide the sidebar logo, overriding the team theme
// See managedStorageSchema.json
showSidebarLogo: hideSidebarLogo ? false : activeTheme.showSidebarLogo,
},
isLoading,
};
}

export default useTheme;
9 changes: 4 additions & 5 deletions src/sidebar/Header.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -53,17 +53,16 @@ const Header: React.FunctionComponent = () => {

return (
<div className="d-flex py-2 pl-2 pr-0 align-items-center">
{showSidebarLogo && (
// `mx-auto` centers the logo
<div className="mx-auto">
<div className="mx-auto">
Copy link
Collaborator Author

@mnholtz mnholtz Oct 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Always include a placeholder for the image so that it still fills the space when hidden; before the extension buttons (e.g. the cog button) were left-aligned instead of staying in place

{showSidebarLogo && (
<img
src={customSidebarLogo ?? logo.regular}
alt={customSidebarLogo ? "Custom logo" : "PixieBrix logo"}
className={styles.logo}
data-testid="sidebarHeaderLogo"
/>
</div>
)}
)}
</div>
{showDeveloperUI && (
<Button
type="button"
Expand Down
5 changes: 5 additions & 0 deletions src/store/enterprise/managedStorageTypes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -73,4 +73,9 @@ export type ManagedStorageState = {
* @since 1.7.36
*/
disableBrowserWarning?: boolean;
/**
* Hides the logo in the PixieBrix sidebar, overriding the team theme.
* @since 2.1.4
mnholtz marked this conversation as resolved.
Show resolved Hide resolved
*/
hideSidebarLogo?: boolean;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be:

Suggested change
hideSidebarLogo?: boolean;
hideSidebarLogo: boolean;

Since the schema says it defaults to false? Or does that default not actually do anything?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a great question. Upon manual inspection the default value doesn't seem to do anything. I also wasn't able to find mention of this value in the docs anywhere https://developer.chrome.com/docs/extensions/reference/manifest/storage

Copy link
Contributor

@twschiller twschiller Oct 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a great question. Upon manual inspection the default value doesn't seem to do anything. I also wasn't able to find mention of this value in the docs anywhere

That's correct - the default does not flow through to Chrome. In general, default in JSON Schemas don't have runtime implications. It's a hint to tools. See: https://json-schema.org/understanding-json-schema/reference/annotations#annotations

The default keyword specifies a default value. This value is not used to fill in missing values during the validation process. Non-validation tools such as documentation generators or form generators may use this value to give hints to users about how to use a value. However, default is typically used to express that if a value is missing, then the value is semantically the same as if the value was present with the default value. The value of default should validate against the schema in which it resides, but that isn't required.

};
5 changes: 5 additions & 0 deletions static/managedStorageSchema.json
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,11 @@
"type": "boolean",
"description": "Disable the browser warning for non-Chrome browsers, e.g., Microsoft Edge",
"default": false
},
"hideSidebarLogo": {
"type": "boolean",
"description": "If set, hide the logo on the PixieBrix sidebar, overriding the team theme.",
mnholtz marked this conversation as resolved.
Show resolved Hide resolved
"default": false
}
}
}
Loading