-
-
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
[theme] Responsive font sizes #14573
[theme] Responsive font sizes #14573
Conversation
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.
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.
@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. |
@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 :). |
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.
|
Details of bundle changes.Comparing: 6a88e26...85d4b3c
|
edit: still looking but just noticed the automatic change in |
@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. |
@oliviertassinari
Perhaps the package needs to be broken in two, with |
@mbrookes works for me, if @oliviertassinari agrees I'll update this. Thanks for reviewing. |
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.
@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
tobody1
. 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.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
@oliviertassinari I believe you may have again misread, or misunderstood the points being made.
1 (I suspect that the association with |
|
Absolutely. That was in mind when I suggested
I couldn't think of anything better, but was hesitant to say so given the drive to keep core light... |
I have different names in mind: |
Thanks both. @mbrookes I grabbed the |
canvas is already a thing, and brush, paint and paintbrush ("painbrush", lol) all sound color related, rather than home to more generic CSS utils. |
@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
I agree. |
@mbrookes I wasn't sure why the I was aiming for this flow:
I understand we're discussing:
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).
|
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 |
@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. |
@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. |
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.
Just a couple of final suggested edits to the docs wording.
Great job @n-batalha! 🎉 When would this be merged? 😄 |
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? |
@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
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.
But I couldn't find any good literature on the topic. |
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 |
@oliviertassinari to be clear, are you suggesting to move all the code here to |
@n-batalha I'm doing a new iteration. Hold on. |
A summary of the changes:
I have tried the function with the whole documentation. It works great. Please review. @n-batalha A big thanks for pushing it! |
@oliviertassinari thanks for following up, LGTM |
This is a risky change at the given state of the release of v4. |
Follow-up of #11452: adding responsive typography styles via an optional helper. Example (documented here):
Closes #11452.
TODO
TODOs in code(descoped)