mui / material-ui

Material UI: Comprehensive React component library that implements Google's Material Design. Free forever.
https://mui.com/material-ui/
MIT License
93.2k stars 32.08k forks source link

1 rem should be 10 px #8926

Closed Doff3n closed 6 years ago

Doff3n commented 6 years ago

I think this 1rem=10px is the defacto standard in the industry by now? You operate with a different scale, 1 rem = 16px?

Expected Behavior

1 rem = 10 px when scaling the page

Current Behavior

1 rem =16px This is the standard in browsers, but not defacto standard: https://www.sitepoint.com/understanding-and-using-rem-units-in-css/

eyn commented 6 years ago

@Doff3n. As per the article you quoted - rem is relative to root element - if you set your Body tag to have a font-size of 10px then everything will scale accordingly. This isn't an issue with material-ui although the change in the last couple of betas from using non-relative units to relative units might've caught you out.

oliviertassinari commented 6 years ago

@eyn Is correct. Also, if you want to keep this 10px font-size on the html, you can change this value in the theme: https://github.com/callemall/material-ui/blob/627c08fc9730c37f2c6a2e7663327961afdeb187/src/styles/createTypography.js#L16

I think that we should improve the comment in the code before closing this issue. (how rem works + why we use it)

Some reference: https://webdesign.tutsplus.com/tutorials/comprehensive-guide-when-to-use-em-vs-rem--cms-23984

Doff3n commented 6 years ago

In my opinion the base settings in your MUITheme should follow 1rem = 10px, in order to avoid for instance: "caption": { "fontSize": "0.75rem" } being translated by the browser to 1.2 rem.

It is more intuitive.

But I managed to inject updated rems following the 1rem=10px conventions into the MUITheme in typescript using:

const typographyOptions:TypographyOptions = {};
const caption:TypographyStyle = { 
  fontFamily:'\"Roboto\", \"Helvetica\", \"Arial\", sans-serif',
  color:'rgba(0, 0, 0, 0.54)',
  fontSize: '1.2rem',
  fontWeight:500,
  lineHeight: '1.3rem' 
};
[display4, display3 etc removed for brewity...]
 typographyOptions.caption = caption;
const theme = createMuiTheme({
    typography: typographyOptions,
   [display4, display3 etc removed for brewity...]
});

I also noticed that you use a mix of rem, px and em. And also the htmlFontSize = 16 has no unit, at the same time the font size is set to 14 in the theme? In addition there is one issue stating that you use inheritance: #8903. You might want to stick with one standard unit, and I think that rem + scaling by the root element is best practice these days. Especially since rem is more than just font sizing: https://css-tricks.com/theres-more-to-the-css-rem-unit-than-font-sizing/

But I am happy with the solution by theming, it works out great, and this is very opinion based, so you may close the issue if you disagree :) Great framework anyways and thanks you so much for the fast reply and your hard work!

oliviertassinari commented 6 years ago

@Doff3n Thanks for the feedback. I have tried to address all your concern. Let me know if you find something missing.

Doff3n commented 6 years ago

This is great, I tried to think of a way to integrate this into the framework, the pxToRem(px) nails it. I found this sass function now: https://gist.github.com/garystorey/5920051)

I could not understand before how changing the fontSize would scale the components in MUI, but this should now work.

I would also say that the PR improves the readability of the code.

We will test this in a while, and I will report back 👍

oliviertassinari commented 6 years ago

The only thing I haven't added is the px fallback. Given our supported browser range. We shouldn't need it.

Doff3n commented 6 years ago

The JSS engine should cover that in addition to vendor prefixes etc.

CssNext and CssModules already covers this.

torian257x commented 4 years ago

the example I use is::

import React from 'react';
import {createMuiTheme, ThemeProvider} from '@material-ui/core/styles';
import Typography from '@material-ui/core/Typography';

const theme = () => {
  return createMuiTheme({
    typography: {
      htmlFontSize: 10,
    },
  });
};

const FontSizeTheme: React.FC = (props) => {
  return (
      <ThemeProvider theme={theme()}>
        <Typography variant="body1" component="span">
          {props.children}
        </Typography>
      </ThemeProvider>
  );
}

export default FontSizeTheme;
TeodorDimitrov89 commented 1 year ago

To make 1rem = 10px, if anyone is still wondering how to achieve it, you need to add html { font size: 62.5%; } in css to override the browser's default default font size. And then const theme = createTheme({

typography: { fontFamily: "", htmlFontSize: 10, body1: { fontSize: "1.6rem", // this will be 16px } } });