Fix the feedbacks of @eps1lon in the review

mui · Sep 25, 2018 · 3309e15 · 3309e15
1 parent cb8d79a
commit 3309e15
Show file tree

Hide file tree

Showing 4 changed files with 35 additions and 48 deletions.
diff --git a/docs/src/modules/components/AppSearch.js b/docs/src/modules/components/AppSearch.js
@@ -91,7 +91,7 @@ const styles = theme => ({
         ...theme.typography.h6,
       },
       '& .algolia-docsearch-suggestion--text': {
-        ...theme.typography.body1,
+        ...theme.typography.body2,
       },
       '& .ds-dropdown-menu': {
         boxShadow: theme.shadows[1],

diff --git a/packages/material-ui/src/ListItemText/ListItemText.js b/packages/material-ui/src/ListItemText/ListItemText.js
@@ -73,7 +73,7 @@ function ListItemText(props, context) {
   if (secondary != null && secondary.type !== Typography && !disableTypography) {
     secondary = (
       <Typography
-        variant="body1"
+        variant="body2"
         className={classNames(classes.secondary, {
           [classes.textDense]: dense,
         })}

diff --git a/packages/material-ui/src/ListItemText/ListItemText.test.js b/packages/material-ui/src/ListItemText/ListItemText.test.js
@@ -75,7 +75,7 @@ describe('<ListItemText />', () => {
       const wrapper = shallow(<ListItemText secondary="This is the secondary text" />);
       assert.strictEqual(wrapper.children().length, 1, 'should have 1 child');
       assert.strictEqual(wrapper.childAt(0).type(), Typography);
-      assert.strictEqual(wrapper.childAt(0).props().variant, 'body1');
+      assert.strictEqual(wrapper.childAt(0).props().variant, 'body2');
       assert.strictEqual(
         wrapper.childAt(0).props().color,
         'textSecondary',
@@ -122,7 +122,7 @@ describe('<ListItemText />', () => {
       );
 
       assert.strictEqual(wrapper.childAt(1).type(), Typography);
-      assert.strictEqual(wrapper.childAt(1).props().variant, 'body1');
+      assert.strictEqual(wrapper.childAt(1).props().variant, 'body2');
       assert.strictEqual(wrapper.childAt(1).props().color, 'textSecondary');
       assert.strictEqual(
         wrapper
@@ -163,7 +163,7 @@ describe('<ListItemText />', () => {
     );
 
     assert.strictEqual(wrapper.childAt(1).type(), Typography);
-    assert.strictEqual(wrapper.childAt(1).props().variant, 'body1');
+    assert.strictEqual(wrapper.childAt(1).props().variant, 'body2');
     assert.strictEqual(wrapper.childAt(1).props().color, 'textSecondary');
     assert.strictEqual(
       wrapper

diff --git a/packages/material-ui/src/styles/createTypography.js b/packages/material-ui/src/styles/createTypography.js
@@ -10,12 +10,13 @@ const caseAllCaps = {
   textTransform: 'uppercase',
 };
 
+const defaultFontFamiliy = '"Roboto", "Helvetica", "Arial", sans-serif';
+
 /**
  * @see @link{https://material.io/design/typography/the-type-system.html}
  * @see @link{https://material.io/design/typography/understanding-typography.html}
  */
 export default function createTypography(palette, typography) {
-  const defaultFontFamiliy = '"Roboto", "Helvetica", "Arial", sans-serif';
   const {
     fontFamily = defaultFontFamiliy,
     // The default font size of the Material Specification.
@@ -50,52 +51,39 @@ export default function createTypography(palette, typography) {
   );
 
   const coef = fontSize / 14;
-
-  const letterSpacingToEm = (tracking, spSize) => {
-    return `${(tracking / spSize) * coef}em`;
-  };
-
-  const pxToRem = size => {
-    return `${(size / htmlFontSize) * coef}rem`;
-  };
-
-  const propertiesForVariant = (weight, size, letterSpacing, casing) => {
+  const letterSpacingToEm = (tracking, spSize) => `${(tracking / spSize) * coef}em`;
+  const pxToRem = size => `${(size / htmlFontSize) * coef}rem`;
+  const buildVariant = (fontWeight, size, lineHeight, letterSpacing, casing) => ({
+    color: palette.text.primary,
+    fontFamily,
+    fontWeight,
+    fontSize: pxToRem(size),
+    // Unitless following http://meyerweb.com/eric/thoughts/2006/02/08/unitless-line-heights/
+    lineHeight,
     // The letter spacing was designed for the Roboto font-family. Using the same letter-spacing
     // across font-families can cause issues with the kerning.
-    const robotoStyles =
-      fontFamily === defaultFontFamiliy
-        ? { letterSpacing: letterSpacingToEm(letterSpacing, size) }
-        : {};
-
-    return {
-      color: palette.text.primary,
-      fontFamily,
-      fontSize: pxToRem(size),
-      fontWeight: weight,
-      ...robotoStyles,
-      ...casing,
-      ...allVariants,
-    };
-  };
+    ...(fontFamily === defaultFontFamiliy
+      ? { letterSpacing: letterSpacingToEm(letterSpacing, size) }
+      : {}),
+    ...casing,
+    ...allVariants,
+  });
 
-  /* eslint-disable key-spacing, no-multi-spaces */
-  // prettier-ignore
   const nextVariants = {
-    h1: propertiesForVariant(fontWeightLight, 96, -1.5),
-    h2: propertiesForVariant(fontWeightLight, 60, -0.5),
-    h3: propertiesForVariant(fontWeightRegular, 48, 0),
-    h4: propertiesForVariant(fontWeightRegular, 34, 0.25),
-    h5: propertiesForVariant(fontWeightRegular, 24, 0),
-    h6: propertiesForVariant(fontWeightMedium, 20, 0.15),
-    subtitle1: propertiesForVariant(fontWeightRegular, 16, 0.15),
-    subtitle2: propertiesForVariant(fontWeightMedium, 14, 0.1),
-    body1Next: propertiesForVariant(fontWeightRegular, 16, 0.5),
-    body2Next: propertiesForVariant(fontWeightRegular, 14, 0.25),
-    buttonNext:  propertiesForVariant(fontWeightMedium, 14, 0.75, caseAllCaps),
-    captionNext: propertiesForVariant(fontWeightRegular, 12, 0.4),
-    overline:  propertiesForVariant(fontWeightRegular, 10, 1.5, caseAllCaps),
+    h1: buildVariant(fontWeightLight, 96, 1, -1.5),
+    h2: buildVariant(fontWeightLight, 60, 1, -0.5),
+    h3: buildVariant(fontWeightRegular, 48, 1.04, 0),
+    h4: buildVariant(fontWeightRegular, 34, 1.17, 0.25),
+    h5: buildVariant(fontWeightRegular, 24, 1.33, 0),
+    h6: buildVariant(fontWeightMedium, 20, 1.6, 0.15),
+    subtitle1: buildVariant(fontWeightRegular, 16, 1.75, 0.15),
+    subtitle2: buildVariant(fontWeightMedium, 14, 1.57, 0.1),
+    body1Next: buildVariant(fontWeightRegular, 16, 1.5, 0.5),
+    body2Next: buildVariant(fontWeightRegular, 14, 1.42, 0.25),
+    buttonNext: buildVariant(fontWeightMedium, 14, 2.57, 0.75, caseAllCaps),
+    captionNext: buildVariant(fontWeightRegular, 12, 1.66, 0.4),
+    overline: buildVariant(fontWeightRegular, 12, 2.66, 1.5, caseAllCaps),
   };
-  /* eslint-enable key-spacing, no-multi-spaces */
 
   // To remove in v4
   const oldVariants = {
@@ -196,7 +184,6 @@ export default function createTypography(palette, typography) {
 
   return deepmerge(
     {
-      letterSpacingToEm,
       pxToRem,
       round,
       fontFamily,