-
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
Adjust layout and colors of the voice recording view #704
base: develop
Are you sure you want to change the base?
Conversation
SDK Size
|
98771f4
to
7fcb4b7
Compare
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.
Overall looks to good to me, but we are missing some stuff 👍
// Then | ||
AssertSnapshot( | ||
view, | ||
variants: .onlyUserInterfaceStyles, |
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.
How does the tests look with different text sizes? 🤔 Do we have tests for this?
7fcb4b7
to
3738c4f
Compare
3738c4f
to
0240cfa
Compare
public var voiceMessageCurrentUserBackground: UIColor = .streamInnerBorder | ||
public var voiceMessageOtherUserBackground: UIColor = .streamBarsBackground | ||
public var voiceMessageCurrentUserRecordingBackground: UIColor = .streamBarsBackground | ||
public var voiceMessageOtherUserRecordingBackground: UIColor = .streamBarsBackground | ||
public var voiceMessageControlBackground: UIColor = .streamWhiteStatic |
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.
We probably could have reused some of the existing Color Pallete colors so that customers could have the styling for free 🤔 On UIKit, we are using the existing background colours in the ColorPallete. So if a customer already set their Theme, the voice recording attachment will be styled for free. In this case, they will need to set the color for this specific component. Which kinda defeats the porpose of a "Theme". That is why most of the names in the color palette don't have context (ex: Background1, Background2, etc..)
Especially for voiceMessageCurrentUserBackground
, isn't this the same as messageCurrentUserBackground
for example?
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.
I used exactly the same colors what we have in our Figma specs. These are slightly different from what the current palette has. Not sure what is the best way here.
a) use existing colors which do not match with Figma spec
b) add background9
and background10
to replace the use of streamBarsBackground
and streamInnerBorder
c) close the PR and add it for v5 (might allows making more such changes which might break existing customisations)
d) …
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.
For customers, it's easier if we are explicit about the names of the colors. Naming like backgroundX
doesn't mean anything and it's always difficult to figure things out.
However, voiceMessageCurrentUserBackground
should be the same as messageCurrentUserBackground
, etc. Probably some misalignment also in the Figma.
So my suggestion would be: try to use what's already available for things that logically fit together (like the example above). Introduce new ones for the stuff we don't have with your naming convention, e.g. voiceMessageControlBackground
.
Quality Gate failedFailed conditions |
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.
Looks good, let's just align on the naming.
public var voiceMessageCurrentUserBackground: UIColor = .streamInnerBorder | ||
public var voiceMessageOtherUserBackground: UIColor = .streamBarsBackground | ||
public var voiceMessageCurrentUserRecordingBackground: UIColor = .streamBarsBackground | ||
public var voiceMessageOtherUserRecordingBackground: UIColor = .streamBarsBackground | ||
public var voiceMessageControlBackground: UIColor = .streamWhiteStatic |
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.
For customers, it's easier if we are explicit about the names of the colors. Naming like backgroundX
doesn't mean anything and it's always difficult to figure things out.
However, voiceMessageCurrentUserBackground
should be the same as messageCurrentUserBackground
, etc. Probably some misalignment also in the Figma.
So my suggestion would be: try to use what's already available for things that logically fit together (like the example above). Introduce new ones for the stuff we don't have with your naming convention, e.g. voiceMessageControlBackground
.
@@ -3,10 +3,20 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/). | |||
|
|||
# Upcoming | |||
|
|||
### ✅ Added | |||
- Colors and images for voice recording view [#704](https://github.com/GetStream/stream-chat-swiftui/pull/704) |
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.
I think we forgot about this one: #688
The recording view should also be themable
🔗 Issue Link
Resolves IOS-621
🎯 Goal
Voice recording view is using layout not matching to design specifications, in addition, some of the colors and images are not configurable.
🛠 Implementation
ColorPalette.voiceMessageCurrentUserBackground
andColorPalette.voiceMessageOtherUserBackground
(background of the whole voice recording view e.g. recording items and additional text)ColorPalette.voiceMessageCurrentUserRecordingBackground
andColorPalette.voiceMessageOtherUserRecordingBackground
(background of the voice recording item)ColorPalette.voiceMessageControlBackground
and use it for play button's background colorImages.pauseFilled
and use it for play buttonImages.playFilled
for the play buttonColorPalette.messageCurrentUserTextColor
orColorPalette.messageOtherUserTextColor
for the title of theVoiceRecordingView
🧪 Testing
N/A - covered by snapshot tests
🎨 Changes
Please refer to updated snapshots.
☑️ Checklist