Skip to content
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

[Typography] Can we extend Typography? #8498

Closed
1 task done
klauszhang opened this issue Oct 1, 2017 · 11 comments
Closed
1 task done

[Typography] Can we extend Typography? #8498

klauszhang opened this issue Oct 1, 2017 · 11 comments
Labels
duplicate This issue or pull request already exists

Comments

@klauszhang
Copy link
Contributor

Should Typography extended while providing extra properties in theme > typography

  • I have searched the issues of this repository and believe that this is not a duplicate.

Expected Behavior

When provide extra property in createMuiTheme > typography, it should be able to access in type of Typography

Current Behavior

Not supported

Steps to Reproduce (for bugs)

em... consider this as a feature request. we should be able to do something like following

import React from 'react';
import { MuiThemeProvider, createMuiTheme } from 'material-ui/styles';
import Typography from 'material-ui/Typography';
import Button from 'material-ui/Button';

function updateTheme() {
  return createMuiTheme({
    typography: {
      content: {
        fontFamily:'Roboto Condensed',
        fontSize: 14
      }
    }
  });
}

function TypographyTheme() {
  return (
    <MuiThemeProvider theme={updateTheme}>
      <div>
        <Typography type="title">{'this is title'}</Typography>
        <Typography type="content">{'this is the content'}</Typography>
      </div>
    </MuiThemeProvider>
  );
}

export default TypographyTheme;

Context

usually people would use Roboto along with Roboto Condensed or Noto to create a Title > Content pair

Your Environment

Tech Version
Material-UI 1.0.0-beta.13
React 16
browser n/a
etc
@klauszhang klauszhang changed the title [Typography] Should we able to extend Typography? [Typography] Can we extend Typography? Oct 1, 2017
@oliviertassinari oliviertassinari added v1 out of scope The problem looks valid but we won't fix it (maybe we will revisit it in the future) labels Oct 2, 2017
@oliviertassinari
Copy link
Member

oliviertassinari commented Oct 2, 2017

@klauszhang This is an interesting proposal but I'm afraid it won't make it given the variant values are static for the static type checking to work correctly. Also, we would require #7633.

Instead, we encourage people to write their own wrapping components on top of Material-UI

import React from 'react'
import PropTypes from 'prop-types'
import { withStyles } from 'material-ui/styles'
import { capitalize } from 'material-ui/utils/helpers'
import MuiTypography from 'material-ui/Typography'

const styles = theme => ({
  markedDisplay3Center: {
    height: 4,
    width: 73,
    display: 'block',
    margin: `${theme.spacing.unit}px auto 0`,
    background: theme.palette.secondary.main,
  },
  markedDisplay2Center: {
    height: 4,
    width: 55,
    display: 'block',
    margin: `${theme.spacing.unit}px auto 0`,
    background: theme.palette.secondary.main,
  },
  markedDisplay1Center: {
    height: 4,
    width: 55,
    display: 'block',
    margin: `${theme.spacing.unit}px auto 0`,
    background: theme.palette.secondary.main,
  },
  markedTitleLeft: {
    height: 2,
    width: 28,
    display: 'block',
    marginTop: theme.spacing.unit / 2,
    background: 'currentColor',
  },
})

const headlineMapping = {
  display4: 'h1',
  display3: 'h1',
  display2: 'h1',
  display1: 'h1',
  headline: 'h3',
  title: 'h2',
  subheading: 'h3',
  body2: 'aside',
  body1: 'p',
}

function Typography(props) {
  const { children, classes, marked, variant, ...other } = props

  return (
    <MuiTypography headlineMapping={headlineMapping} variant={variant} {...other}>
      {children}
      {marked ? (
        <span className={classes[`marked${capitalize(variant) + capitalize(marked)}`]} />
      ) : null}
    </MuiTypography>
  )
}

Typography.propTypes = {
  children: PropTypes.node,
  classes: PropTypes.object.isRequired,
  marked: PropTypes.oneOf([false, 'center', 'left']),
  variant: PropTypes.string,
}

Typography.defaultProps = {
  marked: false,
}

export default withStyles(styles)(Typography)

An alternative solution would be to expose a component factory where people can define their own type.

@oliviertassinari
Copy link
Member

An alternative solution would be to expose a component factory where people can define their own type.

We will think about this in the future if there is a strong signal for using this pattern. Please up vote the issue if you think so. I'm closing for now.

@oliviertassinari oliviertassinari added new feature New feature or request waiting for 👍 Waiting for upvotes and removed out of scope The problem looks valid but we won't fix it (maybe we will revisit it in the future) labels Oct 2, 2017
@PDXIII
Copy link

PDXIII commented Feb 6, 2018

Your suggested approach seems not to work anymore. When I try to follow it I get the desired result but with nasty error messages:

index.js:2177 
Warning: Material-UI: the key `headlineXlarge` provided to the classes property is not implemented in Typography.
You can only override one of the following: root,display4,display3,display2,display1,headline,title,subheading,body2,body1,caption,button,alignLeft,alignCenter,alignRight,alignJustify,noWrap,gutterBottom,paragraph,colorInherit,colorPrimary,colorSecondary,colorTextSecondary,colorError
__stack_frame_overlay_proxy_console__ @ index.js:2177
warning @ browser.js:49
(anonymous) @ withStyles.js:346
render @ withStyles.js:345
finishClassComponent @ react-dom.development.js:7873
updateClassComponent @ react-dom.development.js:7850
beginWork @ react-dom.development.js:8225
performUnitOfWork @ react-dom.development.js:10224
workLoop @ react-dom.development.js:10288
callCallback @ react-dom.development.js:542
invokeGuardedCallbackDev @ react-dom.development.js:581
invokeGuardedCallback @ react-dom.development.js:438
renderRoot @ react-dom.development.js:10366
performWorkOnRoot @ react-dom.development.js:11014
performWork @ react-dom.development.js:10967
requestWork @ react-dom.development.js:10878
scheduleWorkImpl @ react-dom.development.js:10732
scheduleWork @ react-dom.development.js:10689
scheduleTopLevelUpdate @ react-dom.development.js:11193
updateContainer @ react-dom.development.js:11231
(anonymous) @ react-dom.development.js:15226
unbatchedUpdates @ react-dom.development.js:11102
renderSubtreeIntoContainer @ react-dom.development.js:15225
render @ react-dom.development.js:15290
./src/index.js @ index.js:6
__webpack_require__ @ bootstrap 2b343af7df2e62478904:678
fn @ bootstrap 2b343af7df2e62478904:88
0 @ MLSTypography.js:30
__webpack_require__ @ bootstrap 2b343af7df2e62478904:678
./node_modules/ansi-regex/index.js.module.exports @ bootstrap 2b343af7df2e62478904:724
(anonymous) @ bootstrap 2b343af7df2e62478904:724

index.js:2177 
Warning: Failed prop type: Invalid prop `variant` of value `headlineXlarge` supplied to `Typography`, expected one of ["display4","display3","display2","display1","headline","title","subheading","body2","body1","caption","button"].
    in Typography (created by WithStyles(Typography))
    in WithStyles(Typography) (at MLSTypography.js:24)
    in MLSTypography (created by WithStyles(MLSTypography))
    in WithStyles(MLSTypography) (at App.js:11)
    in MuiThemeProvider (at MLSDefaultTheme.js:42)
    in MLSDefaultTheme (at App.js:10)
    in App (at index.js:6)

Any suggestions how to add own type styles tat can be used in the way like yours:

<Typography variant="headlineXlarge">Headline Xlarge</Typography>

Thank you in advance showing me the right direction.

@oliviertassinari
Copy link
Member

@PDXIII I have updated the example. But I believe you didn't copy paste the example in the first place.

@oliviertassinari oliviertassinari removed the waiting for 👍 Waiting for upvotes label Nov 30, 2019
@zeckdude
Copy link
Contributor

For anyone who needs another example, this is my implementation. It is my own Typography component which wraps the Material UI Typography component, but it defines a different variant scale and allows for a primary/secondary font and font weight options.

import React from 'react';
import PropTypes from 'prop-types';
import { mapValues } from 'lodash';
import { makeStyles } from '@material-ui/core/styles';
import MuiTypography from '@material-ui/core/Typography';

const variantMapping = {
  '2xl': {
    element: 'h1',
    styles: {
      primary: {
        fontSize: 81,
        lineHeight: '120%',
        letterSpacing: '-2px',
      },
      secondary: {
        fontSize: 81,
        lineHeight: '120%',
        letterSpacing: 0,
      },
    },
  },
  xl: {
    element: 'h2',
    styles: {
      primary: {
        fontSize: 54,
        lineHeight: '120%',
        letterSpacing: '-1px',
      },
      secondary: {
        fontSize: 54,
        lineHeight: '120%',
        letterSpacing: 0,
      },
    },
  },
  lg: {
    element: 'h3',
    styles: {
      primary: {
        fontSize: 36,
        lineHeight: '120%',
        letterSpacing: '-0.75px',
      },
      secondary: {
        fontSize: 36,
        lineHeight: '120%',
        letterSpacing: 0,
      },
    },
  },
  md: {
    element: 'h4',
    styles: {
      primary: {
        fontSize: 24,
        lineHeight: '130%',
        letterSpacing: '-0.5px',
      },
      secondary: {
        fontSize: 24,
        lineHeight: '140%',
        letterSpacing: 0,
      },
    },
  },
  sm: {
    element: 'p',
    styles: {
      primary: {
        fontSize: 16,
        lineHeight: '140%',
        letterSpacing: 0,
      },
      secondary: {
        fontSize: 16,
        lineHeight: '140%',
        letterSpacing: 0,
      },
    },
  },
  xs: {
    element: 'h5',
    styles: {
      primary: {
        fontSize: 14,
        lineHeight: '140%',
        letterSpacing: 0,
      },
      secondary: {
        fontSize: 14,
        lineHeight: '140%',
        letterSpacing: 0,
      },
    },
  },
  '2xs': {
    element: 'h6',
    styles: {
      primary: {
        fontSize: 12,
        lineHeight: '140%',
        letterSpacing: 0,
      },
      secondary: {
        fontSize: 12,
        lineHeight: '140%',
        letterSpacing: 0,
      },
    },
  },
};

const useStyles = makeStyles(theme => ({
  root: ({ variant, isBold, font }) => {
    const baseStyles = {
      fontWeight: isBold ? 700 : 'normal',
      fontFamily: font === 'secondary' ? ['"Lora"', 'sans-serif'].join(',') : null,
    };
    const variantStyles = variantMapping[variant].styles[font];

    return { ...baseStyles, ...variantStyles };
  },
}));

function Typography({ children, variant, className, component, isBold, font }) {
  const classes = useStyles({ variant, isBold, font });

  return (
    <MuiTypography
      classes={classes}
      variantMapping={mapValues(variantMapping, 'element')}
      variant={variant}
      component={component}
      className={className}
    >
      {children}
    </MuiTypography>
  );
}

Typography.propTypes = {
  children: PropTypes.node.isRequired,
  classes: PropTypes.shape({
    root: PropTypes.string.isRequired,
  }).isRequired,
  variant: PropTypes.oneOf(Object.keys(variantMapping)).isRequired,
  component: PropTypes.string,
  className: PropTypes.string,
  isBold: PropTypes.bool,
  font: PropTypes.oneOf(['primary', 'secondary']),
};

Typography.defaultProps = {
  isBold: false,
  component: null,
  className: '',
  font: 'primary',
};

export default Typography;

@zeckdude
Copy link
Contributor

zeckdude commented Mar 9, 2020

@oliviertassinari I'm just now realizing that while my example above works well, it throws a proptype error:

Invalid prop `variant` of value `md` supplied to `ForwardRef(Typography)`, expected one of ["h1","h2","h3","h4","h5","h6","subtitle1","subtitle2","body1","body2","caption","button","overline","srOnly","inherit"].

Is there any way I can get that error message not to appear?

@oliviertassinari
Copy link
Member

oliviertassinari commented Mar 20, 2020

@zeckdude As we are speaking the only way to avoid this issue is to create a wrapper component and not to spread the variant if out of the built-in supported range. However, #15573 will address it.

@oliviertassinari oliviertassinari added duplicate This issue or pull request already exists and removed new feature New feature or request labels Mar 20, 2020
@oliviertassinari
Copy link
Member

Duplicate of #15573

@oliviertassinari oliviertassinari marked this as a duplicate of #15573 Mar 20, 2020
@zeckdude
Copy link
Contributor

@oliviertassinari Can you provide me an example of the wrapper component you are referring to? I’m unsure how to implement that.

Also, any idea when that fix will be in? Is it in development?

@oliviertassinari
Copy link
Member

@zeckdude #8498 (comment)

@aldrichdev
Copy link

Can we get an updated wrapper example? makeStyles / withStyles is deprecated, no?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
duplicate This issue or pull request already exists
Projects
None yet
Development

No branches or pull requests

5 participants