[theme] Responsive font sizes

[n-batalha]

• I have followed (at least) the PR section of the contributing guide.

Follow-up of #11452: adding responsive typography styles via an optional helper. Example (documented here):

Closes #11452.

TODO

• example algorithm in new csskit package
• demo in docs
• check/change calcs to comply with 4px MD requirement (and allow an option to ignore it)
• tests :)
• review / update mechanism to push to npmjs
• TODOs in code (descoped)

[eps1lon]

Wouldn't this be more appropriate in ´@material-ui/styles? The returntype from fluidRange` is coupled to jss so it makes more sense to include it in the package.

This is currently a Draft Pull Request. Since these are new to GitHub, not sure if you want these or not.

I would actually prefer those now. I think this makes it easier for maintainers to spot if the PR is red because it's incomplete or the PR is red and the contributor needs some help, breakage is expected or unrelated things failed.

I think I'll go with drafts as default for my own PRs (here or anywhere else) and then switch to ready once CI is green or I actually need guidance from a maintainer.

[n-batalha]

@eps1lon as for where to put this, just following suggestions here. It seemed too much but I'm new to this (*) so I don't have a strong opinion yet. Whatever you / @oliviertassinari prefer.

(*) mui and modern frontend dev in general.

[oliviertassinari]

Wouldn't this be more appropriate in ´@material-ui/styles`

@eps1lon I don't think so. This is a generic CSS-in-JS helper. It's interoperable. It's not bound to JSS. You can use it with emotion, styled-components, aphrodite, etc.

@n-batalha Thank you for starting this pull request! We can move most of our @material-ui/core/styles helpers in this package :).

[n-batalha]

I had a stab at doing the math re: the vertical 4px requirement here. Please correct me now if wrong before coding it, font sizes are a pain.

MD line height seems to be differently defined than what it is in CSS (see the image and the font baseline). I'll assume for this purpose that it's equivalent.

I'll next do the math re: vertical rhythm as I don't want to break it (if it exists on the baseline theme) with approximations above. Leaving vertical rhythm partially to the user before it gets even more complicated. Either the original font sizes and line heights result in vertical rhythm, then not applying 4px alignment preserves it (since they are all a multiple of what they were). Or the user picks font sizes and scaling that doesn't require alignment (and again having vertical rhythm, this is preserved in the scaling).

[mui-pr-bot]

Details of bundle changes.

Comparing: 6a88e26...85d4b3c

bundle	parsed diff	gzip diff	prev parsed	current parsed	prev gzip	current gzip
@material-ui/core	+0.01% 🔺	+0.01% 🔺	310,401	310,417	84,529	84,539
@material-ui/core/Paper	+0.02% 🔺	+0.02% 🔺	67,637	67,653	20,118	20,122
@material-ui/core/Paper.esm	+0.02% 🔺	+0.02% 🔺	61,002	61,017	19,040	19,044
@material-ui/core/Popper	0.00%	0.00%	28,590	28,590	10,290	10,290
@material-ui/core/Textarea	0.00%	0.00%	5,513	5,513	2,379	2,379
@material-ui/core/TrapFocus	0.00%	0.00%	3,780	3,780	1,577	1,577
@material-ui/core/styles/createMuiTheme	+0.09% 🔺	+0.09% 🔺	15,943	15,958	5,777	5,782
@material-ui/core/useMediaQuery	0.00%	+0.10% 🔺	2,106	2,106	976	977
@material-ui/lab	+0.01% 🔺	+0.01% 🔺	139,644	139,660	42,539	42,544
@material-ui/styles	0.00%	0.00%	51,165	51,165	15,151	15,151
@material-ui/system	0.00%	+0.03% 🔺	11,765	11,765	3,923	3,924
Button	+0.02% 🔺	+0.02% 🔺	85,317	85,332	25,689	25,693
Modal	0.00%	0.00%	20,365	20,365	6,695	6,695
colorManipulator	0.00%	0.00%	3,904	3,904	1,543	1,543
docs.landing	0.00%	+0.01% 🔺	51,531	51,531	11,368	11,369
docs.main	+0.43% 🔺	+0.55% 🔺	648,808	651,585	202,575	203,682
packages/material-ui/build/umd/material-ui.production.min.js	+0.01% 🔺	+0.01% 🔺	292,250	292,265	82,464	82,469

Generated by 🚫 dangerJS against 85d4b3c

[n-batalha]

@oliviertassinari any obvious step I'm missing for a new package here? Suddenly this happens but I don't yet see a connection between my change and jss-global. Removing my docs diff makes no difference. Yet the next branch is fine.

edit: still looking but just noticed the automatic change in yarn.lock on jss, must be related.
edit2: never mind, fixed

[n-batalha]

@oliviertassinari @eps1lon ready to review. I learned JavaScript and React alone two months ago, so suggest a careful read :).

I wanted to propose changing the way font style names are defined so as to not require hardcoding them in different places, but can handle in a separate MR.

[mbrookes]

Wouldn't this be more appropriate in ´@material-ui/styles`

@eps1lon I don't think so. This is a generic CSS-in-JS helper.

@oliviertassinari responsiveTypography is a theme helper, it takes a Material-UI theme object, and returns a new theme object with the typography values made responsive, according to the parameters provided.

responsiveStyles OTOH is a generic helper.

Perhaps the package needs to be broken in two, with responsiveStyles in this package (perhaps named cssutils rather than csskit), and responsiveTypography somewhere else (core/styles?)...

[n-batalha]

Perhaps the package needs to be broken in two, with responsiveStyles in this package (perhaps named cssutils rather than csskit), and responsiveTypography somewhere else (core/styles?)...

@mbrookes works for me, if @oliviertassinari agrees I'll update this.

Thanks for reviewing.

[oliviertassinari]

@n-batalha You have done a really great work! I'm very happy to see this pull request 😄. I'm sorry for the delay in the review. I wanted to make sure I had enough time to dive into it. It's important, took me 1 hour. Hopefully, building anything meaningful takes time. I have read your article: https://manishearth.github.io/blog/2017/08/10/font-size-an-unexpectedly-complex-css-property/. Oh boy, I'm grateful I didn't have to implement font size in a browser engine:

Regarding the core typography. I think that we should revamp some of it before the stable release.

• 16px is a better default than 14px. Bootstrap, material.io or even our documentation use 16px as a default font size. 14px like Ant Design is understandable as Chinese users have a different alphabet. We document 12px as the default font size for Japanese. So, I think that we should change the Typography default variant from body2 to body1. It's important!
• The line-height, the specification doesn't mention it https://material.io/design/typography/the-type-system.html#. However, it's a key element in achieving a vertical rhythm. I have only found one change that could help align on the 4px grid, the other values look good:

-    body2: buildVariant(fontWeightRegular, 14, 1.5, 0.15),
+    body2: buildVariant(fontWeightRegular, 14, 1.43, 0.15),

What do you think of it?

• I'm wondering about using unitless vs rem for the line-height. The core typography is unitless like Bootstrap or Ant Design or Gatsby. However, MCW is rem based. I think that we should stay unitless, what do you think of changing in the logic in the responsive helper?
• Regarding the best home for this logic. I'm happy to keep it under csskit. I would like to move all the theme logic there. It has multiple advantages.
  • It will encourage us to properly document the thing. We could have a new documentation top folder for it. Have documentation for each helper, with examples, etc.
  • It should be generic enough to be used outside Material-UI. People would rather import a small dependency than all our components.

[mbrookes]

@oliviertassinari I believe you may have again misread, or misunderstood the points being made.

1. This isn't a "kit". The package name could be improved. I suggest cssutils.

2. responsiveStyles() is a generic CSS-in-JS helper. It should remain in cssutils.

3. responsiveTypography() is a theme helper. It is tied to the Material-UI theme. Therefore, if the intention is that cssutils provides generic CSS utils that can (theoretically¹) be used outside of Material-UI, then responsiveTypography()doesn't belong in cssutils. Where do you suggest?

4. This is already a separate package, so I don't understand your repeated point that cssutils (containing responsiveStyles()) should be separate from core. No-one is suggesting otherwise.

¹ (I suspect that the association with @material-ui will be a barrier to adoption outside of the Material-UI community).

[oliviertassinari]

1. I'm happy to find a better name for the package, like overlayer. I would like to include the color helpers too.
2. 👍
3. This one is interesting. Maybe @material-ui/core in this case?
4. 👍

[mbrookes]

1. I have no idea what overlayer means, sorry!

color helpers too.

Absolutely. That was in mind when I suggested cssutils, as you'd already pointed to this PR as the potential home for the colorManipulator utils. The colorManipulator function renaming discussion is a precursor to moving them here, once this PR is merged.

3. Maybe @material-ui/core in this case?

I couldn't think of anything better, but was hesitant to say so given the drive to keep core light...

[oliviertassinari]

I have different names in mind: @material-ui/canvas, brush, paint, painbrush.
One thing we could do to mitigate the bundle size issue with core is to display on the documentation the cost of each component import.

[n-batalha]

Thanks both. @mbrookes I grabbed the css-kit name from #11452 so deferring to @oliviertassinari, as not sure of the plans. Once you both agree on naming I'll change it!

[mbrookes]

I have different names in mind: @material-ui/canvas, brush, paint, painbrush.

canvas is already a thing, and brush, paint and paintbrush ("painbrush", lol) all sound color related, rather than home to more generic CSS utils.

[mbrookes]

I just realized you answered above, in that esdoc is to annotate API's, I'll look into it.

@n-batalha I did say esdoc, but I was mistakenly under the impression it could generate markdown in addition to html.

Something similar to https://www.npmjs.com/package/jsdoc-to-markdown might help.

Creating the script separately so that this PR can be merged would be fine by me.

The idea would be to generate the docs in the README, so that it's viewable in the repo and on npmjs.org. That way we avoid having an additional page in the main docs. (We can link to it on the "related projects" page).

We don't currently automate the documentation of non-component APIs, but the colorManipulator functions have jsdoc comments already, so when they move to this package, your script will be able to merge them into the README.

note: I think only responsiveProperty in css-utils is worth exposing at the moment

I agree.

[n-batalha]

@mbrookes I wasn't sure why the README.md but I understand now that the package docs in npmjs.com only show whatever is in that file, as opposed to show a built doc on demand at publish time.

I was aiming for this flow:

1. dev makes code changes and commits
2. in CD, docs get generated and published (e.g. netlify static site)

I understand we're discussing:

1. dev makes code changes
2. dev runs doc generation to add to the repo (README.md), and commits
3. CI checks if doc generation is idempotent

lodash seems to go halfway:

1. dev makes code changes and commits
2. when publishing versions to release, some script seems to generate different READMEs for npm releases, generates the API docs, commits, makes a tag to be used on the releases in npmjs.com (e.g. 4.17.11-npm). devs not needing to worry on doc generation.

I implemented the second flow above (with the test), and eventually we can move docs to a static site or lodash's approach if you prefer.

A slight problem with JSDoc is that it doesn't seem to handle destructured arguments very nicely (example committed).

In the meantime, some new yarn issue I need to spend time debugging (edit: fixed) 🤕

[mbrookes]

A slight problem with JSDoc is that it doesn't seem to handle destructured arguments very nicely

If esdoc has better support for this or other jsdoc limitations, I found this plugin to generate markdown with esdoc: https://www.npmjs.com/package/esdoc-publish-markdown-plugin

[n-batalha]

@mbrookes Thanks! But TL;DR of the below is I think your suggestion of jsdoc-to-markdown is quite good. Suggest we use it! (already in PR)

---

Detailed reply:

My comment was misleading, I didn't check if other systems handled destructured args better. I have another branch with ESDoc and can confirm it's like JSDoc. The default html output is quite nice.

But I assumed you had excluded the ESDoc Markdown plugin because it's a PoC with no updates in a year, perhaps it's an issue? The ESDoc/JSDoc API seem quite similar if not mostly the same, so if wanting to change later if the plugin improves it likely won't be hard.

lodash uses this but it seems to lack some functionality, such as the templating ability of what you sent earlier, and less developed and used. documentationjs outputs to markdown and has good adoption and many features but seems less simple to generate the README.md as it is now. There are other tools that can be looked at much later but the beauty is that most tools seem to use JSDoc annotation making swapping tools relatively easy and less of a commitment/investment at an early stage.

[mbrookes]

@n-batalha The current output looks fine to me, and the integration nice and straightforward. As you say, we can always swap it out later if something better turns up.

I think we're good to go.

[mbrookes]

Just a couple of final suggested edits to the docs wording.

[PolGuixe]

Great job @n-batalha! 🎉

When would this be merged? 😄

[n-batalha]

Thanks @PolGuixe! I'm just a contributor, even though it's approved the privilege of merging is restricted to Committers.

I presume waiting to know @oliviertassinari's thoughts if it's still preferable to do the opposite approach. @oliviertassinari: sorry still haven't played with Bootstrap's RFS! Is the only difference the one you mentioned above?

[oliviertassinari]

@n-batalha I have rebased the pull request on the next branch. I was hoping the bundle size would be reported, but it's not the case. We are introducing new CSS helpers. Polished has a similar mission. I was curious to see if we could use their helper instead of publishing @material-ui/css-utils. I have created #15540 to explore this option.

I presume waiting to know @oliviertassinari's thoughts if it's still preferable to do the opposite approach. #14573 (comment)

We both agree that the fonts on Mobile should be smaller than on Desktop. The important optical characteristic is the eye angle resolution. The closer the text is, the smaller it can be. The Material Design specification doesn't mention any font responsiveness, it's probably not something they encourage. So, we have to look broader, what's the best practice used in the industry? Should we consider the Material Design specification values primarily for desktop or for mobile? I also think that only the large font size should be responsive, font size below 20px should be ignored.
I think that we should do the opposite considering that:

• this Stack Overflow question asks for smaller font size on mobile (not a larger font size on dekstop) https://stackoverflow.com/questions/52472372/responsive-typography-in-material-ui.
• viewing https://next.material-ui.com/style/typography/#component on mobile shows too large headers.
• Bootstrap does it

But I couldn't find any good literature on the topic.

[oliviertassinari]

Polished has a similar mission. I was curious to see if we could use their helper instead of publishing @material-ui/css-utils. I have created #15540 to explore this option.

After the point raised by @eps1lon, I think that the pull-request can focus on providing a responsive theme helpers. We don't need to release the @material-ui/css-utils package at the same time. I'm grateful for the discussing and the automatic documentation work. It helps to mature the reflection.
We can continue the CSS utils effort in a different pull request. It's still unclear whether the best option is to use polished or not.

[n-batalha]

@oliviertassinari to be clear, are you suggesting to move all the code here to @material-ui/core/styles and maybe once the adoption of polished is clarified, maybe swap responsiveProperty by fluidRange?

[oliviertassinari]

@n-batalha I'm doing a new iteration. Hold on.

[oliviertassinari]

A summary of the changes:

• Hold on to the idea of publishing @material-ui/css-utils. 1. This is relatively independent for the feature we want to implement, we can handle in a different pull request. 2. Maybe we should compound our energy around polished.
• Change the API to be simpler to use (use your original theme wrapping strategy). I have renamed it "responsive font sizes" to be closer to what this function does.
• Change the resizing strategy. I believe people want to reduce the font size on mobile, not increase it on desktop. I have also ignored any font size under 1rem, I had to change the scaling factor configuration accordingly.

I have tried the function with the whole documentation. It works great. Please review.

@n-batalha A big thanks for pushing it!

[n-batalha]

@oliviertassinari thanks for following up, LGTM

[oliviertassinari]

This is a risky change at the given state of the release of v4.
However, given the feedback we still collect around v4.0.0-beta.0, we might need 3 weeks instead of 2 to cut v4 stable.
I have asked React Europe, they accepted a lightning talk on the topic of Material-UI v4 on May 23rd. I hope this change won't need any more iterations. I apologize in advance if it does.