Show release notes UI in inbox when notable new features #2473
Conversation
| ) { | ||
| self.markdown = markdown | ||
| self.owner = owner | ||
| self.repo = repo | ||
| super.init(nibName: nil, bundle: nil) | ||
| title = NSLocalizedString("Preview", comment: "") | ||
| self.title = title | ||
| if asMenu { |
There was a problem hiding this comment.
Not sure what this does/when it is used? What is this ViewController showing again?
|
|
||
| weak var delegate: NewFeaturesCellDelegate? | ||
|
|
||
| private let label = UILabel() |
There was a problem hiding this comment.
This could be a bit more descriptive, I think? 😄
| backgroundColor = .white | ||
| contentView.backgroundColor = Styles.Colors.Blue.light.color | ||
| contentView.layer.cornerRadius = Styles.Sizes.cardCornerRadius | ||
| contentView.layer.borderWidth = 1 / UIScreen.main.scale |
There was a problem hiding this comment.
Should this be 1 * scale? Not sure what you're trying to achieve here.
There was a problem hiding this comment.
Looks to be a 1px thin line rather than a 1pt thin line! I've used this a couple times 😄
There was a problem hiding this comment.
Ah, a borderWidth of 1 would be in points. Interesting 🤔
| contentView.addSubview(closeButton) | ||
|
|
||
| let tint = Styles.Colors.Blue.medium.color | ||
| label.text = NSLocalizedString("View new features!", comment: "") |
There was a problem hiding this comment.
"View new features in this release" maybe?
There was a problem hiding this comment.
I'd even maybe consider chucking the actual version number in? Makes it look less repetitive!
View new features in v1.24
There was a problem hiding this comment.
Might include version, want to keep this short to avoid the risk of truncation
|
|
||
| override func layoutSubviews() { | ||
| super.layoutSubviews() | ||
| contentView.frame = UIEdgeInsetsInsetRect(CGRect( |
There was a problem hiding this comment.
Is this the same as inset(by:)? That'd be a bit more Swifty and readable :)
There was a problem hiding this comment.
That’s for x and y insetting (not UIEdgeInsets)
There was a problem hiding this comment.
Swift 4.2 (codebase still 4.0)
| self?.markdown = markdown | ||
| self?.update() | ||
| case .error(let error): | ||
| print(error?.localizedDescription ?? "Error updating new features") |
There was a problem hiding this comment.
Just a print not user facing
| } | ||
| } | ||
|
|
||
| override func createBinders(from value: String) -> [ListBinder] { |
There was a problem hiding this comment.
This is some... interesting code. Having difficulties parsing/understanding this completely :)
There was a problem hiding this comment.
Takes some getting used to. Heavy use of generics but result is completely type safe.
| private func showChanges(markdown: String) { | ||
| viewController?.showContextualMenu(IssuePreviewViewController( | ||
| markdown: markdown, | ||
| owner: "GitHawkApp", |
There was a problem hiding this comment.
I think someone added the GitHawk owner/repo combination in a constant somewhere? Not sure who/where.
There was a problem hiding this comment.
@BasThomas Yup I did! But its in the Templates pull request which hasn't been merged
| switch result { | ||
| case .success(let markdown): | ||
| self?.markdown = markdown | ||
| self?.update() |
| markdown: markdown, | ||
| owner: "GitHawkApp", | ||
| repo: "GitHawk", | ||
| title: controller.version, |
There was a problem hiding this comment.
Potentially: What's new in v1.0.1?
There was a problem hiding this comment.
It’s a small window, wanna keep it brief to avoid truncation
| ) | ||
| } | ||
|
|
||
| override func accessibilityPerformEscape() -> Bool { |
|
@rnystrom Any chance this could have impacted the inbox zero icon? I'm getting a blank screen |
cc @BasThomas