-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
[Reply] Contrast aware updates. #1284
Conversation
Added colors for standard, medium, and high in both light and dark themes. Added helper function to select contrast level based on values from SysUI controls. Added more variance in application of surface container roles to better indicate elevation and emphasis.
Code formatting.
Formatting.
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.
Very minor feedback; I'm assuming all the colors are perfect and beautiful in every way
) | ||
} | ||
Spacer(modifier = Modifier.width(8.dp)) |
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.
Nit: This spacing is a combination of line 115 and line 129. I'd combine them to just do 12dp in one place, probably 115, but I don't have a strong preference whether it's that or a spacer with no horizontalArrangement
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.
Done.
} | ||
|
||
MaterialTheme( | ||
colorScheme = replyColorScheme, | ||
typography = replyTypography, | ||
shapes = shapes, | ||
typography = replyTypography, shapes = shapes, |
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.
Nit: Should still be separate lines
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.
Done.
content = content | ||
) | ||
} | ||
|
||
@Composable | ||
fun ReplyTheme( |
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.
Is the ReplyTheme still used anywhere?
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.
It was being used in previews but I've replaced all those references and removed ReplyTheme. Done.
Removed partially used theme and implemented code style suggestions. Modified selectSchemeForContrast function to allow it to pass through on API versions without contrast support.
…ples into jwill/reply-contrast
Added colors for standard, medium, and high in both light and dark themes.
Added helper function to select contrast level based on values from SysUI controls.
Added more variance in application of surface container roles to better indicate elevation and emphasis.