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
[TimeDisplay] Inherit text color from theme #5816
[TimeDisplay] Inherit text color from theme #5816
Conversation
@@ -70,7 +70,7 @@ class TimeDisplay extends Component { | |||
borderTopLeftRadius: 2, | |||
borderTopRightRadius: 2, | |||
backgroundColor: timePicker.headerColor, | |||
color: 'white', | |||
color: timePicker.textColor, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a breaking change. We can't merge like that.
timePicker.textColor = palette.accent3Color
I think that we should change that line with palette.textColor
https://github.com/callemall/material-ui/blob/master/src/styles/getMuiTheme.js#L296
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @oliviertassinari! That makes a lot of sense. I'll update the PR accordingly.
@@ -293,7 +293,7 @@ export default function getMuiTheme(muiTheme, ...more) { | |||
}, | |||
timePicker: { | |||
color: palette.alternateTextColor, | |||
textColor: palette.accent3Color, | |||
textColor: palette.textColor, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My bad, I would rather be using alternateTextColor
as in the datePicker
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No worries. Here you go.
Hi @oliviertassinari, thanks for your prompt feedback! Let me know what I can do to move this forward. |
Thanks |
TimeDisplay component has text color fixed to 'white' instead of inheriting it from the theme. I looked at DatePicker component to see how it works, I found out that, unlike TimePicker, it's using datePicket.textColor from theme to set text color. This pull request brings TimePicker on a par with DatePicker.