-
-
Notifications
You must be signed in to change notification settings - Fork 32.4k
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
[docs] Improve font size scaling of some demos #19950
Conversation
No bundle size changes comparing c7e6eee...084d91f |
ce530c3
to
b25ab40
Compare
b25ab40
to
133747b
Compare
What do you think of using 24px as the baseline over 40px? I have looked at the accessibility feature of Chrome, they have the following for the html font-size:
|
Hum, but at the same time, there is this 200% resize criterion: https://www.w3.org/WAI/WCAG21/Understanding/resize-text.html. Maybe it should be 32px as baseline? |
@@ -11,8 +11,6 @@ import Typography from '@material-ui/core/Typography'; | |||
const useStyles = makeStyles((theme: Theme) => | |||
createStyles({ | |||
root: { | |||
width: '100%', | |||
maxWidth: 360, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The change breaks the purpose of the demo:
You should change the list item alignment when displaying 3 lines or more, set the alignItems="flex-start" property.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand the purpose from this sentence. How does the width relate to the number of items?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea is to have list items with enough height to show the impact of the flex positioning of the avatar (top vs middle)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So why not set the height instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Relying on text wrapping avoids the need to know the computed line-height. In our case, I think that a width ch approach would work.
Cool stuff. I had to search for what the ch unit is for, I had forgotten, it's probably the first time I see a good reason for using it: and seems specific for our demos. |
I'm not using it as a baseline. It was just a extreme example of a base font size (marked as "huge" in chrome).
I was looking for a better way. How did you find it? I always used the settings directly. |
Yeah I used the settings as well. Not very handy though since I need to open it in a separate tab. With zoom I can use a shortcut. |
Using a fixed width for a demo can give a bad impression when testing font-size scaling of our components. To preserve the "fixed asthetics" we can use
ch
units. It breaks down on very large scales but it's better than what we currently have.There are plenty of demos where we could improve the scaling impressions. I think this is a good start. At least for the first demos on a page we should give the best impression possible.
Credits to @missmatsuko for their article.
Using 40px font-size as the base:
before:
https://material-ui.netlify.com/components/text-fields/
after:
https://deploy-preview-19950--material-ui.netlify.com/components/text-fields/
Argos diff is expected, should be accepted by a reviewer.