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

Getting ListItemText to inherit color is a pain #11834

Closed
2 tasks done
jedwards1211 opened this issue Jun 12, 2018 · 11 comments
Closed
2 tasks done

Getting ListItemText to inherit color is a pain #11834

jedwards1211 opened this issue Jun 12, 2018 · 11 comments
Labels
component: list This is the name of the generic UI component, not the React module! good first issue Great for first contributions. Enable to learn the contribution process.

Comments

@jedwards1211
Copy link
Contributor

  • This is a v1.x issue (v0.x is no longer maintained).
  • I have searched the issues of this repository and believe that this is not a duplicate.

Expected Behavior

I wish it were this easy:

<ListItemText color="inherit">Inherited Color</ListItemText>

Current Behavior

I have to do:

<ListItemText><Typography color="inherit">Inherited Color</Typography></ListItemText>

This is a pain. I could create a wrapper component that inherits the color, but that would also be a pain. Most Material UI components make it nice and easy to inherit the color.

Context

I have a sidebar containing <ListItems> with a dark background, so I want all of the <ListItemText> to be white, and it would be nice to be able to just inherit the white foreground color of the sidebar.

Your Environment

Tech Version
Material-UI v1.2.1
@oliviertassinari
Copy link
Member

I have to do:

You also have to add disableTypography .

@jedwards1211
Copy link
Contributor Author

Ah, I hadn't even tried, I was just guessing. So it takes even more text to do.
As a side note, I wish there were some magic way to reference the JSS class name for the ListItemText and Typography within the styles for my sidebar...then I could override the color there and it would be fairly clean. Maybe I can eventually think of a way to make that possible and propose it as a new feature in JSS.

@jedwards1211
Copy link
Contributor Author

So I'm basically wishing we could add a color property to ListItem that gets forwarded to the primary Typography it renders. However, we would also need to handle the color in the case that disableTypography is true. What do you think?

@jedwards1211
Copy link
Contributor Author

@oliviertassinari at the very least it would be helpful to have primaryProps and secondaryProps, in the same vein as inputProps on an Input component.

@oliviertassinari
Copy link
Member

oliviertassinari commented Jun 13, 2018

I wish there were some magic way to reference the JSS class name for the ListItemText and Typography within the styles for my sidebar

If I understand correctly what you are looking for. This should work:

<ListItemText classes={{ primary: 'your class' }} />

it would be helpful to have primaryProps and secondaryProps

I have no objection to adding a primaryTypographyProps and secondaryTypographyProps properties 👍 .

@oliviertassinari oliviertassinari added good first issue Great for first contributions. Enable to learn the contribution process. component: list This is the name of the generic UI component, not the React module! labels Jun 13, 2018
@jedwards1211
Copy link
Contributor Author

jedwards1211 commented Jun 13, 2018

@oliviertassinari but you're not open to a color prop that gets forwarded to the typography? I have a working clone of ListItemText that does that in my own project, so I could PR it pretty quickly...

@jedwards1211
Copy link
Contributor Author

@oliviertassinari by the way, rendering <Typography> inside of <ListItemText disableTypography> would probably confuse the heck out of people who are just getting started in a project that uses Material UI:

<ListItemText disableTypography>
  <Typography color="inherit">Test</Typography>
</ListItemText>

It's not very elegant. We could check if the user passed in their own <Typography> and if so, not require them to use disableTypography.

@oliviertassinari
Copy link
Member

oliviertassinari commented Jun 13, 2018

but you're not open to a color prop that gets forwarded to the typography?

@jedwards1211 I think that it's too specific, the primary and secondary don't have the same color by default. Yeah, you can wrap the component. But I don't have a strong opinion about it. @mbrookes what do you think?

@oliviertassinari
Copy link
Member

by the way, rendering inside of would probably confuse the heck out of people who are just getting started in a project that uses Material UI:

It's a good point. We might even make this behavior the default, so people don't have to provide disableTypography at all.

@mbrookes
Copy link
Member

I agree with @oliviertassinari that primaryTypographyProps and secondaryTypographyProps` properties is the way to go for flexibility.

If we added color, we'd also then need secondaryColor, and there might still be the need for the typography props for some other use-case, so we potentially end up with four new properties.

@jedwards1211
Copy link
Contributor Author

Okay. I wish there could be a PrimaryListItemText component, but unfortunately there would be no magic way to make the secondary text display underneath the primary text in an API like this:

<ListItem>
  <PrimaryListItemText color="inherit">Primary</PrimaryListItemText>
  <SecondaryListItemText>Secondary</SecondaryListItemText>
</ListItem>

jedwards1211 added a commit to jedwards1211/material-ui that referenced this issue Jun 14, 2018
jedwards1211 added a commit to jedwards1211/material-ui that referenced this issue Jun 14, 2018
oliviertassinari pushed a commit to jedwards1211/material-ui that referenced this issue Jun 14, 2018
oliviertassinari pushed a commit that referenced this issue Jun 14, 2018
#11858)

* feat(ListItemText) add primaryTypographyProps and secondaryTypographyProps props

fix #11834

* ready to merge
zlargon added a commit to zlargon/mbta that referenced this issue Jul 9, 2018
acroyear pushed a commit to acroyear/material-ui that referenced this issue Jul 14, 2018
mui#11858)

* feat(ListItemText) add primaryTypographyProps and secondaryTypographyProps props

fix mui#11834

* ready to merge
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: list This is the name of the generic UI component, not the React module! good first issue Great for first contributions. Enable to learn the contribution process.
Projects
None yet
Development

No branches or pull requests

3 participants