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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

[docs] Fix Safari code font size #34859

Merged
merged 1 commit into from Oct 31, 2022

Conversation

@oliviertassinari oliviertassinari added bug 馃悰 Something doesn't work docs Improvements or additions to the documentation labels Oct 22, 2022
@mui-bot
Copy link

mui-bot commented Oct 22, 2022

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

No bundle size changes

Generated by 馃毇 dangerJS against 1a7686f


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

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.

// inline code
'& code': {
// inline code block
'& :not(pre) > code': {
Copy link
Member Author

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
fontFamily: 'inherit',
fontSize: '0.813rem',
fontSize: theme.typography.pxToRem(13),
Copy link
Member Author

Choose a reason for hiding this comment

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

The correct API

.token.comment,
.token.prolog,
.token.doctype,
.token.cdata {
color: slategray;
color: #8292a2;
Copy link
Member Author

Choose a reason for hiding this comment

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


.token.punctuation {
color: #f8f8f2;
}

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

Choose a reason for hiding this comment

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

@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
@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
Comment on lines -141 to -143
'& pre': {
fontSize: theme.typography.pxToRem(16),
},
Copy link
Member Author

Choose a reason for hiding this comment

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

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

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
Member Author

@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
Member

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
Copy link
Member Author

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

Comment on lines +38 to +40
// Reset for Safari
// https://github.com/necolas/normalize.css/blob/master/normalize.css#L102
fontSize: '1em',
Copy link
Member Author

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 Specific to the docs-infra product label Nov 21, 2022
daniel-rabe pushed a commit to daniel-rabe/material-ui that referenced this pull request Nov 29, 2022
feliperli pushed a commit to jesrodri/material-ui that referenced this pull request Dec 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 馃悰 Something doesn't work design This is about UI or UX design, please involve a designer docs Improvements or additions to the documentation scope: docs-infra Specific to the docs-infra product
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants