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] Fix typography fallback value #37845

Merged
merged 1 commit into from
Jul 11, 2023

Conversation

siriwatknp
Copy link
Member

@siriwatknp siriwatknp commented Jul 6, 2023

closes #37000

Root cause

The getCssVar('fontFamily-body', 'JetBrains Mono') mistakenly wraps the JetBrains Mono with var(…).

Fixed by adding the fallback directly at the end of the variable:

- getCssVar('fontFamily-body', 'JetBrains Mono')
+ getCssVar('fontFamily-body, JetBrains Mono') // return `var(--joy-fontFamily-body, JetBrains Mono)`

Further improvement

This can be further improved in a separate PR by removing the entire logic for checking CSS value and use the above method instead to reduce Regex calculation:

diff --git a/packages/mui-system/src/cssVars/createGetCssVar.ts b/packages/mui-system/src/cssVars/createGetCssVar.ts
index 17eb0b7461..ae1a90ea4e 100644
--- a/packages/mui-system/src/cssVars/createGetCssVar.ts
+++ b/packages/mui-system/src/cssVars/createGetCssVar.ts
@@ -8,15 +8,7 @@ export default function createGetCssVar<T extends string = string>(prefix: strin
       return '';
     }
     const value = vars[0];
-    if (
-      typeof value === 'string' &&
-      !value.match(
-        /(#|\(|\)|(-?(\d*\.)?\d+)(px|em|%|ex|ch|rem|vw|vh|vmin|vmax|cm|mm|in|pt|pc))|^(-?(\d*\.)?\d+)$|(\d+ \d+ \d+)/,
-      )
-    ) {
-      return `, var(--${prefix ? `${prefix}-` : ''}${value}${appendVar(...vars.slice(1))})`;
-    }
-    return `, ${value}`;
+    return `, var(--${prefix ? `${prefix}-` : ''}${value}${appendVar(...vars.slice(1))})`;
   }
 
   // AdditionalVars makes `getCssVar` less strict, so it can be use like this `getCssVar('non-mui-variable')` without type error.

@siriwatknp siriwatknp added bug 🐛 Something doesn't work package: joy-ui Specific to @mui/joy customization: theme Centered around the theming features labels Jul 6, 2023
@mui-bot
Copy link

mui-bot commented Jul 6, 2023

Netlify deploy preview

https://deploy-preview-37845--material-ui.netlify.app/

Bundle size report

Details of bundle changes (Toolpad)
Details of bundle changes

Generated by 🚫 dangerJS against 4d68d22

Comment on lines +95 to +104
it('should have correct font family', () => {
const theme = extendTheme({ fontFamily: { body: 'JetBrains Mono' } });
expect(theme.typography.body1).to.deep.equal({
fontFamily: 'var(--joy-fontFamily-body, JetBrains Mono)',
fontSize: 'var(--joy-fontSize-md, 1rem)',
lineHeight: 'var(--joy-lineHeight-md, 1.5)',
color: 'var(--joy-palette-text-primary, var(--joy-palette-neutral-800, #25252D))',
});
});

Copy link
Member Author

Choose a reason for hiding this comment

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

This test case covers the issue.

@siriwatknp siriwatknp merged commit baaaec3 into mui:master Jul 11, 2023
20 of 21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work customization: theme Centered around the theming features package: joy-ui Specific to @mui/joy
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Joy] Using extendTheme with fontFamily can lead to invalid CSS variable names
3 participants