Skip to content

Commit

Permalink
Allow custom themes to override embed options query parameter (#8021)
Browse files Browse the repository at this point in the history
## Describe your changes

We set the theme based on the query param each time the script run is
finished. This can break other theme settings, so we want to respect the
query parameters only on initial load (in determining the default theme.

## GitHub Issue Link (if applicable)
Closes #7118 

## Testing Plan

- Updated JS Unit Tests for the default theme. There were no tests for
the script finished action, so I did not include tests for the removal.

---

**Contribution License Agreement**

By submitting this pull request you agree that all contributions to this
project are made under the Apache 2.0 license.
  • Loading branch information
kmcgrady committed Mar 12, 2024
1 parent b90d822 commit cadcba4
Show file tree
Hide file tree
Showing 6 changed files with 238 additions and 58 deletions.
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
11 changes: 0 additions & 11 deletions frontend/app/src/App.tsx
Expand Up @@ -43,10 +43,8 @@ import {
getIFrameEnclosingApp,
hashString,
isColoredLineDisplayed,
isDarkTheme,
isEmbed,
isInChildFrame,
isLightTheme,
isPaddingDisplayed,
isScrollingHidden,
isToolbarDisplayed,
Expand Down Expand Up @@ -107,7 +105,6 @@ import {
LibConfig,
AppConfig,
} from "@streamlit/lib"
import noop from "lodash/noop"
import without from "lodash/without"

import { UserSettings } from "@streamlit/app/src/components/StreamlitDialog/UserSettings"
Expand Down Expand Up @@ -1058,14 +1055,6 @@ export class App extends PureComponent<Props, State> {
const successful =
status === ForwardMsg.ScriptFinishedStatus.FINISHED_SUCCESSFULLY
window.setTimeout(() => {
// Set the theme if url query param ?embed_options=[light,dark]_theme is set
const [light, dark] = this.props.theme.availableThemes.slice(1, 3)
if (isLightTheme()) {
this.setAndSendTheme(light)
} else if (isDarkTheme()) {
this.setAndSendTheme(dark)
} else noop() // Do nothing when ?embed_options=[light,dark]_theme is not set

// Notify any subscribers of this event (and do it on the next cycle of
// the event loop)
this.state.scriptFinishedHandlers.map(handler => handler())
Expand Down
116 changes: 79 additions & 37 deletions frontend/lib/src/theme/utils.test.ts
Expand Up @@ -54,6 +54,34 @@ const matchMediaFillers = {
dispatchEvent: jest.fn(),
}

const windowLocationSearch = (search: string): any => ({
location: {
search,
},
})

const windowMatchMedia = (theme: "light" | "dark"): any => ({
matchMedia: (query: any) => ({
matches: query === `(prefers-color-scheme: ${theme})`,
media: query,
...matchMediaFillers,
}),
})

const mockWindow = (...overrides: object[]): jest.SpyInstance => {
const localStorage = window.localStorage
const windowSpy = jest.spyOn(window, "window", "get")

windowSpy.mockImplementation(() => ({
localStorage,
...windowLocationSearch(""),
...windowMatchMedia("light"),
...Object.assign({}, ...overrides),
}))

return windowSpy
}

describe("Styling utils", () => {
describe("computeSpacingStyle", () => {
test("pulls correct theme values", async () => {
Expand Down Expand Up @@ -359,48 +387,36 @@ describe("createTheme", () => {
})

describe("getSystemTheme", () => {
let windowSpy: jest.SpyInstance

afterEach(() => {
windowSpy.mockRestore()
window.localStorage.clear()
})

it("returns lightTheme when matchMedia does *not* match dark", () => {
Object.defineProperty(window, "matchMedia", {
writable: true,
value: jest.fn().mockImplementation(query => ({
matches: false,
media: query,
...matchMediaFillers,
})),
})
windowSpy = mockWindow()

expect(getSystemTheme().name).toBe("Light")
})

it("returns darkTheme when matchMedia does match dark", () => {
Object.defineProperty(window, "matchMedia", {
writable: true,
value: jest.fn().mockImplementation(query => ({
matches: true,
media: query,
...matchMediaFillers,
})),
})
windowSpy = mockWindow(windowMatchMedia("dark"))

expect(getSystemTheme().name).toBe("Dark")
})
})

describe("getDefaultTheme", () => {
beforeEach(() => {
// sourced from:
// https://jestjs.io/docs/en/manual-mocks#mocking-methods-which-are-not-implemented-in-jsdom
Object.defineProperty(window, "matchMedia", {
writable: true,
value: jest.fn().mockImplementation(query => ({
matches: false,
media: query,
...matchMediaFillers,
})),
})
let windowSpy: jest.SpyInstance

afterEach(() => {
windowSpy.mockRestore()
window.localStorage.clear()
})

it("sets default to the auto theme when there is no cached theme", () => {
windowSpy = mockWindow()
const defaultTheme = getDefaultTheme()

expect(defaultTheme.name).toBe(AUTO_THEME_NAME)
Expand All @@ -409,16 +425,7 @@ describe("getDefaultTheme", () => {
})

it("sets the auto theme correctly when the OS preference is dark", () => {
// sourced from:
// https://jestjs.io/docs/en/manual-mocks#mocking-methods-which-are-not-implemented-in-jsdom
Object.defineProperty(window, "matchMedia", {
writable: true,
value: jest.fn().mockImplementation(query => ({
matches: true,
media: query,
...matchMediaFillers,
})),
})
mockWindow(windowSpy, windowMatchMedia("dark"))

const defaultTheme = getDefaultTheme()

Expand All @@ -427,13 +434,48 @@ describe("getDefaultTheme", () => {
})

it("sets the default to the user preference when one is set", () => {
windowSpy = mockWindow()
setCachedTheme(darkTheme)

const defaultTheme = getDefaultTheme()

expect(defaultTheme.name).toBe("Dark")
expect(defaultTheme.emotion.colors).toEqual(darkTheme.emotion.colors)
})

it("sets default to the light theme when an embed query parameter is set", () => {
windowSpy = mockWindow(
windowLocationSearch("?embed=true&embed_options=light_theme")
)
const defaultTheme = getDefaultTheme()

expect(defaultTheme.name).toBe("Light")
// Also verify that the theme is our lightTheme.
expect(defaultTheme.emotion.colors).toEqual(lightTheme.emotion.colors)
})

it("sets default to the dark theme when an embed query parameter is set", () => {
windowSpy = mockWindow(
windowLocationSearch("?embed=true&embed_options=dark_theme")
)
const defaultTheme = getDefaultTheme()

expect(defaultTheme.name).toBe("Dark")
// Also verify that the theme is our darkTheme.
expect(defaultTheme.emotion.colors).toEqual(darkTheme.emotion.colors)
})

it("respects embed query parameter is set over system theme", () => {
windowSpy = mockWindow(
windowMatchMedia("dark"),
windowLocationSearch("?embed=true&embed_options=light_theme")
)
const defaultTheme = getDefaultTheme()

expect(defaultTheme.name).toBe("Light")
// Also verify that the theme is our lightTheme.
expect(defaultTheme.emotion.colors).toEqual(lightTheme.emotion.colors)
})
})

describe("isColor", () => {
Expand Down
29 changes: 21 additions & 8 deletions frontend/lib/src/theme/utils.ts
Expand Up @@ -39,6 +39,8 @@ import {
ThemeSpacing,
} from "@streamlit/lib/src/theme"

import { isLightTheme, isDarkTheme } from "@streamlit/lib/src/util/utils"

import { fonts } from "./primitives/typography"
import {
computeDerivedColors,
Expand Down Expand Up @@ -379,15 +381,26 @@ export const removeCachedTheme = (): void => {

export const getDefaultTheme = (): ThemeConfig => {
// Priority for default theme
// 1. Previous user preference
// 2. OS preference
// If local storage has Auto, refetch system theme as it may have changed
// based on time of day. We shouldn't ever have this saved in our storage
// but checking in case!
const cachedTheme = getCachedTheme()
return cachedTheme && cachedTheme.name !== AUTO_THEME_NAME
? cachedTheme
: createAutoTheme()

// 1. Previous user preference
// We shouldn't ever have auto saved in our storage in case
// OS theme changes but we explicitly check in case!
if (cachedTheme && cachedTheme.name !== AUTO_THEME_NAME) {
return cachedTheme
}

// 2. Embed Parameter preference
if (isLightTheme()) {
return lightTheme
}

if (isDarkTheme()) {
return darkTheme
}

// 3. OS preference
return createAutoTheme()
}

const whiteSpace = /\s+/
Expand Down
129 changes: 129 additions & 0 deletions frontend/lib/src/util/utils.test.ts
Expand Up @@ -24,6 +24,12 @@ import {
isEmbed,
setCookie,
preserveEmbedQueryParams,
isColoredLineDisplayed,
isToolbarDisplayed,
isPaddingDisplayed,
isScrollingHidden,
isLightTheme,
isDarkTheme,
} from "./utils"

describe("getCookie", () => {
Expand Down Expand Up @@ -234,6 +240,129 @@ describe("isEmbed", () => {
expect(isEmbed()).toBe(true)
})

it("embed Options should return false even if ?embed=true", () => {
windowSpy.mockImplementation(() => ({
location: {
search: "?embed=true",
},
}))

expect(isColoredLineDisplayed()).toBe(false)
expect(isToolbarDisplayed()).toBe(false)
expect(isPaddingDisplayed()).toBe(false)
expect(isScrollingHidden()).toBe(false)
expect(isLightTheme()).toBe(false)
expect(isDarkTheme()).toBe(false)
})

it("embed Options should return false even if ?embed=false", () => {
windowSpy.mockImplementation(() => ({
location: {
search:
"?embed=false&embed_options=show_colored_line,show_toolbar,show_padding,disable_scrolling",
},
}))

expect(isColoredLineDisplayed()).toBe(false)
expect(isToolbarDisplayed()).toBe(false)
expect(isPaddingDisplayed()).toBe(false)
expect(isScrollingHidden()).toBe(false)
})

it("embed Options should return false even if ?embed is not set", () => {
windowSpy.mockImplementation(() => ({
location: {
search:
"?embed_options=show_colored_line,show_toolbar,show_padding,disable_scrolling",
},
}))

expect(isColoredLineDisplayed()).toBe(false)
expect(isToolbarDisplayed()).toBe(false)
expect(isPaddingDisplayed()).toBe(false)
expect(isScrollingHidden()).toBe(false)
})

it("should specify light theme if in embed options", () => {
windowSpy.mockImplementation(() => ({
location: {
search: "?embed_options=light_theme",
},
}))

expect(isLightTheme()).toBe(true)
})

it("should specify dark theme if in embed options", () => {
windowSpy.mockImplementation(() => ({
location: {
search: "?embed_options=dark_theme",
},
}))

expect(isDarkTheme()).toBe(true)
})

it("should disable scrolling if in embed options", () => {
windowSpy.mockImplementation(() => ({
location: {
search: "?embed=true&embed_options=disable_scrolling",
},
}))

expect(isColoredLineDisplayed()).toBe(false)
expect(isToolbarDisplayed()).toBe(false)
expect(isPaddingDisplayed()).toBe(false)
expect(isScrollingHidden()).toBe(true)
expect(isLightTheme()).toBe(false)
expect(isDarkTheme()).toBe(false)
})

it("should show padding if in embed options", () => {
windowSpy.mockImplementation(() => ({
location: {
search: "?embed=true&embed_options=show_padding",
},
}))

expect(isColoredLineDisplayed()).toBe(false)
expect(isToolbarDisplayed()).toBe(false)
expect(isPaddingDisplayed()).toBe(true)
expect(isScrollingHidden()).toBe(false)
expect(isLightTheme()).toBe(false)
expect(isDarkTheme()).toBe(false)
})

it("should show the toolbar if in embed options", () => {
windowSpy.mockImplementation(() => ({
location: {
search: "?embed=true&embed_options=show_toolbar",
},
}))

expect(isColoredLineDisplayed()).toBe(false)
expect(isToolbarDisplayed()).toBe(true)
expect(isPaddingDisplayed()).toBe(false)
expect(isScrollingHidden()).toBe(false)
expect(isLightTheme()).toBe(false)
expect(isDarkTheme()).toBe(false)
})

it("should show the colored line if in embed options", () => {
windowSpy.mockImplementation(() => ({
location: {
search: "?embed=true&embed_options=show_colored_line",
},
}))

expect(isColoredLineDisplayed()).toBe(true)
expect(isToolbarDisplayed()).toBe(false)
expect(isPaddingDisplayed()).toBe(false)
expect(isScrollingHidden()).toBe(false)
expect(isLightTheme()).toBe(false)
expect(isDarkTheme()).toBe(false)
})

it("isEmbed is case insensitive, so should return true when ?embed=TrUe", () => {
windowSpy.mockImplementation(() => ({
location: {
Expand Down

0 comments on commit cadcba4

Please sign in to comment.