chore: Elm theme improvement on Course Discussions #90
Conversation
| @@ -68,15 +68,15 @@ public struct CommentCell: View { | |||
| onReportTap() | |||
| }, label: { | |||
| comment.abuseFlagged | |||
| ? CoreAssets.reported.swiftUIImage | |||
| : CoreAssets.report.swiftUIImage | |||
| ? (CoreAssets.reported.swiftUIImage.renderingMode(.template)) | |||
There was a problem hiding this comment.
do we need outer brackets here?
There was a problem hiding this comment.
Without these brackets the Xcode is generating the following warning Void Function in Ternary Violation: Using ternary to call Void functions should be avoided (void_function_in_ternary)
There was a problem hiding this comment.
Then maybe you should heed the warning recommendation and not hide the warning but refactor the code?
Something like this:
let icon = comment.abuseFlagged ? CoreAssets.reported.swiftUIImage : CoreAssets.report.swiftUIImage icon.renderingMode(.template)
WDYT?
| .frame(width: 48, height: 48) | ||
| .cornerRadius(isThread ? 8 : 24) | ||
| .cornerRadius(24) |
There was a problem hiding this comment.
is this change acceptable for upstream?
There was a problem hiding this comment.
I guess so because this is the only place where the user avatar isn't circular. So after this change, the user avatar will be consistent across the app.
There was a problem hiding this comment.
Hello @marcotuts and @sdaitzman. Could you please confirm that this almost square avatar is not a design feature and should be the same throughout the app? Thank you
There was a problem hiding this comment.
to me the avatar being square and circle is a design inconsistency. It should either be circle everywhere or square everywhere. For 2U we have a circular approach to icons and buttons.
There was a problem hiding this comment.
I agree with you. Just need confirmation we shouldn't add theming here.
| : Theme.Colors.textSecondaryLight) | ||
| Spacer() | ||
| Button(action: { | ||
| onReportTap() | ||
| }, label: { | ||
| comments.abuseFlagged | ||
| ? CoreAssets.reported.swiftUIImage | ||
| : CoreAssets.report.swiftUIImage | ||
| ? (CoreAssets.reported.swiftUIImage.renderingMode(.template)) |
There was a problem hiding this comment.
do we need outer brackets here?
| } | ||
|
|
||
| public final class ThemeConfig: NSObject { | ||
| public var isRoundedCorners: Bool | ||
| public var buttonCornersRadius: Double | ||
|
|
There was a problem hiding this comment.
Vertical Whitespace Violation: Limit vertical whitespace to a single empty line; currently 2 (vertical_whitespace)
Description:
Jira Ticket: LEARNER-10162
Config PR: https://github.com/edx/edx-mobile-config/pull/184
This PR adds some color improvements in the discussion module and changes the button corner style throughout the app.
before.mov
after.mov