UI improvements [WIP, DO NOT MERGE] #45
Conversation
…link sharing options, a vector YC logo, font overriding and more
…ctivityViewControllers to dedupe code
|
@weiran just checking in about those icons again 😄 |
|
@weiran Just a heads up it looks like your latest libHN changes broke job posts (they only have a urlString and title now, no ID or time). |
| } | ||
|
|
||
| extension MainTabBarController: Themed { | ||
| func applyTheme(_ theme: AppTheme) { | ||
| tabBar.barTintColor = theme.barBackgroundColor | ||
| tabBar.tintColor = theme.barForegroundColor | ||
|
|
||
| let application = UIApplication.shared.delegate as! AppDelegate |
There was a problem hiding this comment.
Can use a chain of if lets here to avoid force casting.
| @@ -34,6 +36,12 @@ class CommentsViewController : UIViewController { | |||
| setupPostTitleView() | |||
| view.showAnimatedSkeleton(usingColor: AppThemeProvider.shared.currentTheme.skeletonColor) | |||
| loadComments() | |||
|
|
|||
| let activity = NSUserActivity(activityType: "com.weiranzhang.Hackers.comments") | |||
There was a problem hiding this comment.
Can this be wrapped in a setupHandoff method?
| @@ -107,6 +128,16 @@ extension CommentsViewController: PostTitleViewDelegate { | |||
|
|
|||
| // show link | |||
| let safariViewController = ThemedSafariViewController(url: url) | |||
| let activity = NSUserActivity(activityType: "com.weiranzhang.Hackers.link") | |||
There was a problem hiding this comment.
There's some duplication with the following block of code in a few places.
|
|
||
| extension UIFont { | ||
|
|
||
| @objc class func mySystemFont(ofSize size: CGFloat) -> UIFont { |
There was a problem hiding this comment.
A better name is themedSystemFont so that it's clear this method returns a font based on the current theme.
| skeletonColor: UIColor(rgb: 0xAAAAAA) | ||
| skeletonColor: UIColor(rgb: 0xAAAAAA), | ||
|
|
||
| regularFontName: UIFont.systemFont(ofSize: 1).fontName, |
There was a problem hiding this comment.
Perhaps use some placeholder value for size instead of 1 to avoid confusion.
| } | ||
| }.cellUpdate { cell, row in | ||
| let activeTheme = UserDefaults.standard.enabledTheme | ||
| cell.textLabel?.textColor = activeTheme.textColor |
There was a problem hiding this comment.
Can this cell conform to the Themed protocol instead of being manually updated?
I'm afraid I don't have the original sources other than the 1x, 2x, 3x PNG icons.
Thanks I'll have a look at it. |
|
Some great improvements in here, thanks! There's also a lot of changes going on in this single PR which makes it hard to review the code, and also review the changes involved. I like most of them but I'm unsure about long-press gestures (I'm thinking of adding cell swipe actions to reply/share/collapse thread etc). Smaller PRs will make it easier to review and merge if you're able to split out future features/enhancements. |
|
Closing this for now. @robbiet480 let me know if you'd like to continue, otherwise I'll probably take several of your improvements into the 4.0 milestone myself. Thanks. |
This PR adds a few nice UI tweaks:
Indicates if SFSafariViewController should enable collapsing of the navigation bar and hiding of the bottom toolbar when the user scrolls web content.)@weiran Can you provide me sources for the icons you are using? I need to generate a new set of icons that look good in the original theme as right now the grey doesn't look great...