Skip to content

[docs] Fix Safari code font size#34859

Merged
mnajdova merged 1 commit into
mui:masterfrom
oliviertassinari:fix-safari-text-font-size
Oct 31, 2022
Merged

[docs] Fix Safari code font size#34859
mnajdova merged 1 commit into
mui:masterfrom
oliviertassinari:fix-safari-text-font-size

Conversation

@oliviertassinari
Copy link
Copy Markdown
Member

@oliviertassinari oliviertassinari commented Oct 22, 2022 •
edited
Loading

@oliviertassinari oliviertassinari added type: bug It doesn't behave as expected. docs Improvements or additions to the documentation. labels Oct 22, 2022
@mui-bot
Copy link
Copy Markdown

mui-bot commented Oct 22, 2022 •
edited
Loading

Messages
📖 Netlify deploy preview: https://deploy-preview-34859--material-ui.netlify.app/

No bundle size changes

Generated by 🚫 dangerJS against 1a7686f

oliviertassinari
oliviertassinari commented Oct 22, 2022
View reviewed changes
Comment thread docs/public/static/styles/prism-okaidia.css

code[class*='language-'] {
code[class*='language-'],
pre[class*='language-'] {
Copy link
Copy Markdown
Member Author

@oliviertassinari oliviertassinari Oct 22, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I have synced the top of this CSS file with https://unpkg.com/prismjs/themes/prism-okaidia.css. I have also commented what is not necessary.

Comment thread docs/src/modules/components/MarkdownElement.js
// inline code
'& code': {
// inline code block
'& :not(pre) > code': {
Copy link
Copy Markdown
Member Author

@oliviertassinari oliviertassinari Oct 22, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I use :not(pre) in the selector to avoid the need to reset the styles for code blocks. This selector more accurately selects inline code and ONLY. For example, it matches 1. but doesn't 2.

  1. A foo B
 <Foo />

@oliviertassinari oliviertassinari force-pushed the fix-safari-text-font-size branch 9 times, most recently from 1f8a463 to 498345b Compare October 23, 2022 12:10
oliviertassinari
oliviertassinari commented Oct 23, 2022
View reviewed changes
Comment thread docs/src/modules/components/MarkdownElement.js
right: `${theme.spacing(1)} /*! @noflip */`,
fontFamily: 'inherit',
fontSize: '0.813rem',
fontSize: theme.typography.pxToRem(13),
Copy link
Copy Markdown
Member Author

@oliviertassinari oliviertassinari Oct 23, 2022

Choose a reason for hiding this comment

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

The correct API

Comment thread docs/public/static/styles/prism-okaidia.css
.token.doctype,
.token.cdata {
color: slategray;
color: #8292a2;
Copy link
Copy Markdown
Member Author

@oliviertassinari oliviertassinari Oct 23, 2022

Choose a reason for hiding this comment

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

PrismJS/prism#2292

Comment thread docs/public/static/styles/prism-okaidia.css
}

.namespace {
.token.namespace {
Copy link
Copy Markdown
Member Author

@oliviertassinari oliviertassinari Oct 23, 2022

Choose a reason for hiding this comment

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

PrismJS/prism#2161

@github-actions github-actions Bot added the PR: out-of-date The pull request has merge conflicts and can't be merged. label Oct 23, 2022
@oliviertassinari oliviertassinari force-pushed the fix-safari-text-font-size branch from 7b1900a to a3e9f19 Compare October 23, 2022 16:09
@github-actions github-actions Bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged. label Oct 23, 2022
@oliviertassinari oliviertassinari changed the title [docs] Fix code font size with Safari [docs] Fix Safari code font size Oct 23, 2022
@oliviertassinari oliviertassinari force-pushed the fix-safari-text-font-size branch 2 times, most recently from 300b89e to 56ee07a Compare October 23, 2022 16:24
@oliviertassinari oliviertassinari force-pushed the fix-safari-text-font-size branch 3 times, most recently from ff7ab46 to 524075d Compare October 23, 2022 16:33
oliviertassinari
oliviertassinari commented Oct 23, 2022
View reviewed changes
Comment thread docs/src/modules/components/TopLayoutBlog.js
Comment on lines -141 to -143
'& pre': {
fontSize: theme.typography.pxToRem(16),
},
Copy link
Copy Markdown
Member Author

@oliviertassinari oliviertassinari Oct 23, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

This has no impacts, e.g. https://mui.com/blog/date-pickers-stable-v5/#integrated-localization. It's dead code.

@oliviertassinari oliviertassinari added the design This is about UI or UX design, please involve a designer. label Oct 23, 2022
@michaldudak
Copy link
Copy Markdown
Member

michaldudak commented Oct 27, 2022

There are many changes unrelated to the Safari code font size in this PR. Could you either split it into multiple PRs or summarize all the changes in the description and rename the PR to better reflect what it's about?

@oliviertassinari
Copy link
Copy Markdown
Member Author

oliviertassinari commented Oct 27, 2022

@michaldudak I think that we could create about 10 PRs out of this one, which more or less matches each of the line comments.

@michaldudak
Copy link
Copy Markdown
Member

michaldudak commented Oct 28, 2022

IMO it's important to have a description matching the content of the PR. When I do git blame and look for reasons for a specific change, I skim through the description, not the comments. In this case, the description suggests the change is only about font size, which is not.

@oliviertassinari oliviertassinari force-pushed the fix-safari-text-font-size branch 4 times, most recently from dc5fe89 to b94554f Compare October 30, 2022 17:13
@oliviertassinari oliviertassinari force-pushed the fix-safari-text-font-size branch from b94554f to 1a7686f Compare October 30, 2022 17:16
@oliviertassinari
Copy link
Copy Markdown
Member Author

oliviertassinari commented Oct 30, 2022

@michaldudak Done, I have isolated the changes to different PRs.

@oliviertassinari oliviertassinari mentioned this pull request Oct 30, 2022
Merged
oliviertassinari
oliviertassinari commented Oct 30, 2022
View reviewed changes
Comment thread docs/src/modules/components/MarkdownElement.js
Comment on lines +38 to +40
// Reset for Safari
// https://github.com/necolas/normalize.css/blob/master/normalize.css#L102
fontSize: '1em',
Copy link
Copy Markdown
Member Author

@oliviertassinari oliviertassinari Oct 30, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

The fix for Safari font size is to not apply the custom font size on the <code> but on the <pre> element.

@mnajdova mnajdova merged commit c84cca7 into mui:master Oct 31, 2022
@oliviertassinari oliviertassinari deleted the fix-safari-text-font-size branch October 31, 2022 10:32
@oliviertassinari oliviertassinari added the scope: docs-infra Involves the docs-infra product (https://www.notion.so/mui-org/b9f676062eb94747b6768209f7751305). label Nov 21, 2022
daniel-rabe pushed a commit to daniel-rabe/material-ui that referenced this pull request Nov 29, 2022
@oliviertassinari
[docs] Fix Safari code font size (mui#34859)
e083428
feliperli pushed a commit to jesrodri/material-ui that referenced this pull request Dec 6, 2022
@oliviertassinari @feliperli
[docs] Fix Safari code font size (mui#34859)
9197d9e
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@michaldudak michaldudak michaldudak approved these changes

@mnajdova mnajdova Awaiting requested review from mnajdova

Assignees

No one assigned

Labels

design This is about UI or UX design, please involve a designer. docs Improvements or additions to the documentation. scope: docs-infra Involves the docs-infra product (https://www.notion.so/mui-org/b9f676062eb94747b6768209f7751305). type: bug It doesn't behave as expected.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@oliviertassinari @mui-bot @michaldudak @mnajdova