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

RFC: material typography spec v2 #12741

Closed
9 of 14 tasks
eps1lon opened this issue Sep 1, 2018 · 35 comments
Closed
9 of 14 tasks

RFC: material typography spec v2 #12741

eps1lon opened this issue Sep 1, 2018 · 35 comments
Labels
component: Typography The React component. design: material This is about Material Design, please involve a visual or UX designer in the process
Milestone

Comments

@eps1lon
Copy link
Member

eps1lon commented Sep 1, 2018

This is a proposal for a migration path from the v1 typography spec to v2.

Breaking changes

  • every variant will have a primary color. The spec does not list color as a property of typography.
  • removes the following variants:
    • display1
    • display2
    • display3
    • display4
    • headline
    • title
    • subheading
  • changes the style of the following variants:
    • body1
    • body2
    • caption
    • button

Implementation

Color

The spec does not mention color. Use primary by default and apply color manually on every usage.

Variant Name

Use names from spec but use headlineX instead of hX which matches mwc implementation

Roadmap

  • new minor (implemented with [Typography] Add typography v2 variants #12916, scheduled for 3.2.0):
    • add new variants from the spec, conflicting names will be suffixed with Next
    • set suppressDeprecationWarning on every internal Typography usage to suppress warnings. They are for the user not the dev. Users should be warned if an internal component uses a deprecated variant. Recommend setting useNextVariants per component.
    • show deprecation warnings for categories that will be removed or restyled in a new minor version
    • expose useNextVariants to Typography. This will use the style of variants in the next major (only concerns variants that will be restyled for all variants so that using useNextVariants has no breaking change when switching to new major)
    • update typescript definitions
    • recommend new variants and colors that should replace deprecated variants
    • Update docs to use typography v2
  • new major with:
    • useNextVariants, suppressDeprecationWarning will become noop props
    • write codemod for migration:
      • remove useNextVariants, suppressDeprecationWarning props
      • apply new variants and colors according to recommendation
    • refactor internal usage of Typography, use codemod
    • remove old variants
  • new minor with:
    • warn when using useNextVariants, suppressDeprecationWarning because noop

If we don't consider style changes a breaking change we can skip the hole suppressDeprecationWarning step which reduces the workload but is probably annoying for users that aim at pixel perfect apps.

Misc

Spec:

Existing Implementations:

Related:

@oliviertassinari Could you ping core contributors?

@oliviertassinari
Copy link
Member

@eps1lon Thanks for setting that up. We might not need a codemod for such change, the search&replace can potentially be simple to perform. I have no objection otherwise.

ping @mui-org/core-contributors

@oliviertassinari oliviertassinari added component: Typography The React component. design: material This is about Material Design, please involve a visual or UX designer in the process labels Sep 2, 2018
@oliviertassinari oliviertassinari modified the milestones: v3.x, v4 Sep 2, 2018
@leMaik
Copy link
Member

leMaik commented Sep 9, 2018

Maybe we could provide a new @material-ui/typography package after that change that brings back the old Typography component so that we don't break people's projects?

Or maybe (even better) allow selecting the typography style in the theme? Thus making the change almost non-breaking by setting all typography properties in the theme and allow users to bring back the old typography config.

@oliviertassinari
Copy link
Member

oliviertassinari commented Sep 9, 2018

Maybe we could provide a new @material-ui/typography package after that change that brings back the old Typography component so that we don't break people's projects?

@leMaik I would rather avoid this type of operational overhead. What's wrong with adding a depreciation in a minor and removing the old variant in a major?

Or maybe (even better) allow selecting the typography style in the theme?

I love the idea. It's way more flexibility. We need to find a way to have dynamic prop-types, doesn't sounds good for TypeScript users. We also need a way to identify the theme.typography keys that are variants.

@leMaik
Copy link
Member

leMaik commented Sep 9, 2018

What's wrong with adding a depreciation in a minor and removing the old variant in a major?

Nothing. A breaking change in this particular component just seems weird, it'll be like "Oh, you rely on our font styles? Well, better update everything because we dropped half of them." At least that's how it looks in our apps that use Material-UI. That's why I'd like to see a component that brings back the old Typography. It might be an unneccessary breaking change that causes a lot of work.

@eps1lon
Copy link
Member Author

eps1lon commented Sep 12, 2018

Maybe this seems overly pedantic but we do implement the material design and if this design changes so should we.

This is currently implemented with a simple feature flag in createTheme. I could start cherry picking my work to exclude the deprecation warnings but do we really want to support deprecated typography variants?

@nathanmarks
Copy link
Member

I say we just include the deprecation warnings and remove them in the next major.

@JeanBeaurepaire
Copy link
Contributor

image

In fact, this feature should also include new headers and both subheading, but also new category overline

@eps1lon
Copy link
Member Author

eps1lon commented Sep 17, 2018

@Abbo44 I'm aware of the spec. The new variants are almost finished.

I just need to polish the deprecation warnings so that they don't interfere with our own test suite.

@eps1lon
Copy link
Member Author

eps1lon commented Sep 19, 2018

Implemented typography v2 with deprecation warnings and docs in typography v2 in #12916.

@elmeerr
Copy link
Contributor

elmeerr commented Oct 9, 2018

The deprecation warning lists caption as deprecated but in docs example caption variant is being used and is not on the from => to list
image

Also the Stepper component is still using body1 (I don't know if you already know but as I'm here I think it worths to mention)

Kind regards,

@eps1lon
Copy link
Member Author

eps1lon commented Oct 9, 2018

@elmeerr caption is not deprecated. It will only change it's style which is why we warn about this.

@elmeerr
Copy link
Contributor

elmeerr commented Oct 9, 2018

@eps1lon Indeed, my bad! Just saw one warning and assume that was all the same. But the DialogTitle is still using title variant...this will be removed 😄

@kenwithee
Copy link

I am working through the demo component ClippedDrawer and noticed these warnings below. Being a newbie on this I am not sure if I am doing something wrong or need to update something on the demo in GitHub to prepare the ClippedDrawer demo for the next version?

Warning: Material-UI: You are using the deprecated typography variant subheading that will be removed in the next major release.
Please read the migration guide under https://material-ui.com/style/typography#migration-to-typography-v2

Warning: Material-UI: You are using the typography variant body1 which will be restyled in the next major release.
Please read the migration guide under https://material-ui.com/style/typography#migration-to-typography-v2

@kenwithee
Copy link

Just in case any other newbies like me run across my question above with the default examples and getting warnings. I was able to solve it with the content on this page: https://material-ui.com/customization/themes/

In particular, it seems the default theme was the culprit and the above discussion makes more sense now about the find/replace. I suspect that after that is done then the examples, like ClippedDrawer, that uses variants that trigger the warnings will be resolved.

In the meantime, I did the following to create a custom theme with the only option being the useNextVariants set to true and the warnings went away now. Here is my create-react-app code that I am using to test which seems to have resolved it for the ClippedDrawer example.

import React from 'react';
import ReactDOM from 'react-dom';
import App from './App';
import { BrowserRouter } from "react-router-dom";
import registerServiceWorker from './registerServiceWorker';
import { MuiThemeProvider, createMuiTheme } from '@material-ui/core/styles';

const theme = createMuiTheme({
   typography: {
     useNextVariants: true,
   },
 });

ReactDOM.render(
   <MuiThemeProvider theme={theme}>
      <BrowserRouter>
         <App />
      </BrowserRouter>
   </MuiThemeProvider>,
   document.getElementById('root')
);

registerServiceWorker();

@JeanBeaurepaire
Copy link
Contributor

Hi @eps1lon , I think you forgot to implement the overline type in TypographyClassKey, because i'm unable to override overline type in MuiTypography.

Actual TypographyClassKey :

export type TypographyClassKey = | 'root' | 'h1' | 'h2' | 'h3' | 'h4' | 'h5' | 'h6' | 'subtitle1' | 'subtitle2' | 'body1' | 'body2' | 'caption' | 'button' | 'srOnly' | 'alignLeft' | 'alignCenter' | 'alignRight' | 'alignJustify' | 'noWrap' | 'gutterBottom' | 'paragraph' | 'colorInherit' | 'colorSecondary' | 'colorTextSecondary' | 'display4' // deprecated | 'display3' | 'display2' | 'display1' | 'headline' | 'title' | 'subheading';

@novascreen
Copy link
Contributor

Use names from spec but use headlineX instead of hX which matches mwc implementation

@oliviertassinari I was wondering why this was changed to hX?

I think it is a lot more confusing to use hX if we remap the components that are used by default. For example h1-3 are so large, i never use them, so headline4 would be my <h1> as with the deprecated variant display1 out of the box.
Reading <Typography variant="h4"> you expect an h4, but reading <Typography variant="headline4"> doesn't necessarily trigger that expectation. I'm assuming that's why MWC chose to avoid hX.
Always thought the beauty of the previous naming was that it separated visual style from semantics, that's just my $0.02

@oliviertassinari
Copy link
Member

oliviertassinari commented Oct 10, 2018

I was wondering why this was changed to hX?

@novascreen Because of https://twitter.com/olivtassinari/status/1044347621425520642
capture d ecran 2018-10-10 a 21 00 39
Next time, we can use the documentation notification system to collect more votes.

We are using the following rewrite at work:

<Typography headlineMapping={{
  h1: 'h1',
  h2: 'h1',
  title: 'h1',
}} />

@novascreen
Copy link
Contributor

Fair enough

@oliviertassinari
Copy link
Member

@novascreen Yet, you are definitely right, variant="h1" needs to be understood as headling1.

@3rwww1
Copy link

3rwww1 commented Oct 15, 2018

Is it just me or is the new kerning really weird ?

I have the Roboto font loaded from CDN with all the necessary weights.

Before:
image

After:
image

Before:
image

After:
image

@eps1lon
Copy link
Member Author

eps1lon commented Oct 15, 2018

@3rwww1
The spec added letter-spacing to their variants. You can simply remove this with:

const theme = createMuiTheme({
  typography: {
    allVariants: {
      letterSpacing: 0
    },
  }
});

@3rwww1
Copy link

3rwww1 commented Oct 15, 2018

@eps1lon thanks for the tip! Please disregard my comment and sorry for the noise.

Background: I'm aware of the spec change and actually checked out https://material.io/design/typography/the-type-system.html#type-scale before posting. Initially thought you might have misinterpreted the letter-spacing conversion to web units - sorry for doubting you folks 🙏

Then checked with different browsers and OSes, and AFAIK this kerning issue only happens on Firefox Linux (OK on Chrome/Linux, Chrome/OSX, Firefox/OSX). The difference is quite striking though (at least for me):

Firefox/Linux:
image

Chrome/Linux:
image

I do not have a Windows setup install (or different Linux flavor) handy to run further tests.

@Arya-Aghaei

This comment has been minimized.

@oliviertassinari
Copy link
Member

https://betterwebtype.com/rhythm-in-web-typography

@3rwww1
Copy link

3rwww1 commented Oct 18, 2018

So, in case someone else also notices this weird font rendering after migrating, this is officially a Firefox kerning issue:
https://bugzilla.mozilla.org/show_bug.cgi?id=1441843

Summary:

  • kerning breaks on firefox Linux and Windows when the CSS letter-spacing property is being used
  • wish I could patch things up by adding font-kerning: normal (as suggested in @oliviertassinari's link) to the affected typography variants, but it doesn't fix anything (kerning seems entirely disabled when letter-spacing is set)

@eps1lon
Copy link
Member Author

eps1lon commented Oct 18, 2018

@3rwww1 We considered removing the letter-spacing entirely because @oliviertassinari had concerns with kerning issues but I'm pretty adamant when it comes to spec compliance by default.

The only difference I can spot is when using font-kerning: none. All other values look equal. The bad thing is that I didn't notice it before but now it does look pretty confusing on Firefox if you have longer texts.

@wijwoj
Copy link
Contributor

wijwoj commented Oct 19, 2018

As suggested by @eps1lon I have the following in my createMuiTheme to suppress the odd looking extra letter spacing (particularly in FF) after upgrade to v2 typography:

  typography: {
    useNextVariants: true,
    allVariants: {
      letterSpacing: 0
    },
  }

However, that has not removed any letter spacing from any of my themed/styled components. Am I missing something?

image

@oliviertassinari
Copy link
Member

@wijwoj Something is wrong with your setup: https://codesandbox.io/s/5xy7q2ykxx.

@wijwoj
Copy link
Contributor

wijwoj commented Oct 22, 2018

Ah sorry. Thanks for checking. Will dig in and work out what am doing wrong. Thanks.

EDIT: I am a dufus. That typography config section has somehow found its way into the palette section. All working now. Thanks @oliviertassinari .

@doasync
Copy link

doasync commented Nov 19, 2018

I get warning about deprecated typography when using createMuiTheme.

Warning: Material-UI: you are using the deprecated typography variants that will be removed in the next major release.

screenshot from 2018-11-20 01-23-34

P. S. "@material-ui/core": "3.5.1",

@FranBran
Copy link

FranBran commented Feb 7, 2019

Google seems to have updated their color specs for Typography (?). Unlike MUI defines "primary", "secondary", "disabled" and "hint", google only defines "high emphasis", "medium emphasis" and "disabled". Should MUI take over these changes accordingly? see https://material.io/design/color/text-legibility.html#

@FranBran
Copy link

FranBran commented Feb 8, 2019

And why uses Formcontrol "body2" as Typography-variant? Shouldn't it be "body1"?
grafik

@eps1lon
Copy link
Member Author

eps1lon commented Feb 15, 2019

And why uses Formcontrol "body2" as Typography-variant? Shouldn't it be "body1"?
grafik

@FranBran Could you open a separate issue and provide links to the spec? You can also just go ahead and open a PR to change it if it's indeed defined that way in the spec.

@rlarnfktkd1
Copy link

please add useNextVariants type

@oliviertassinari
Copy link
Member

@rlarnfktkd1 This was removed in v4.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: Typography The React component. design: material This is about Material Design, please involve a visual or UX designer in the process
Projects
None yet
Development

No branches or pull requests