-
Notifications
You must be signed in to change notification settings - Fork 16
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
fix: navigation bar colors #232
fix: navigation bar colors #232
Conversation
Theme/Theme/Theme.swift
Outdated
public struct UIColors { | ||
public private(set) static var textPrimary = ThemeAssets.textPrimary.color | ||
public private(set) static var accentColor = ThemeAssets.accentColor.color | ||
public private(set) static var background = ThemeAssets.background.color | ||
|
||
public static func update( | ||
textPrimary: UIColor = ThemeAssets.textPrimary.color, | ||
accentColor: UIColor = ThemeAssets.accentColor.color, | ||
background: UIColor = ThemeAssets.background.color | ||
) { | ||
self.textPrimary = textPrimary | ||
self.accentColor = accentColor | ||
self.background = 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.
@rnr Why it's needed to create a new struct UIColors when we already have uiColor()
extension written on Colors
?
Even if we want to define categorical UIColors, I guess it would be nice to have all the values that Colors have and also it would be nice if you can remove uiColor()
extension so we will have a single source. Thoughts?
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.
@saeedbashir
I'm not sure we need to add the whole bunch of colors to the UIColors structure - it seems like unnecessary duplication.
I experimented again and found that only 'accentColor' and 'textPrimary' in this structure is sufficient. Calculated UIColors get broken when we use these in UIApplicationExtension.swift
for override func viewWillLayoutSubviews()
. For all other places (like CCSInjection or Discussions) it looks working.
So uiColor()
is helpful extension and I don't think we need to delete this.
I commit small changes (got rid unnecessary navigation background modifier) . Please review.
0667023
@rnr I'm only able to replicate the issue when I revert the changes of Could you please check it again to see if you are able to reproduce the issue by using |
@saeedbashir |
This PR fixes bug with Navigation Bar background and title colors in dark mode.
By some reason calculated UIColor doesn't work for navigation bar (like
Theme.Colors.textPrimary.uiColor()
) and I added UIColors struct into Theme.After fix Navigation Bar looks like this: