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.

Sign up for GitHub

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

Jump to bottom

Sb/add theme basics #725

Draft
wants to merge 1 commit into
base: wk/20231120-try-mui-joy
Choose a base branch
from
+41 −0
Draft

Sb/add theme basics #725

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions packages/components/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1,3 @@
export * from './lib/Button';

export * from './lib/theme';
39 changes: 39 additions & 0 deletions packages/components/src/lib/theme.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
import { CssVarsProvider, extendTheme } from '@mui/joy/styles';
import type { PaletteRange } from '@mui/joy/styles';

Check failure on line 2 in packages/components/src/lib/theme.tsx

View workflow job for this annotation

GitHub Actions / lint 

'PaletteRange' is defined but never used

declare module '@mui/joy/styles' {
interface ColorPalettePropOverrides {
// apply to all Joy UI components that support `color` prop
brand: true;
}

interface Palette {
// this will make the node `secondary` configurable in `extendTheme`
// and add `secondary` to the theme's palette.
brand: {
blue: string;
lightBlue: string;
};
Comment on lines +13 to +16
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this not have to be brand: PaletteRange? That's what the docs would suggest. Can you link in your PR description to whatever you're following? That's the only what i can judge correct-ness.

Copy link
Contributor

@WilliamKelley WilliamKelley Nov 28, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Also this seems like it would expose color="brand" as an option for color-enabled components. But "brand" is not a valid color for us and is not our desired API. So this would be a wrong way to get these values (blue, light blue) in the theme

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried PalletteRange and got the following error when I tried to set a value to blue:

Type '{ blue: string; lightBlue: string; }' is not assignable to type '{ 50?: string | undefined; 100?: string | undefined; 200?: string | undefined; 300?: string | undefined; 400?: string | undefined; 500?: string | undefined; 600?: string | undefined; 700?: string | undefined; ... 27 more ...; solidDisabledBg?: string | undefined; }

Which is why I went with being specific to each one.

}
}
export const prendaTheme = extendTheme({
colorSchemes: {
light: {
palette: {
brand: {
blue: '#0A4872',
lightBlue: '#D7F3FF',
},
},
},
},
fontFamily: {
body: '"Poppins", sans-serif',
Copy link
Contributor

Choose a reason for hiding this comment

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

Poppins is only used for headings. Normal body text is Inter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, I was putting something there as a placeholder. Where is all of the body/h1 => font documented?
I looked here

Copy link
Contributor

Choose a reason for hiding this comment

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

There's no defined mapping because there shouldn't be. See my comment in Figma on yours. Variants and the underlying html tag (component) are rightfully independent.

},
});

export const ThemeProvider = ({
children,
}: {
children: React.ReactElement;
}) => <CssVarsProvider theme={prendaTheme}>{children}</CssVarsProvider>;
Loading