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

[Joy] Export CssVarsProvider with default theme #29150

Merged
merged 23 commits into from
Oct 31, 2021

Conversation

siriwatknp
Copy link
Member

@siriwatknp siriwatknp commented Oct 19, 2021

This is the first PR for defining Joy's theme structure. It is necessary before building components.

The theme structure is based on the github discussion #29024.

Necessary APIs for building components are exported from mui-joy/src/styles similar to material. All the types of the theme structure is defined in mui-joy/src/styles/defaultTheme.ts

  • CssVarsProvider
  • ThemeProvider
  • styled

Some properties are not included yet (will add in other PR when it is needed).

  • breakpoints
  • spacing
  • transition

@mui-pr-bot
Copy link

mui-pr-bot commented Oct 19, 2021

Details of bundle changes

Generated by 🚫 dangerJS against 8b48b84

@@ -0,0 +1,268 @@
const colors = {
Copy link
Member Author

Choose a reason for hiding this comment

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

Copied from tailwindcss to get started with some default colors.

Comment on lines -25 to -28
? { theme?: PartialDeep<Theme> }
? { theme?: Theme }
: {
theme: PartialDeep<Omit<Theme, 'colorSchemes'>> & {
colorSchemes: PartialDeep<
Copy link
Member Author

Choose a reason for hiding this comment

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

Remove PartialDeep here because it breaks function type. It is better to let DesignSystem handle the Theme type.

Comment on lines +32 to +34
RequiredDeep<
Record<ApplicationColorScheme, Theme['colorSchemes'][ApplicationColorScheme]>
>;
Copy link
Member Author

Choose a reason for hiding this comment

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

use RequiredDeep to make sure that all of the key:value for ApplicationColorScheme must be specified.

Comment on lines 45 to 52
DesignSystemThemeInput extends {
colorSchemes: Record<DesignSystemColorScheme | ApplicationColorScheme, any>;
},
DesignSystemColorScheme extends string,
ApplicationColorScheme extends string = never,
ApplicationThemeInput extends {
colorSchemes: Record<DesignSystemColorScheme | ApplicationColorScheme, any>;
} = DesignSystemThemeInput,
Copy link
Member Author

Choose a reason for hiding this comment

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

Split into DesignSystemThemeInput and ApplicationThemeInput to handle types separately.

| number
>,
string
expectedStyle: Partial<
Copy link
Member Author

Choose a reason for hiding this comment

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

In testfile.test.tsx, without Partial, the matcher expect all of CSSStyleDeclaration

@siriwatknp siriwatknp marked this pull request as ready for review October 21, 2021 03:29
@siriwatknp siriwatknp changed the title [Joy] Export ThemeProvider with default theme [Joy] Export CssVarsProvider with default theme Oct 21, 2021
@siriwatknp siriwatknp marked this pull request as draft October 21, 2021 12:09
@siriwatknp siriwatknp marked this pull request as ready for review October 22, 2021 09:52
@siriwatknp siriwatknp mentioned this pull request Oct 25, 2021
1 task
Copy link
Member

@mnajdova mnajdova left a comment

Choose a reason for hiding this comment

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

I didn't really understand the need for the useTheme option in createStyled. Could you elaborate more on it. Especially as with it we are adding wrappers around each styled component. This means that we are diverging from how it is implemented in both emotion/styled-components.

packages/mui-joy/src/styles/CssVarsProvider.spec.tsx Outdated Show resolved Hide resolved
packages/mui-joy/src/styles/ThemeProvider.tsx Show resolved Hide resolved
packages/mui-system/src/createStyled.js Outdated Show resolved Hide resolved
packages/mui-system/src/createStyled.js Outdated Show resolved Hide resolved
@siriwatknp
Copy link
Member Author

@mnajdova @hbjORbj Do you see anything else that I should fix in this PR?

I have reverted the createStyled and moved forward with emotion's ThemeProvider. I think it should be fine for starting the default theme.

Copy link
Member

@hbjORbj hbjORbj left a comment

Choose a reason for hiding this comment

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

🤩

Co-authored-by: Benny Joo <sldisek783@gmail.com>
@siriwatknp siriwatknp merged commit dfdae5c into mui:master Oct 31, 2021
@oliviertassinari
Copy link
Member

It broke the CI, it seems to be always failing when the test run on Firefox

Screenshot 2021-11-01 at 11 55 38

expect(screen.getByTestId('palette').textContent).to.equal(
JSON.stringify({
brand: {
50: 'var(--joy-palette-brand-50)',
Copy link
Member

Choose a reason for hiding this comment

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

Does it mean that the Material Design CSS variable will be?

Suggested change
50: 'var(--joy-palette-brand-50)',
50: 'var(--md-palette-brand-50)',

Copy link
Member Author

@siriwatknp siriwatknp Nov 1, 2021

Choose a reason for hiding this comment

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

yes, developer will be able to custom the prefix.

@@ -38,15 +38,28 @@
"typescript:module-augmentation": "node scripts/testModuleAugmentation.js"
},
"peerDependencies": {
"@emotion/react": "^11.4.1",
Copy link
Member

Choose a reason for hiding this comment

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

It might be safer to use the same version as Material Design:

Suggested change
"@emotion/react": "^11.4.1",
"@emotion/react": "^11.5.0",

Copy link
Member Author

Choose a reason for hiding this comment

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

Noted.

@@ -0,0 +1,141 @@
import * as React from 'react';
import colors from '../colors';

Copy link
Member

Choose a reason for hiding this comment

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

Should we leave a TODO?

Suggested change
// TODO extend import { createTheme, ThemeOptions, Theme } from '@mui/system';

@oliviertassinari oliviertassinari changed the title [Joy] Export CssVarsProvider with default theme [joy] Export CssVarsProvider with default theme Nov 1, 2021
@oliviertassinari oliviertassinari added the package: joy-ui Specific to @mui/joy label Nov 1, 2021
@danilo-leal danilo-leal changed the title [joy] Export CssVarsProvider with default theme [Joy] Export CssVarsProvider with default theme Feb 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package: joy-ui Specific to @mui/joy
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants