-
Notifications
You must be signed in to change notification settings - Fork 90
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
Apply theme colors to voice recordings #679
Apply theme colors to voice recordings #679
Conversation
@@ -88,7 +89,6 @@ public struct VoiceRecordingContainerView<Factory: ViewFactory>: View { | |||
player.subscribe(handler) | |||
} | |||
.padding(.all, 8) | |||
.background(Color(colors.background)) |
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.
Text colour change is good, but removing the background would diverge from the default design which we can't make. Could you please exclude the background colour change and then we are good to take it in.
Thank you for contributing!
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.
Without this change, it looks like my first picture, this is a bug. I removed this color because I don't think this container view should have a bg of its own, since it should just rely on a message bubble to provide the color for the both of them, no? Using a hardcoded color here is wrong however you look at it - sender and receiver might have different bg colors. In any case, if you don't like the way I fixed it, please fix it however you see fit, but right now it looks wrong and needs to be fixed.
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.
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.
Related PR has been merged
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.
hope that one includes text colors too, so I'm closing this one, thank you
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.
Just added a few comments 👍 We will need to re-record the snapshot tests
Wrong PR
🔗 Issue Link
Jira or Github issue link, if applicable.
🎯 Goal
Apply theme colors to voice recordings
🛠 Implementation
Provide a description of the implementation.
🧪 Testing
Describe the steps how this change can be tested (or why it can't be tested).
🎨 Changes
before
after
☑️ Checklist