[ListItemText] Text styles are being overruled by unexposed Typography component styles

[zachwolf]

Hello,

Sorry if this topic has already been raised, I've searched through the open and closed issues but haven't found anything that matches it. Possibly this, but they didn't provide enough info.

Problem description

I'm trying to apply styling to a <ListItemText> component. The issue is that the component wraps the content in a <Typography> component. This <Typography> component has its own styles, I believe the only way to style them as it is today would be to style them globally with the theme. This is only an issue when trying to apply specific styles such as text color.

In other words, given something like this:

// example.js
// make styles
const styleSheet = createStyleSheet('SimpleList', theme => ({
  text: {
    color: 'red'
  },
}));

// in render
<ListItemText
  primary="Inbox"
  classes={{
    text: classes.text
  }}
/>

This is what's rendering:

In the CSS section of the screenshot above, the class and style are getting applied correctly. However, the component's text is not red. This is because a more specific style is applied one child below:

I think the simplest fix would be to either expose the Typography components colorInherit prop or set it to always true. So, something like:

// https://github.com/callemall/material-ui/blob/next/src/List/ListItemText.js#L43-L58

  return (
    <div className={className} {...other}>
      {primary &&
        (typeof primary === 'string'
-         ? <Typography type="subheading" className={classNames({ [classes.text]: dense })}>
+         ? <Typography type="subheading" className={classNames({ [classes.text]: dense })} colorInherit>
              {primary}
            </Typography>
          : primary)}
      {secondary &&
        (typeof secondary === 'string'
-         ? <Typography secondary type="body1" className={classNames({ [classes.text]: dense })}>
+         ? <Typography secondary type="body1" className={classNames({ [classes.text]: dense })} colorInherit>
              {secondary}
            </Typography>
          : secondary)}
    </div>
  );

🎩:

I can submit this change in a PR, if it looks like an ok fix.

Link to minimal working code that reproduces the issue

Simplified app
Specific lines

Versions

• Material-UI: 1.0.0-alpha.16
• React: 15.5.4
• Browser: Chrome 58.0.3029.110

[oliviertassinari]

Some thought on the issue:

• The typeof primary === 'string' logic is brittle, I would rather have a disableTypography property
• I don't think that colorInherit is a good fix as the color currently rely on palette.text.primary that depend on the theme light/dark mode.
• We might miss a primaryClassName, but if we expose a disableTypography property, do we still need it? I guess yet.

[zachwolf]

I like that solution a lot. It provides the flexibility needed and matches an existing pattern.

I've forked and pushed the solution here. Should I open a PR?

[oliviertassinari]

@zachwolf Yes, that would be awesome!

[kgregory]

@oliviertassinari @zachwolf I'm running into this issue at the moment and was wondering if we could change the CSS API for the ListItemText component. The problem is the text class is only applied to the Typography component when the List is dense. Ideally, there would be two classes:

• text, applied to the inner Typography component
• textDense, applied to the Typography component when the list is dense

There may be an argument for breaking it into classes for primary, secondary, but this would be a simple and clear offering. At this point, the text class is confusing, but the other classes make sense:

• root, applied to the root element
• dense, applied to the root element when the list is dense
• inset, applied to the root element when the list item text is inset

It is worth noting that changing the CSS API as I've suggested would be a breaking change, since anyone using the text class is intending on styling the ListItemText's inner Typography in a dense List.

I know I can disableTypography and set primary to my own JSX, but it would be easier if all I want to do is change the appearance of the text in a minor way.

Let me know if this sounds good and I'll submit a PR.

[oliviertassinari]

At this point, the text class is confusing

I agree.

I know I can disableTypography and set primary to my own JSX, but it would be easier if all I want to do is change the appearance of the text in a minor way.

Hummmm, given the change you are proposing is simple, I think that we can move forward into that direction 👍 .

[kgregory]

@oliviertassinari thanks, PR submitted