From 4a13f9d1fe84b348d75200126b0d684a95cb4b1f Mon Sep 17 00:00:00 2001 From: Matthew Wilcoxson Date: Mon, 12 May 2025 18:21:16 +0100 Subject: [PATCH 1/2] Fix merging of child / parent themes. --- changelog.md | 1 + src/storybook/theme/Colours.mdx | 4 +-- src/storybook/theme/Logos.mdx | 7 +++-- src/themes/BaseTheme.ts | 6 ++++ src/themes/DiamondTheme.ts | 19 +++++++++---- src/themes/GenericTheme.ts | 12 ++++---- src/themes/ThemeManager.test.ts | 50 +++++++++++++++++++++++++++++++++ src/themes/ThemeManager.ts | 45 +++++++++++++++++++++++++++++ 8 files changed, 129 insertions(+), 15 deletions(-) create mode 100644 src/themes/ThemeManager.test.ts create mode 100644 src/themes/ThemeManager.ts diff --git a/changelog.md b/changelog.md index 4b95159..66cbdb1 100644 --- a/changelog.md +++ b/changelog.md @@ -10,6 +10,7 @@ SciReactUI Changelog ### Fixed - Styles added to Navbar and Footer incorrectly remove built in styles. +- Themes were not inheriting all details from their parent. ### Changed - Breadcrumbs component takes optional linkComponent prop for page routing. diff --git a/src/storybook/theme/Colours.mdx b/src/storybook/theme/Colours.mdx index 8e07ca1..828ac58 100644 --- a/src/storybook/theme/Colours.mdx +++ b/src/storybook/theme/Colours.mdx @@ -59,13 +59,13 @@ export function ThemeColorItem({title, theme, colourSet, mode}) { ### Light Mode - + ### Dark Mode - + diff --git a/src/storybook/theme/Logos.mdx b/src/storybook/theme/Logos.mdx index c1e7d1b..4df9765 100644 --- a/src/storybook/theme/Logos.mdx +++ b/src/storybook/theme/Logos.mdx @@ -1,4 +1,4 @@ -import { Meta, ColorPalette, ColorItem } from '@storybook/blocks'; +import { Meta } from '@storybook/blocks'; import {ImageColorSchemeSwitch} from "../../components/controls/ImageColorSchemeSwitch"; @@ -24,7 +24,10 @@ import {ImageColorSchemeSwitch} from "../../components/controls/ImageColorScheme } ``` - +
+ +

Example logo

+
## Sizes There are two image sizes. I regular `normal` and a smaller version `short`, these are used in the Navbar and diff --git a/src/themes/BaseTheme.ts b/src/themes/BaseTheme.ts index 400a268..e79b225 100644 --- a/src/themes/BaseTheme.ts +++ b/src/themes/BaseTheme.ts @@ -29,11 +29,17 @@ const BaseThemeOptions /* : ThemeOptions */ = { palette: { background: { default: "#fafafa" }, }, + text: { + primary: "#050505" + }, }, dark: { palette: { background: { default: "#050505" }, }, + text: { + primary: "#fafafa" + }, }, }, components: {}, diff --git a/src/themes/DiamondTheme.ts b/src/themes/DiamondTheme.ts index 63d7d3e..2135ec4 100644 --- a/src/themes/DiamondTheme.ts +++ b/src/themes/DiamondTheme.ts @@ -1,7 +1,7 @@ -import { createTheme, Theme } from "@mui/material/styles"; import type {} from "@mui/material/themeCssVarsAugmentation"; +import { createTheme, Theme } from "@mui/material/styles"; -import { BaseThemeOptions } from "./BaseTheme"; +import {mergeThemeOptions} from "./ThemeManager"; import logoImageDark from "../public/diamond/logo-dark.svg"; import logoImageLight from "../public/diamond/logo-light.svg"; @@ -10,8 +10,7 @@ import logoShort from "../public/diamond/logo-short.svg"; const dlsLogoBlue = "#202740"; const dlsLogoYellow = "#facf07"; -const DiamondTheme: Theme = createTheme({ - ...BaseThemeOptions, +const DiamondThemeOptions = mergeThemeOptions({ logos: { normal: { src: logoImageLight, @@ -41,6 +40,9 @@ const DiamondTheme: Theme = createTheme({ dark: "#AF9004", // dark yellow contrastText: "#000000", // black }, + text: { + secondary: "#161B2C" + }, }, }, dark: { @@ -57,13 +59,16 @@ const DiamondTheme: Theme = createTheme({ dark: "#AF9004", // dark yellow contrastText: "#000000", // black }, + text: { + secondary: "#8090CA" + }, }, }, }, components: { MuiButton: { styleOverrides: { - root: ({ theme }) => ({ + root: ({ theme }: any ) => ({ textTransform: "none", "&.MuiButton-contained": {}, "&.default": { @@ -92,4 +97,6 @@ const DiamondTheme: Theme = createTheme({ }, }); -export { DiamondTheme }; +const DiamondTheme: Theme = createTheme( DiamondThemeOptions ); + +export { DiamondTheme, DiamondThemeOptions }; diff --git a/src/themes/GenericTheme.ts b/src/themes/GenericTheme.ts index 113234f..12ceb72 100644 --- a/src/themes/GenericTheme.ts +++ b/src/themes/GenericTheme.ts @@ -1,12 +1,12 @@ -import { createTheme, Theme } from "@mui/material/styles"; +import type {} from "@mui/material/themeCssVarsAugmentation"; +import {createTheme, Theme} from "@mui/material/styles"; -import { BaseThemeOptions } from "./BaseTheme"; +import {mergeThemeOptions} from "./ThemeManager"; import logoImageDark from "../public/generic/logo-dark.svg"; import logoImageLight from "../public/generic/logo-light.svg"; -const GenericTheme: Theme = createTheme({ - ...BaseThemeOptions, +const GenericThemeOptions = mergeThemeOptions({ logos: { normal: { src: logoImageLight, @@ -22,4 +22,6 @@ const GenericTheme: Theme = createTheme({ }, }); -export { GenericTheme }; +const GenericTheme: Theme = createTheme(GenericThemeOptions) +console.log( GenericTheme ) +export { GenericTheme, GenericThemeOptions }; diff --git a/src/themes/ThemeManager.test.ts b/src/themes/ThemeManager.test.ts new file mode 100644 index 0000000..660d731 --- /dev/null +++ b/src/themes/ThemeManager.test.ts @@ -0,0 +1,50 @@ +import "@testing-library/jest-dom"; + +import { BaseThemeOptions } from "./BaseTheme"; +import { mergeThemeOptions } from "./ThemeManager"; + +describe("Theme Manager merge", () => { + it("should merge", () => { + const a = { a: 1, b: 2 }; + const b = { x: 1, y: 2 }; + const new_a = mergeThemeOptions(a,b); + + expect(new_a).toStrictEqual( { a: 1, b: 2, x: 1, y: 2} ) + }) + + + it("should deep merge", () => { + const a = { a: 1, b: 2, c: {d: 1, e: 2}}; + const b = { x: 1, y:{ z: 3 } }; + const merged = mergeThemeOptions(a, b); + + expect(merged).toStrictEqual( { a: 1, b: 2, c: {d: 1, e: 2}, x: 1, y:{ z: 3 }} ) + }) + + it("should use a value over b", () => { + const a = {a: 100, b: 2}; + const b = {a: 1, c: 3}; + const merged = mergeThemeOptions(a, b); + + expect(merged).toStrictEqual({ a: 100, b: 2, c:3 }) + }) + + it("should take the base theme and make a new one", () => { + const fontSize = 4422; + const a = { typography: { fontSize: fontSize } }; + const b = BaseThemeOptions; + + const merged = mergeThemeOptions(a, b); + + expect(BaseThemeOptions.typography.fontSize).not.toStrictEqual( fontSize ) + expect(merged.typography.fontSize).toStrictEqual( fontSize ) + }) + + it("should effectively clone to an empty object", () => { + const a = {a: 100, b: 2}; + const cloned = mergeThemeOptions({}, a); + + expect(cloned).toStrictEqual(a) + }) + +}) \ No newline at end of file diff --git a/src/themes/ThemeManager.ts b/src/themes/ThemeManager.ts new file mode 100644 index 0000000..5412ebd --- /dev/null +++ b/src/themes/ThemeManager.ts @@ -0,0 +1,45 @@ +import { BaseThemeOptions } from "./BaseTheme"; + +/* + Merge two options, with newThemeOptions having precedence. + If no parent is selected the BaseThemeOptions is used + Doesn't affect either options passed in. + */ +function mergeThemeOptions(newThemeOptions: any, parentThemeOptions: any=BaseThemeOptions ) { + const parentThemeOptionsCopy = deepCopyObject(parentThemeOptions); + return mergeObjects(parentThemeOptionsCopy, newThemeOptions ) +} + + +function mergeObjects(mainThemeOptions: any, parentThemeOptions: any, visited = new Map()) { + //This Deep Merge algorithm is based on https://www.geeksforgeeks.org/how-to-deep-merge-two-objects-in-typescript/ + if (isObject(mainThemeOptions) && isObject(parentThemeOptions)) { + for (const key in parentThemeOptions) { + if (isObject(parentThemeOptions[key])) { + if (!mainThemeOptions[key]) { + mainThemeOptions[key] = {}; + } + // Check if the parentThemeOptions object has already been visited + if (!visited.has(parentThemeOptions[key])) { + visited.set(parentThemeOptions[key], {}); + mergeObjects(mainThemeOptions[key], parentThemeOptions[key], visited); + } else { + mainThemeOptions[key] = visited.get(parentThemeOptions[key]); + } + } else { + mainThemeOptions[key] = parentThemeOptions[key]; + } + } + } + return mainThemeOptions; +} + +function isObject(item: any): boolean { + return item !== null && typeof item === 'object' && !Array.isArray(item); +} + +function deepCopyObject(themeOptions: any) { + return mergeObjects({}, themeOptions ) +} + +export { mergeThemeOptions }; \ No newline at end of file From 9f8e43a902b3a5d91ee50e07e1b63610e9a9b677 Mon Sep 17 00:00:00 2001 From: Matthew Wilcoxson Date: Wed, 11 Jun 2025 10:40:08 +0100 Subject: [PATCH 2/2] Irritating changes. --- src/themes/BaseTheme.ts | 4 +- src/themes/DiamondTheme.ts | 10 ++-- src/themes/GenericTheme.ts | 8 +-- src/themes/ThemeManager.test.ts | 92 +++++++++++++++++---------------- src/themes/ThemeManager.ts | 65 ++++++++++++----------- 5 files changed, 95 insertions(+), 84 deletions(-) diff --git a/src/themes/BaseTheme.ts b/src/themes/BaseTheme.ts index e79b225..d51ef9b 100644 --- a/src/themes/BaseTheme.ts +++ b/src/themes/BaseTheme.ts @@ -30,7 +30,7 @@ const BaseThemeOptions /* : ThemeOptions */ = { background: { default: "#fafafa" }, }, text: { - primary: "#050505" + primary: "#050505", }, }, dark: { @@ -38,7 +38,7 @@ const BaseThemeOptions /* : ThemeOptions */ = { background: { default: "#050505" }, }, text: { - primary: "#fafafa" + primary: "#fafafa", }, }, }, diff --git a/src/themes/DiamondTheme.ts b/src/themes/DiamondTheme.ts index 2135ec4..770f830 100644 --- a/src/themes/DiamondTheme.ts +++ b/src/themes/DiamondTheme.ts @@ -1,7 +1,7 @@ import type {} from "@mui/material/themeCssVarsAugmentation"; import { createTheme, Theme } from "@mui/material/styles"; -import {mergeThemeOptions} from "./ThemeManager"; +import { mergeThemeOptions } from "./ThemeManager"; import logoImageDark from "../public/diamond/logo-dark.svg"; import logoImageLight from "../public/diamond/logo-light.svg"; @@ -41,7 +41,7 @@ const DiamondThemeOptions = mergeThemeOptions({ contrastText: "#000000", // black }, text: { - secondary: "#161B2C" + secondary: "#161B2C", }, }, }, @@ -60,7 +60,7 @@ const DiamondThemeOptions = mergeThemeOptions({ contrastText: "#000000", // black }, text: { - secondary: "#8090CA" + secondary: "#8090CA", }, }, }, @@ -68,7 +68,7 @@ const DiamondThemeOptions = mergeThemeOptions({ components: { MuiButton: { styleOverrides: { - root: ({ theme }: any ) => ({ + root: ({ theme }:{ theme:Theme}) => ({ textTransform: "none", "&.MuiButton-contained": {}, "&.default": { @@ -97,6 +97,6 @@ const DiamondThemeOptions = mergeThemeOptions({ }, }); -const DiamondTheme: Theme = createTheme( DiamondThemeOptions ); +const DiamondTheme: Theme = createTheme(DiamondThemeOptions); export { DiamondTheme, DiamondThemeOptions }; diff --git a/src/themes/GenericTheme.ts b/src/themes/GenericTheme.ts index 12ceb72..d945eb0 100644 --- a/src/themes/GenericTheme.ts +++ b/src/themes/GenericTheme.ts @@ -1,7 +1,7 @@ import type {} from "@mui/material/themeCssVarsAugmentation"; -import {createTheme, Theme} from "@mui/material/styles"; +import { createTheme, Theme } from "@mui/material/styles"; -import {mergeThemeOptions} from "./ThemeManager"; +import { mergeThemeOptions } from "./ThemeManager"; import logoImageDark from "../public/generic/logo-dark.svg"; import logoImageLight from "../public/generic/logo-light.svg"; @@ -22,6 +22,6 @@ const GenericThemeOptions = mergeThemeOptions({ }, }); -const GenericTheme: Theme = createTheme(GenericThemeOptions) -console.log( GenericTheme ) +const GenericTheme: Theme = createTheme(GenericThemeOptions); +console.log(GenericTheme); export { GenericTheme, GenericThemeOptions }; diff --git a/src/themes/ThemeManager.test.ts b/src/themes/ThemeManager.test.ts index 660d731..40bd359 100644 --- a/src/themes/ThemeManager.test.ts +++ b/src/themes/ThemeManager.test.ts @@ -4,47 +4,51 @@ import { BaseThemeOptions } from "./BaseTheme"; import { mergeThemeOptions } from "./ThemeManager"; describe("Theme Manager merge", () => { - it("should merge", () => { - const a = { a: 1, b: 2 }; - const b = { x: 1, y: 2 }; - const new_a = mergeThemeOptions(a,b); - - expect(new_a).toStrictEqual( { a: 1, b: 2, x: 1, y: 2} ) - }) - - - it("should deep merge", () => { - const a = { a: 1, b: 2, c: {d: 1, e: 2}}; - const b = { x: 1, y:{ z: 3 } }; - const merged = mergeThemeOptions(a, b); - - expect(merged).toStrictEqual( { a: 1, b: 2, c: {d: 1, e: 2}, x: 1, y:{ z: 3 }} ) - }) - - it("should use a value over b", () => { - const a = {a: 100, b: 2}; - const b = {a: 1, c: 3}; - const merged = mergeThemeOptions(a, b); - - expect(merged).toStrictEqual({ a: 100, b: 2, c:3 }) - }) - - it("should take the base theme and make a new one", () => { - const fontSize = 4422; - const a = { typography: { fontSize: fontSize } }; - const b = BaseThemeOptions; - - const merged = mergeThemeOptions(a, b); - - expect(BaseThemeOptions.typography.fontSize).not.toStrictEqual( fontSize ) - expect(merged.typography.fontSize).toStrictEqual( fontSize ) - }) - - it("should effectively clone to an empty object", () => { - const a = {a: 100, b: 2}; - const cloned = mergeThemeOptions({}, a); - - expect(cloned).toStrictEqual(a) - }) - -}) \ No newline at end of file + it("should merge", () => { + const a = { a: 1, b: 2 }; + const b = { x: 1, y: 2 }; + const new_a = mergeThemeOptions(a, b); + + expect(new_a).toStrictEqual({ a: 1, b: 2, x: 1, y: 2 }); + }); + + it("should deep merge", () => { + const a = { a: 1, b: 2, c: { d: 1, e: 2 } }; + const b = { x: 1, y: { z: 3 } }; + const merged = mergeThemeOptions(a, b); + + expect(merged).toStrictEqual({ + a: 1, + b: 2, + c: { d: 1, e: 2 }, + x: 1, + y: { z: 3 }, + }); + }); + + it("should use a value over b", () => { + const a = { a: 100, b: 2 }; + const b = { a: 1, c: 3 }; + const merged = mergeThemeOptions(a, b); + + expect(merged).toStrictEqual({ a: 100, b: 2, c: 3 }); + }); + + it("should take the base theme and make a new one", () => { + const fontSize = 4422; + const a = { typography: { fontSize: fontSize } }; + const b = BaseThemeOptions; + + const merged = mergeThemeOptions(a, b); + + expect(BaseThemeOptions.typography.fontSize).not.toStrictEqual(fontSize); + expect(merged.typography.fontSize).toStrictEqual(fontSize); + }); + + it("should effectively clone to an empty object", () => { + const a = { a: 100, b: 2 }; + const cloned = mergeThemeOptions({}, a); + + expect(cloned).toStrictEqual(a); + }); +}); diff --git a/src/themes/ThemeManager.ts b/src/themes/ThemeManager.ts index 5412ebd..f9d2807 100644 --- a/src/themes/ThemeManager.ts +++ b/src/themes/ThemeManager.ts @@ -1,45 +1,52 @@ import { BaseThemeOptions } from "./BaseTheme"; +/* eslint-disable @typescript-eslint/no-explicit-any */ /* Merge two options, with newThemeOptions having precedence. If no parent is selected the BaseThemeOptions is used Doesn't affect either options passed in. */ -function mergeThemeOptions(newThemeOptions: any, parentThemeOptions: any=BaseThemeOptions ) { - const parentThemeOptionsCopy = deepCopyObject(parentThemeOptions); - return mergeObjects(parentThemeOptionsCopy, newThemeOptions ) +function mergeThemeOptions( + newThemeOptions: object, + parentThemeOptions: object = BaseThemeOptions, +) { + const parentThemeOptionsCopy = deepCopyObject(parentThemeOptions); + return mergeObjects(parentThemeOptionsCopy, newThemeOptions); } - -function mergeObjects(mainThemeOptions: any, parentThemeOptions: any, visited = new Map()) { - //This Deep Merge algorithm is based on https://www.geeksforgeeks.org/how-to-deep-merge-two-objects-in-typescript/ - if (isObject(mainThemeOptions) && isObject(parentThemeOptions)) { - for (const key in parentThemeOptions) { - if (isObject(parentThemeOptions[key])) { - if (!mainThemeOptions[key]) { - mainThemeOptions[key] = {}; - } - // Check if the parentThemeOptions object has already been visited - if (!visited.has(parentThemeOptions[key])) { - visited.set(parentThemeOptions[key], {}); - mergeObjects(mainThemeOptions[key], parentThemeOptions[key], visited); - } else { - mainThemeOptions[key] = visited.get(parentThemeOptions[key]); - } - } else { - mainThemeOptions[key] = parentThemeOptions[key]; - } - } - } - return mainThemeOptions; +function mergeObjects( + mainThemeOptions: any, + parentThemeOptions: any, + visited = new Map(), +) { + //This Deep Merge algorithm is based on https://www.geeksforgeeks.org/how-to-deep-merge-two-objects-in-typescript/ + if (isObject(mainThemeOptions) && isObject(parentThemeOptions)) { + for (const key in parentThemeOptions) { + if (isObject(parentThemeOptions[key])) { + if (!mainThemeOptions[key]) { + mainThemeOptions[key] = {}; + } + // Check if the parentThemeOptions object has already been visited + if (!visited.has(parentThemeOptions[key])) { + visited.set(parentThemeOptions[key], {}); + mergeObjects(mainThemeOptions[key], parentThemeOptions[key], visited); + } else { + mainThemeOptions[key] = visited.get(parentThemeOptions[key]); + } + } else { + mainThemeOptions[key] = parentThemeOptions[key]; + } + } + } + return mainThemeOptions; } function isObject(item: any): boolean { - return item !== null && typeof item === 'object' && !Array.isArray(item); + return item !== null && typeof item === "object" && !Array.isArray(item); } -function deepCopyObject(themeOptions: any) { - return mergeObjects({}, themeOptions ) +function deepCopyObject(themeOptions: object) { + return mergeObjects({}, themeOptions); } -export { mergeThemeOptions }; \ No newline at end of file +export { mergeThemeOptions };