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

Remove DialogContentText, use Typography #12565

Closed
atrauzzi opened this issue Aug 17, 2018 · 5 comments
Closed

Remove DialogContentText, use Typography #12565

atrauzzi opened this issue Aug 17, 2018 · 5 comments
Labels
breaking change component: dialog This is the name of the generic UI component, not the React module!
Milestone

Comments

@atrauzzi
Copy link
Contributor

atrauzzi commented Aug 17, 2018

Maybe I've got myself tripped up on another documentation opportunity...

We're making great progress right now in establishing a theme for our application(s), but amongst some of the research/snag items we've hit is one about the Typography component.

In looking at some of the MUI component source code and based on our desire to specify little-T-typography in a standard way in certain circumstances, is there any guidance on whether using the Typography component is the right way to go about this?

A simplified scenario might be say with a dialog:

<DialogContent>
    <DialogContentText>
        <Typography>
            Are you sure you want to do <em>that thing</em>?
        </Typography>
    </DialogContentText>
</DialogContent>

When doing this using default MUI properties, we end up with DOM validation errors from the browser as follows:

image

This makes me suspect that nesting Typography and specifically using its variants might not be correct. Even despite the fact that I can force the element used to be say - a span locally, or even globally using my theme.

So I guess this all boils down to what the best advice is for controlling typography. We definitely not inclined to have it be radically different throughout the application. But even with the above example, should we be using Typography or should we be specifying styles for DialogContentText that are potentially duplicates of Typography configurations we have elsewhere in our theme?

Are there times where we should only be adding inline children to MUI elements and is there any kind of rhyme/reason to that so that we can know ahead of time what our strategy should be?

@oliviertassinari oliviertassinari added the component: Typography The React component. label Aug 17, 2018
@oliviertassinari
Copy link
Member

oliviertassinari commented Aug 17, 2018

@atrauzzi From my perspective, the typography story of Material-UI starts with the theme.typography.x objects. I have been using this pattern at multiple occasions in the past, especially in userland:
https://github.com/mui-org/material-ui/blob/96d2c38cfa6f5d8800b8a2632bff4cab56fd9472/packages/material-ui/src/Button/Button.js#L13-L15
However, this pattern, while powerful, is cumbersome. I use it when I can't afford to create an extra DOM node.

The Typography component should answer +80% of the use cases. Take Bootstrap, you would use

  • <h3 className="h1" />
  • <h4 className="h2" />
  • etc.

The equivalent with Material-UI is: (the variant values will soon change with #12377, it will be simpler to grasp 🎉)

  • <Typography component="h3" variant="display4" />
  • <Typography component="h4" variant="display3" />
  • etc.

Is it ok to nest Typography component?

Yes, I have personnaly been doing stuff like this:

<Typogrpaghy component="ul">
  <li>Foo</li>
  <li><Typogrpaghy variant="caption">Material-UI</Typogrpaghy></li>
<Typogrpaghy>

How should people handle the p > p issue?

You can set the component property, you can remove the nesting in the first place or you can use the theme typography styles.

@atrauzzi
Copy link
Contributor Author

Alright, so based on my example above using DialogContent*, would it make sense to take the fact that it renders using a <p> tag as an indicator that it should only have inline children? More specifically, does that means that I might want to favour your first approach of mixing styles in for the DialogContentText rather than trying to force a Typography to work underneath it?

This relates to something that I've wondered while using MUI: Is there a specific reason why Typography isn't used universally throughout MUI as the "block content child" for many types of logical component structures? For example, I see something like this with cards:

<Card className={classes.card}>
     <CardContent>
          <Typography className={classes.title} color="textSecondary">

...but that seems inconsistent to dialog, despite having this *Content naming convention.

Maybe at least for now my specific issue is with the fact that DialogContentText seems redundant to just using Typography? But perhaps there are other similar hierarchies I've yet to encounter and might face similar confusion with. 😉

@oliviertassinari
Copy link
Member

based on my example above using DialogContent*

@atrauzzi I don't see the point of doing DialogContentText > Typography. Just remove the DialogContentText component.

Maybe at least for now my specific issue is with the fact that DialogContentText seems redundant to just using Typography?

Yes, it's. I think that we can kill the component in v2.

@oliviertassinari oliviertassinari added breaking change component: dialog This is the name of the generic UI component, not the React module! and removed component: Typography The React component. discussion labels Aug 18, 2018
@oliviertassinari oliviertassinari added this to the v2 milestone Aug 18, 2018
@atrauzzi
Copy link
Contributor Author

atrauzzi commented Aug 18, 2018

Oh, nice! That definitely helps!

Removing redundant blocks also for sure helps bring things together semantically. Thanks for all your help.

@oliviertassinari oliviertassinari changed the title When are the right and wrong times to be using Typography? Remove DialogContentText, use Typography Sep 16, 2018
@oliviertassinari
Copy link
Member

I think that we can keep the component after all: #14795.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change component: dialog This is the name of the generic UI component, not the React module!
Projects
None yet
Development

No branches or pull requests

2 participants