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

[docs] Make <main> responsive to font size #24531

Merged
merged 1 commit into from
Feb 1, 2021

Conversation

eps1lon
Copy link
Member

@eps1lon eps1lon commented Jan 21, 2021

This is a bit of an experiment how we can reconcile grids in Material design with system-adjustable font settings.

Right now we're restricting <main> to a pixel based max-width. This works fine with the default font settings. However, browsers can change the default font settings. Increasing the font size to "Large" in chrome (20px) leads to a cramped props tables in /api:

Screenshot from 2021-01-21 15-32-17

By making the max-width based on ch we give every font setting the same amount of characters per line (up until the screen is no longer big enough).
Preview: https://deploy-preview-24531--material-ui.netlify.app/api/text-field/

In the end, it might be interesting if we could make breakpoints based on ch entirely. This is just an incomplete fix for our specific problem in props tables.

@eps1lon eps1lon added docs Improvements or additions to the documentation accessibility a11y labels Jan 21, 2021
@mui-pr-bot
Copy link

mui-pr-bot commented Jan 21, 2021

Details of bundle changes

Generated by 🚫 dangerJS against 90008f9

@eps1lon eps1lon marked this pull request as ready for review January 21, 2021 14:49
Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

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

The change seems (I don't have a large enough screen to be able to truly experience the difference) to work better when the base font size is increased (I haven't tested decreased).

Does it make sense to create breakpoints based on ch?

I'm not sure to understand the question. If we move forward with maxWidth: '120ch', I think that it would indeed make more sense in this very case.

If it's a broader question:

  • I don't think that the Container should use the ch unit by default, simply because the content matching a specific set of characters is not the most common objective for its usage.
  • The breakpoints are designed to match underlying hardware devices, it seems that a unit pixels based best approximate it.

My main question would be: Is the change worth not dogfooding the Container's default implementation?

@eps1lon
Copy link
Member Author

eps1lon commented Jan 22, 2021

I don't think that the Container should use the ch unit by default, simply because the content matching a specific set of characters is not the most common objective for its usage.

Yeah, definitely. What I meant is that we use ch based breakpoints in the theme used in our docs.

The breakpoints are designed to match underlying hardware devices, it seems that a unit pixels based best approximate it.

Sure, but this seems to break down once font size is adjustable. Especially the larger the container is.

@oliviertassinari
Copy link
Member

@mbrookes What do you think?

Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

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

No answer from Matt so far, we can wait a bit more, otherwise, it seems OK-ish. The layout of the docs is very custom anyway.

@eps1lon eps1lon merged commit 0d803bf into mui:next Feb 1, 2021
@eps1lon eps1lon deleted the docs/main-max-width-font-size branch February 1, 2021 11:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accessibility a11y docs Improvements or additions to the documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants