Use rem instead of px units

mifrej commented 4 years ago

[x] The issue is present in the latest release.
[x] I have searched the issues of this repository and believe that this is not a duplicate.

In order to get fully fluent responsive experiences in the MUI components I setup all "sizing properties" to apply em unit if naked number is used as a value for them.

Current Behavior 😯

I passed my custom settings to defaultUnit plugin in JSS with accordance to documentation
Then I feed StylesProvider with such customized preset:
Theme option for htmlFontSize I set to 10.
And my reset CSS has

As a result, some of the properties in the default theme (which are originally implemented with unitless values) are becoming oversized. Exemplary original styles implementation chunk of code:

This is happening because the implicit output unit (while using numbers only) for "sizes like properties" e.g. padding, margin is preassumed as px.

So when I have margin: 16 it becomes 16em in my scenario. Which produces 160px overblown margins, paddings, and so on in the default theme. Like for example: This enforces a lot of tedious override procedures in the theme, like:

Expected Behavior 🤔

For the originally set unitless values in the default theme I would expect one of scenario (starting from most appreciated):

The values are recalculated to the respective defaultUnit settings in the JSS. So when I set defaultUnit to em for some CSS properties, the original implementation should recalculate all the unitless values based on htmlFontSize property in theme. Something like:
```
if (customDefaultUnit === 'em') unitlessProps.map(prop => (prop / theme.htmlFontSize);
```
It's a pseudo code to get the basic concept. This scenario would create full responsive (proportional sizing) across all the styling in the default theme.
Default unitless props are becoming hardcoded to pixel-units in case if defaultUnit plugin is changed. This would make some theme props output as pixel-sized, which would create an inconsistent sizing pattern. But at least it would not break the theme while customizing the defaultUnit JSS preset.
Make the unitless concept of theming out of the theming, and make everything pixel-based. Very grim scenario but at least consistent and not brittle.

Steps to Reproduce 🕹

I hope the above explanation is enough. I will provide a working example if necessary.

Context 🔦

CSS theme that uses only proportional sizing (em, rem %) in order to get the best responsive UI experiences and simplify the process of customizing styles.

Currently, part of the API support this task like font sizing but some of the props (as described above) produce fixed pixel values which output as an inconsistent theme (some values are em and some px). This enforces many tedious overrides in theme in order to get proper responsive "fluid" results.

I don't know if this type of issue "the Bug" (I guess it is something between the Bug and request to Change functionality) applies to the described scenario. So please amend accordingly. I might, of course, miss some documentation and do things just wrong. So please direct in the better implementation suggestions.

Your Environment 🌎

Tech	Version
Material-UI	v4.9.14
React	v16.13.1
Browser	["last 1 version", "> 0.5%", "not dead"]
TypeScript	v3.8.3

eps1lon commented 4 years ago

I wasn't aware of that plugin. I think there's little harm in adding px to all unitless numbers in our styles. Would be nice if you could work on a PR that removes unitless numbers from our styles and we'll see if our visual regression tests break.

mifrej commented 4 years ago

jss-default-unit plugin is listed in MUI docs. So yes, it is embedded in the default preset of MUI JSS plugins and I guess there is the reason for it.

It would be great to know what was the purpose of adding the unitless values in many places of MUI styles in the first place? I don't want to throw it out without getting generic idea behind it. As I mentioned it is the last resort scenario, that would hardcode MUI styling system to pixels completely.

I would rather go (even if it could be the longer way) to something alike (pxToRem) utility for typography, we have for font sizes in the MUI theme. But here it would be applicable for all unitless values. Something like unitelessTransformFm property which could be undefined by default. So the developer could assign a custom function for recalculating unitless values to one's will.

eps1lon commented 4 years ago

It would be great to know what was the purpose of adding the unitless values in many places of MUI styles in the first place?

Probably premature optimization to save bytes from 'px'. This is negligible though.

As I mentioned it is the last resort scenario, that would hardcode MUI styling system to pixels completely.

This was already the intention before.

I would rather go (even if it could be the longer way) to something alike (pxToRem) utility for typography, we have for font sizes in the MUI theme. But here it would be applicable for all unitless values.

We weren't talking about font related sizes. Obviously they should use already existing helpers.

Let's not get ahead of ourselves and start with spacing.

oliviertassinari commented 4 years ago

@mifrej I don't follow. What's the use case for using the relative em unit systematically? I would expect that you are looking for a customized version of the spacing helper, as we document to match Bootstrap:

const theme = createMuiTheme({
  spacing: factor => `${0.25 * factor}rem`, // (Bootstrap strategy)
});

mifrej commented 4 years ago

@mifrej are you sure about forcing the font size to 10px on the HTML element? We discourage this approach for a11y concerns in the documentation.

I'm setting it to 62.5% and then all the typography elements are easier to relate to that. So that p: { font-size: 1.6rem } which then translate to 16px (easier to calculate). Here is an article on the topic. The 62.5% method is perfectly eligible for a11y. Users has control over scaling the font-sizing.

Also, I don't follow. What's the use case for using the relative em unit systematically?

Proportional sizing (whichever form em, rem, %) is giving fully fluid proportional scaling of the interface.

Explained in details here

So while using proportional units systematically, resizing interface programmatically is a matter to change the container sizes which then will translate gracefully to proportional resizing of the inner elements.

To give the real use case for the right understanding. I was working on the web product (educational platform) where the decision was made to change font-sizing and improve spacing across the UI, in order to adapt to modern standards. All existing legacy code was based in pixel unit (obviously 😬). In order to adapt the sizing, the team spent months to propagate it correctly. It was a project on its own, named "Fonts Bigger". Each element of the UI needed to be readjusted. All tree in the DOM with all its margins paddings, and so on. As the px sizing is fixed and is not cascading across the DOM.

So the moment you use px unit somewhere, know that in case of need to adapt interface gracefully, you create a maintenance dept.

This project I mentioned, would have not existed if the proper proportional sizing was implemented in the first place.

I'm sorry for prolonging this comment but I hope we are going to bend over more on this aspect and implement solutions supporting the best practices possible. Or at least discuss it more thoroughly as I see there is some accidental choices:

Probably premature optimization to save bytes from 'px'. This is negligible though.

I was deducing this has actually some thorough background in managing the units in the system.

Looking at Foundation framework. They designed the relative sizing the right way from the beginning. I cannot find pixel-based sizing in their UI (there might be exceptions of course, like border-width for e.g.).

I would expect that you are looking for a customized version of the spacing helper, as we document to match Bootstrap

I haven't yet grasped fully the intention of the spacing helper. But I feel it is redundant, while the right proportional sizing unit is in place.

joshwooding commented 4 years ago

It would be great to know what was the purpose of adding the unitless values in many places of MUI styles in the first place?

Probably premature optimization to save bytes from 'px'. This is negligible though.

I use it for ease of use and it's quicker to type. I'm not strongly attached to it though.

oliviertassinari commented 4 years ago

@mifrej Thanks for the detailed answer and pointing out how my previous statement was wrong.

It seems that neither styled-components nor emotion allows customizing the default unit, they both use px so I would be in favor of not supporting it either, for simplicity. Closing as "won't fix".

mifrej commented 4 years ago

Then this won't change the fact that there is something to clean up for clarity and consistency's sake (scenario 3 I mentioned at the beginning).

If the defaultUnit JSS plugin is not used with awareness (accidentally), then it should be gone from preset.
Values intended to be pixels should stay pixels (unless if I'm going to override them with the theme overrides).

With the proportional unified sizing, I hope I at least put some awareness that this designing of UI would make MUI more versatile. And this is is the ultimate goal of what I see the roadmap on v5 and further major versions. Most used UI libraries are using this approach as a default. Bootstrap made a huge "rewrite" to get into the proportion sizing in from v3 to v4. Foundation was using it always thus being most usability accessibility friendly. Let's follow the great heritage they have.

oliviertassinari commented 4 years ago

@mifrej There are a couple of different topics bundled at once here.

1. JSS default unit plugin

The default unit plugin of JSS is used, we need it to get the px conversion of unitless values. My point was that I don't think it's worth supporting different configurations because it won't be supported once we support styled-components or emotion. Both of these libraries assume px is the default unit, I think it should always stay true.

2. Changing the unit we use for spacing

Should we use px, em or rem? Have run a quick benchmark on this matter:

GitHub: px
Bootstrap: rem/em
new facebook.com: px
fluent UI: px
foundation: rem/em
tailwind: rem

Would you confirm that the incentive for using rem over pixel is exclusively to have the spacing change when end-users customize their base font size? For instance, when going from 16px to 20px? This leads to the following question. When do we want the base font size to change the spacing between elements too?

Regarding the incentive for using em, it seems to be about easier scaling when changing the font-size. When do we want it?

3. Bootstrap migration

Bootstrap made a huge "rewrite" to get into the proportion sizing in from v3 to v4.

Do you have more resources on this effort? I could find https://github.com/twbs/bootstrap/issues/1943#issuecomment-3942792. They seem to be much more into rem than em. Any idea why?

4. Foundation

I have a curiosity question on this one. 10 years ago, Zurb Foundation was the leading UI library. Today, it has faded and is almost forgotten. Do you know why it happened? What made Bootstrap "took over" so drastically and quickly?

mifrej commented 4 years ago

1. JSS default unit plugin

The default unit plugin of JSS is used, we need it to get the px conversion of unitless values. My point was that I don't think it's worth supporting different configurations because it won't be supported once we support styled-components or emotion. Both of these libraries assume px is the default unit, I think it should always stay true.

Got it! Are you going to drop support for JSS completely with v5 or later? Asking just to know and maybe start using styled-components whenever possible already.

2. Changing the unit we use for spacing

Would you confirm that the incentive for using rem over pixel is exclusively to have the spacing change when end-users customize their base font size? For instance, when going from 16px to 20px? This leads to the following question. When do we want the base font size to change the spacing between elements too?

rem is a simplified version of em one may say. So it with rem you have one point of relative reference, wherewith em the first (described with sizing properties) immediate parent is the relative point.

So using em is more powerful (you can set different points of reference to make changes in a different point in DOM tree). But it is harder to grasp.

rem relate to root element always, but still gives superior control over the UI system then pixels. What I mean with the pixel unit you are just stuck. In order to adapt your design programmatically (I'm not talking about zooming browser here but changing the values in your code, different theme for example or adapting UI style guide), one must amend all the pixel occurrences in the code to make proportional scaling of UI to work

So in case to rem you control scale relatively to the one root element (html, :root).

With em there is way more control for component relational sizing, however more difficult to setup.

Any scenario relational sizing is superior from keeping fixed-pixel based sizing which is hard to opt-out from

Uniformed relative sizing is not only about font-size. It is applicable to other properties, like margin, padding, width. Only applying relative sizing to all these properties makes the UI "elastic"

Regarding the incentive for using em, it seems to be about easier scaling when changing the font-size. When do we want it?

It is mostly about proper scaling of UI and easier maintainability. For e.g., if I want to change the size of the button in my UI, I don't want to run through all its CSS properties and amend pixels all over the place. I just change its font-size and the other properties adapt proportionally. So maring-bottom: 0.5em is then proportionally more, padding: 0.5em 1em; is getting more space proportionally. This won't happen when I have everything px based. And now whenever MUI uses px values in theme I must override them to em (or rem whatever scenario chosen). Once more I put the link to this article explaining the concept in detail. https://css-tricks.com/building-resizeable-components-relative-css-units/

3. Bootstrap migration

Bootstrap made a huge "rewrite" to get into the proportion sizing in from v3 to v4.

Do you have more resources on this effort? I could find twbs/bootstrap#1943 (comment). They seem to be much more into rem than em. Any idea why?

rem is easier to apply as mentioned above. But also makes em application easier if someone wishes. As if it was "huge rewrite" maybe I'm a bit mouthful, but that's the assumption I made based on how much pixel dependent they were in v3.

4. Foundation

I have a curiosity question on this one. 10 years ago, Zurb Foundation was the leading UI library. Today, it has faded and is almost forgotten. Do you know why it happened? What made Bootstrap "took over" so drastically and quickly?

I think Bootstrap was always more popular. And why was/is that that Bootstrap has more popularity? I think is just like evolution. Some just pick up at a better place and time. From my experience, the Zurb got attention later and started getting more appreciation while Frontend community was getting more awareness about accessibility and usability where Bootstrap was not at its greatest from the start. I migrated few projects to Zurb Foundation at some point for the sake of better UI structure and maintainability and never regret that.

But what happened after React came into play? IMHO They lost traction of the importance of JS frameworks and until they realized, it was too late to start implementing a real component-driven, full-fledged JS design system. It's a pity though. They have great input into the market and this should be appreciated.

Important now is to get the best heritage from all "competition" (related products). Implementing in pixels is basically withholding you greatly form using other forms of relative sizing.

Implementing rem based sizing is still good to continue with "pixel thinking" (1rem = 10px, then 1.6rem is 16px) and allow to follow with em if one wish.

oliviertassinari commented 4 years ago

1. JSS

Are you going to drop support for JSS completely with v5 or later?

The plan is to either keep @material-ui/styles or to redirect to react-jss (which is very close) in v5. We don't plan to break people's code on the style end of things, the upgrade should be relatively easy.

How would you picture the migration from our side? Any diff code example?

mifrej commented 4 years ago

Most browsers today have the base font-size set to 16px. Respecting that (which would make the most sense) we can utilize the remCalc helper to then use for all the sizing values in styles.

So exemplary I took the Button component and went through the styles to adapt it.

diff --git a/packages/material-ui/src/Button/Button.js b/packages/material-ui/src/Button/Button.js
index fe392b926..d514a0910 100644
--- a/packages/material-ui/src/Button/Button.js
+++ b/packages/material-ui/src/Button/Button.js
@@ -6,13 +6,17 @@ import { fade } from '../styles/colorManipulator';
 import ButtonBase from '../ButtonBase';
 import capitalize from '../utils/capitalize';

-export const styles = (theme) => ({
+export const styles = (theme) => {
+  /*
+    @base should be taken from `htmlFontSize` if treated as root rem font-size
+  */
+  const remCalc = (size, base = 16) => `${size / base}rem`;
+  return {
   /* Styles applied to the root element. */
   root: {
     ...theme.typography.button,
     boxSizing: 'border-box',
-    minWidth: 64,
-    padding: '6px 16px',
+    padding: `${remCalc(6)} ${remCalc(16)}`,
     borderRadius: theme.shape.borderRadius,
     color: theme.palette.text.primary,
     transition: theme.transitions.create(['background-color', 'box-shadow', 'border'], {
@@ -42,7 +46,7 @@ export const styles = (theme) => ({
   },
   /* Styles applied to the root element if `variant="text"`. */
   text: {
-    padding: '6px 8px',
+    padding: `${remCalc(6)} ${remCalc(8)}`,
   },
   /* Styles applied to the root element if `variant="text"` and `color="primary"`. */
   textPrimary: {
@@ -68,8 +72,8 @@ export const styles = (theme) => ({
   },
   /* Styles applied to the root element if `variant="outlined"`. */
   outlined: {
-    padding: '5px 15px',
-    border: `1px solid ${
+    padding: `${remCalc(5)} ${remCalc(15)}`,
+    border: `1px solid ${ // for border width px are ok most of the time as we most likely don't want to scale borders (exception)
       theme.palette.type === 'light' ? 'rgba(0, 0, 0, 0.23)' : 'rgba(255, 255, 255, 0.23)'
     }`,
     '&$disabled': {
@@ -224,10 +228,10 @@ export const styles = (theme) => ({
   /* Styles applied to the startIcon element if supplied. */

I didn't use the existing theme helper pxToRem as it has some coef factor I don't get, but I guess we could keep such helper in theme and propagate htmlFontSize prop as the base argument for all sizes in styles.

These adaptations would need to be applied throughout all existing components and then used for future ones.

When now I look at it it wouldn't be actually that painful to do. Would it?

oliviertassinari commented 4 years ago

@mifrej Thanks for the diff, it could be interesting to further explore this change for v5.

hadasmaimon commented 4 years ago

This is a very important point! and a very simple fix has been proposed (for changes to all files - you can find an even easier way to do this) Waiting for your fix, thanks!

vdjurdjevic commented 3 years ago

It would be great if rems become the default in V5. I am also always using rems for everything, and it's annoying to always use helper function like: <Box sx={{ width: pxToRem(100)}} /> all the time.

oliviertassinari commented 3 years ago

@vdjurdjevic The system doesn't transform the width, height CSS properties. I'm rereading the thread, and I can't find a reason why rem is superior to px. So far, we have heard:

It allows to control scale relative to the one root element (html, :root) => the theme gives the same capability.
else?

Designers seem to use pixels as their main unit. Using rem creates an indirection.

vdjurdjevic commented 3 years ago

If the user changes the base font in browser settings to let's say 24px, with rems everything would follow up. If you have fixed width containers for example, and use rem for fonts only, the text gets bigger, a container doesn't. Another trick I use with rems is setting media query to just scale everything up for big displays (like TVs):

@media screen and (min-width: 1921px) {
    html {
        font-size: calc(100% + 0.3vw);
    }
}

oliviertassinari commented 3 years ago

If the user changes the base font in browser settings to let's say 24px, with rems everything would follow up. If you have fixed width containers for example, and use rem for fonts only, the text gets bigger, a container doesn't

@vdjurdjevic Why should it follow in the first place? Would it look and feel better?

vdjurdjevic commented 3 years ago

Let's say you have a sidebar and main content layout. You fix the sidebar to 300px, and the main content takes the rest of the space. And the user increases font size by 50%. A sidebar will start to wrap text or cause overflow. I may be wrong, but it seems logical that if a user has issues with sight, he would like to zoom everything, not just text. Now, it can be done with browsers zoom (ctrl +), that's exactly the behavior, but there are still users that use font size settings to do that (browser zoom feature is newer). Take VSCode for example, when you zoom it, you zoom everything.

oliviertassinari commented 3 years ago

@vdjurdjevic I'm not sure. I don't think that it's obvious. When I change the default body font-size from (default) 16px to 24px (very large) on facebook.com, only the font-size change. Aren't the OS magnifier and the browser zoom level better tailored for zooming everything?

Looking at major websites like Google, Facebook, or design-oriented companies like Stripe or Apple and looking at what they use => pixels for margins, padding, etc, it makes me seriously wonder if rem should be the default.

vdjurdjevic commented 3 years ago

That's why I said I may be wrong :) I am really not sure about this. I may consider switching back to pixels if that's what Google, Facebook, Stripe, and Apple are doing :)

oliviertassinari commented 3 years ago

Looking back at the initial pain point of the author of this issue, it seems that the discussion has shifted from:

1. I'm configuring JSS to handle unitless values as rem instead of px, it breaks because Material-UI uses unitless everywhere

to

2. Should Material-UI use rem by default?

On the first pain point, we are moving to emotion/sc. I couldn't find anything about changing the default unit with sc. In emotion, I could find https://github.com/emotion-js/emotion/issues/977 around how it's problematic. It might be possible to implement with a custom stylus plugin, but it seems that it would issue with dependencies, exactly what this issue is about. It will probably be worse as JSS was only used by Material-UI, emotion and sc are more widely adopted by dependencies.

On the second point, https://github.com/mui-org/material-ui/issues/21030#issuecomment-761556927 raises concerns about either it would be an improvement or the opposite.

I'm closing, it's a temporary no. If new information is brought to life, we can resume the discussion :). Thanks for participating!

tkodev commented 2 years ago

I think rem should not be the default value for spacings, margins, widths, etc, because of a few things:

accessibility users often have their font size set differently than the default 14-16 px, this is NOT the same as page zoom and we shouldn't treat it as such. https://medium.com/@sascha.wolff/dont-use-rem-em-for-paddings-margins-and-more-94e19026b000
using material ui to build injected dom elements, such as browser extensions will tie these element's visuals to the page's default font size. This is not ideal. While they can use things like shadow-dom, it's not always something that's possible, depending on the application.

KingScooty commented 2 years ago

Hmm... i was always under the impression rems for vertical rhythm were actually important for accessibility, as it kept everything equally spaced when zooming. So margin tops and bottoms, padding tops and bottoms, font size. Horizontal aspects weren't as relevant.

Happy to be corrected here! I'm just rather surprised that there's notion to only limit rems to font sizes...

mui / material-ui