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

[Styles] ThemeManager cannot override default Typography ? #1915

Closed
pandaiolo opened this issue Oct 19, 2015 · 16 comments · Fixed by #7248
Closed

[Styles] ThemeManager cannot override default Typography ? #1915

pandaiolo opened this issue Oct 19, 2015 · 16 comments · Fixed by #7248
Labels
new feature New feature or request

Comments

@pandaiolo
Copy link
Contributor

This is a question : can I specify, alongside Spacing, FontFamily, etc, a non-default Typography object to override defaults, with the ThemeManager.getMuiTheme(xxx) ?

If this is not possible, this would be a nice addition, especially to define my own settings for the font weights

Thanks

@shaurya947
Copy link
Contributor

Not as of today @pandaiolo

Right now, Theme and Typography are independent. But perhaps it makes sense to link them inside the ThemeManager.

@patrickgordon
Copy link

👍 would love to see this

@newoga
Copy link
Contributor

newoga commented Mar 9, 2016

@pandaiolo @patrickgordon Just to let you know, everywhere styles/typography.js is used can now be overridden in getMuiTheme.js (there were a number of improvements in this area in 0.15.0 development. The only caveat is that there is no way to globally redefine the typography defaults. This is similar to how colors works in that you can theme colors for any of the components, but you can't redefine what lightBlue means in styles/colors.js.

The current typography options in styles/typography.js should be treated as constants, similar to styles/colors.js and should not be modifiable.

@callemall/material-ui Let me know what you think about this. If we want to support a more global approach to adjusting things like fontWeight, we'll probably have to add some keys in baseTheme similar to how pallette works.

@alitaheri
Copy link
Member

I strongly think typography should become just another key on the muiTheme like how zIndex is

@oliviertassinari
Copy link
Member

I strongly think typography should become just another key on the muiTheme like how zIndex is

The zIndex is another key inside the muiTheme because we need consistency between all the components and it's more efficient to have the values close. I think that the use case for the typography key is different from the zIndex.
But it's more like the palette, I agree with @alitaheri 👍.

@newoga
Copy link
Contributor

newoga commented Mar 9, 2016

I agree conceptually. However, I disagree that we should put typography in there as is. 😄

Here's why:

This is what's currently in styles/typography.js

import {
fullBlack,
darkBlack,
lightBlack,
minBlack,
fullWhite,
darkWhite,
lightWhite,
} from './colors';

class Typography {

  constructor() {
    // text colors
    this.textFullBlack = fullBlack;
    this.textDarkBlack = darkBlack;
    this.textLightBlack = lightBlack;
    this.textMinBlack = minBlack;
    this.textFullWhite = fullWhite;
    this.textDarkWhite = darkWhite;
    this.textLightWhite = lightWhite;

    // font weight
    this.fontWeightLight = 300;
    this.fontWeightNormal = 400;
    this.fontWeightMedium = 500;

    this.fontStyleButtonFontSize = 14;
  }
}

export default new Typography();

To me all the colors here are redundant. There's no point in having them separate from styles/colors.js, we should remove them and whever they're being use, just impor from styles/colors.js instead.

We can keep the fontWeights and sizes, however they shouldn't be customized because there's no reason why the fontWeight constants should be changed either because the represent actual things. Medium is never anything other than 500, same way white is never anything other than #FFFFFF.

That being said, I think we should have a set of typography options like:

  • primaryText
  • secondaryText
  • disabledText

And typography style options where you can apply the font weight constants and colors and etc.:

  • Display 4, 3, 2, 1
  • Headline
  • Title
  • Subheading
  • Body 2, 1
  • Caption
  • Button

The biggest difference are these are conceptual rather than concrete. These don't exist in this project yet.

@rob2d
Copy link

rob2d commented Mar 26, 2017

I am currently having related pain (using v1.0.0). The default font-family seems to always override certain components; e.g. MuiText. I am using a CSS everything-selector, but then if using other libraries, the typography package settings override everything, so individual classes must have the font-family specified which gets a little bit redundant with JSS (especially considering I am just aiming to use Roboto Light vs Roboto). If anyone knows of a better workaround in the meantime, please let me know! Thanks.

@oliviertassinari
Copy link
Member

I'm closing that issue as everything in the theme can be changed on the next branch.
I have been updating the documentation regarding the theme. Maybe that could help, let us know: https://material-ui-1dab0.firebaseapp.com/customization/overrides.

@gnapse
Copy link
Contributor

gnapse commented Jun 24, 2017

In our app we want to use material-ui but with a font other than Roboto. We're using the next branch.

I tried to follow the instructions to make global theme variations which take me to the theme variables that should be tweaked.

This last one recommends taking a look at material-ui/style/theme.js. While digging there in the code, I realize that the font family is not easily modifiable globally. It is hardcoded in a set of constants in material-ui/style/typography.js:7 but that function seems to be an internal API. First, it's not exported by style/index.js. It is called from within createTheme, but constants are not being passed in that call, so there's no way to influence those constants.

The only ways I see it possible right now are either re-creating in my app the entire invocation to createTheme but tweaking the call to createTypography in it, passing it a custom fontFamily in the constants argument. Or simply creating a whole copy of the default theme and change the parts with a reference to the font family.

Here's more or less what I had to do to achieve it following the first of the two options given in hte previous paragraph. I extracted all this logic into a separate file from which I import the theme in my main app index.js file. Note how I even had to deal with creating the palette myself, since the createTypography function needs it as the first argument.

import createTheme from 'material-ui/styles/theme';
import createTypography from 'material-ui/styles/typography';
import createPalette from 'material-ui/styles/palette';

const palette = createPalette();

export const theme = createTheme({
  palette,
  typography: createTypography(palette, {
    fontFamily: '"Lato", "Helvetica", "Arial", sans-serif',
  })
});

Any other recommendations as to how achieve using a different font family globally? It would be nice if it were possible without having to meddle too much with internal APIs.

@rob2d
Copy link

rob2d commented Jun 24, 2017

@oliviertassinari this is great, thanks!

@oliviertassinari
Copy link
Member

@gnapse Right, that's exactly how we expect users to achieve this. Here is another example:

import MuiThemeProvider from 'material-ui/styles/MuiThemeProvider'
import createPalette from 'material-ui/styles/palette'
import createTypography from 'material-ui/styles/typography'
import createMuiTheme from 'material-ui/styles/theme'

const primary = {
  // custom
}

const accent = {
  // custom
}

const palette = createPalette({
  primary,
  accent,
})

let theme = createMuiTheme({
  palette,
  typography: createTypography(palette, {
    fontFamily: "'Font 1', sans-serif",
    fontSize: 14,
    fontWeightLight: 300, // Font 1
    fontWeightRegular: 400, // Font 1
    fontWeightMedium: 700, // Font 2
  }),
})

const fontFamilySecondary = "'Font 2', sans-serif"
const fontHeader = {
  color: theme.palette.text.primary,
  fontWeight: theme.typography.fontWeightMedium,
  fontFamily: fontFamilySecondary,
  textTransform: 'uppercase',
}

theme = {
  ...theme,
  palette: {
    ...theme.palette,
    background: {
      ...theme.palette.background,
      default: white,
    },
  },
  typography: {
    ...theme.typography,
    fontFamilySecondary,
    display4: {
      ...theme.typography.display4,
      ...fontHeader,
      fontSize: 60,
    },
    display3: {
      ...theme.typography.display3,
      ...fontHeader,
      fontSize: 48,
    },
    display2: {
      ...theme.typography.display2,
      ...fontHeader,
      fontSize: 42,
    },
    display1: {
      ...theme.typography.display1,
      ...fontHeader,
      fontSize: 36,
    },
    headline: {
      ...theme.typography.headline,
      fontSize: 20,
      fontWeight: theme.typography.fontWeightLight,
    },
    title: {
      ...theme.typography.title,
      ...fontHeader,
      fontSize: 18,
    },
    subheading: {
      ...theme.typography.subheading,
      fontSize: 18,
    },
    body2: {
      ...theme.typography.body2,
      fontWeight: theme.typography.fontWeightRegular,
      fontSize: 16,
    },
    body1: {
      ...theme.typography.body1,
      fontSize: 14,
    },
  },
}

const createDefaultContext = () => {
  const { styleManager } = MuiThemeProvider.createDefaultContext({ theme })

  return {
    styleManager,
    theme,
  }
}

export default createDefaultContext

but that function seems to be an internal API. First, it's not exported by style/index.js

Oops, that's a good point, we should expose it!

@gnapse
Copy link
Contributor

gnapse commented Jun 24, 2017

@oliviertassinari thanks for your quick reply.

In addition to expose these functions I would suggest a couple more things:

  1. The fact that you need to create the palette so you can invoke createTypography does not seem right. Especially in cases like mine where I want to create the typography just to change the font family. I understand that if the parameter is there, is because the process of creating the typography needs to know about the palette. What I'm saying is that... (next point below).
  2. There should be a way where one could tweak some parameters that would make createTheme pick them up. That hard-coded literal fontFamily string I linked to should be influenceable more easily, without having to meddle with all this api.
  3. Even if you don't agree with my opinions on how this API should be changed, I think it would be good to make it more explicit in the documentation. Perhaps that very example you pasted out to me above would make it evident to people how to approach these customizations.

I know that when advancing such a big project, sometimes is normal that documentation is not thorough in every detail. This is all meant as constructive critique. And I'm happy to help making PRs myself. I just don't feel comfortable enough yet to assume what's the correct or intended way to go, to make the PR in the first place.

@oliviertassinari
Copy link
Member

You can get away without creating the palette. What you could do instead is first creating the theme object, then overrding the typography value with the output of createTypography. That way you let Material-UI creating the palette. I can provide an example if needed. Would that address the concern?

I agree, documentation is always welcomed as long as not creating noise :).

@gnapse
Copy link
Contributor

gnapse commented Jun 24, 2017

I don't mind that much the code I ended up with, but an example of what you say would be good as well.

@yaminyaylo
Copy link

yaminyaylo commented Dec 13, 2017

It is not working in my case

const primary = {
    '20': '#EDF5D7',
    '40': '#DBECB0',
    '60': '#CAE288',
    '80': '#B8D961',
    '100': '#A6CF39',
    '300': '#95C32C',
}
const accent = {
    '20': '#EDF5D7',
    '40': '#DBECB0',
    '60': '#CAE288',
    '80': '#B8D961',
    '100': '#A6CF39',
    '300': '#95C32C',
}

export default createMuiTheme({
    overrides: {
        MuiFormLabel: MuiFormLabel,
        MuiFormControlLabel: MuiFormControlLabel,
        MuiFormHelperText: MuiFormHelperText,
        MuiRadio: MuiRadio,
        MuiTable: MuiTable,
        MuiTableBody: MuiTableBody,
        MuiTableHead: MuiTableHead,
        MuiTableRow: MuiTableRow,
        MuiTableCell: MuiTableCell,
        MuiSnackbarContent: MuiSnackbarContent,
        MuiSwitch: MuiSwitch,
        MuiLinearProgress: MuiLinearProgress,
        MuiButton: MuiButton,
        MuiIconButton: MuiIconButton,
        MuiSvgIcon: MuiSvgIcon,
        MuiTab: MuiTab,
        MuiTabs: MuiTabs,
    },
    typography: typography,
    palette: createPalette({primary, accent}),
})

I had error

index.js:61 Uncaught TypeError: Object(...) is not a function

@oliviertassinari
Copy link
Member

@yaminyaylo This is not actionable, we need reproduction examples.

mnajdova pushed a commit to mnajdova/material-ui that referenced this issue Nov 10, 2020
Bumps [jss](https://github.com/cssinjs/jss) from 10.2.0 to 10.3.0.
- [Release notes](https://github.com/cssinjs/jss/releases)
- [Changelog](https://github.com/cssinjs/jss/blob/master/changelog.md)
- [Commits](cssinjs/jss@v10.2.0...v10.3.0)

Signed-off-by: dependabot-preview[bot] <support@dependabot.com>

Co-authored-by: dependabot-preview[bot] <27856297+dependabot-preview[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new feature New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants