[docs] Fix MarkdownElement regression from adding CSS variables
#35096
Conversation
siriwatknp
commented
Nov 11, 2022
•
siriwatknp
commented
- I have followed (at least) the PR section of the contributing guide.
siriwatknp
added
docs
| '& :not(pre) > code': { | ||
| color: '#fff', | ||
| }, |
There was a problem hiding this comment.
|
siriwatknp
changed the title
MarkdownElement regression on dark mode
| @@ -32,6 +32,7 @@ const StyledMarkdownElement = styled(MarkdownElement)(({ theme }) => ({ | |||
|
|
|||
| const StyledSimpleCodeEditor = styled(SimpleCodeEditor)(({ theme }) => ({ | |||
| ...theme.typography.body2, | |||
| letterSpacing: '0.01071em', // must sync with MarkdownElement.js#L42, otherwise the text will overlap when highlight the code. | |||
There was a problem hiding this comment.
The regression https://deploy-preview-34976--material-ui.netlify.app/material-ui/react-snackbar/#positioned-snackbars
there is a slight offset.
Why was letterSpacing: '0.01071em', MarkdownElement.js#L42, added in #34976?
It feels like we could solve this in a simpler way: by removing these two letterSpacing CSS properties.
If we really want this style, then it might be better to drop lightTheme.typography.fontFamilyCode (what I did was a bit crap, I shouldn't have imported lightTheme, but likely a clean fontCode object). Once we have this fontCode object, we could then spread it where needed:
- ...theme.typography.body2,
- fontSize: theme.typography.pxToRem(13),
- fontFamily: theme.typography.fontFamilyCode,
- letterSpacing: '0.01071em',
+ ...fontCode,x2
or maybe it should have been a variant, yeah maybe even simpler:
- ...theme.typography.body2,
- fontSize: theme.typography.pxToRem(13),
- fontFamily: theme.typography.fontFamilyCode,
- letterSpacing: '0.01071em',
+ ...theme.typography.code,and then make sure Material UI + branding theme is always the parent context.
There was a problem hiding this comment.
and then make sure Material UI + branding theme is always the parent context.
Can we not do this? It is very hard to reason about and produce a different result when the component renders in a different context.
The reason I use lightTheme and darkTheme is to make the component agnostic of the context. This means the component can render in any place without the theme context and it still produces the same appearance.
There was a problem hiding this comment.
Why was letterSpacing: '0.01071em', MarkdownElement.js#L42, added in #34976?
It is because theme.typography.body2 in Material UI contains letterSpacing: '0.01071em' but the branding theme does not so I have to add it here so that we preserve the existing behavior and Argos regression pass.
There was a problem hiding this comment.
Let's break it. I am removing the letter-spacing.
There was a problem hiding this comment.
Can we not do this? It is very hard to reason about and produce a different result when the component renders in a different context.
The meat of this proposal was to make the page throw when the theme in the context provided to a docs-infra component (e.g. MarkdownElement) isn't the brandingTheme. For example, why are 1 and 2 having two different values:
I shared a possible solution in #34976 (comment). Importing the theme solves the visible problem but it feels like we don't solve the root problem strange context values, e.g. with the sx prop we can't use this approach. We also branch away from the default API we recommend people use the component. I fear that it makes it harder to solve as it hides the instances where the value in the context is wrong.
The flip side is that we are getting closer to https://vanilla-extract.style/ with this theme import approach.
It is because theme.typography.body2 in Material UI contains letterSpacing: '0.01071em' but the branding theme does not so I have to add it here so that we preserve the existing behavior and Argos regression pass.
👍 to remove it. I think that letterSpacing was designed for the roboto font with text but we render code here.
There was a problem hiding this comment.
The meat of this proposal was to make the page throw when the theme in the context provided to a docs-infra component
Seems like we have different thoughts on this. Better to discuss it in a proper place to include other opinions and create documentation around it 👉 added to Notion page.
There was a problem hiding this comment.
@siriwatknp I have added a few comments and options on the Notion page. How do you want to move from there? It could be a use case for the the optional decision making meeting slots.
There was a problem hiding this comment.
- I will start documenting "How it currently works"
- Create options that we want to have in the future
- Include X and Toolpad team to take a look, give feedback, and discuss until we agree on the final decision.
- Start implementing (likely 1st-2nd quarter 2023). Keep in mind that whichever option we choose, might impact the Toolpad and X pages so there will be some migration period.
oliviertassinari
changed the title
MarkdownElement regression on dark modeMarkdownElement regression from adding CSS variables
