Translate BlogDetailsViewController to Swift #24981
Conversation
Generated by 🚫 Danger |
|
| App Name | WordPress | |
| Configuration | Release-Alpha | |
| Build Number | 30050 | |
| Version | PR #24981 | |
| Bundle ID | org.wordpress.alpha | |
| Commit | 0b06d52 | |
| Installation URL | 1homnrvpvsq8o |
|
| App Name | Jetpack | |
| Configuration | Release-Alpha | |
| Build Number | 30050 | |
| Version | PR #24981 | |
| Bundle ID | com.jetpack.alpha | |
| Commit | 0b06d52 | |
| Installation URL | 42fcsnqcue0qg |
Porting the original Objective-C code
kean
left a comment
kean
left a comment
There was a problem hiding this comment.
I smoke-tested the changes on iPhone and iPad and it worked as expected. Left a couple of comments.
| } | ||
| } | ||
|
|
||
| @objc public final class BlogDetailsTableViewModel: NSObject { |
There was a problem hiding this comment.
Most of this code typically belongs to the ViewController: cell registration, cell configuration, sections, delegate & data source, scrolling, etc. Would it be feasible to move it to the ViewController?
There was a problem hiding this comment.
The structure is mainly a byproduct of the gradual ObjC code to Swift translation. We can probably move things around and refine the structure. But I think that can be done later if needed. The purpose of this PR is to make the view controller a Swift implementation, so that we don't have to jump between ObjC and Swift code when adding a row or something to the blog details screen.
There was a problem hiding this comment.
But I think that can be done later if needed.
That sounds good. This code definitely doesn't belong to a ViewModel though. I would appreciate if you could create a follow up PR to clean things up a bit. Approving as is for now.
| static func home(viewController: BlogDetailsViewController?) -> Row { | ||
| Row( | ||
| kind: .home, | ||
| title: NSLocalizedString("Home", comment: "Noun. Links to a blog's dashboard screen."), |
There was a problem hiding this comment.
(nit) It'd be great to add namespace-based keys for this file
| title: BlogDetailsViewController.Strings.subscribers, | ||
| image: UIImage(named: "wpl-mail"), | ||
| action: { [weak viewController] _ in | ||
| MainActor.assumeIsolated { |
There was a problem hiding this comment.
(nit) MainActor.assumeIsolated seems unnecessary. if it is, I'd suggest making action @MainActor.
There was a problem hiding this comment.
The assumeIsolated is added to fix a compilation issue due to SubscribersBlog being bound to the main actor.
| } | ||
|
|
||
| @objc public func shouldAddJetpackSection() -> Bool { | ||
| public func shouldAddJetpackSection() -> Bool { |
There was a problem hiding this comment.
These methods look particularly out of place with most of the UI code in the ViewModel (see previous comment). This code actually does seem that it might belong to a ViewModel.
(nit) I'd suggest to put these in a single shouldShow(_ section: Section) method.
| } | ||
| enum Strings { | ||
| static let contentSectionTitle = NSLocalizedString( | ||
| "my-site.menu.content.section.title", |
There was a problem hiding this comment.
(nit) It should be camelCase (mySite.*). Marking as nit as it's the existing code.
|
kean
left a comment
kean
left a comment
There was a problem hiding this comment.
Approved with one outstanding comments regarding the ViewModel.
Description
The majority of the changes are translated, without much of the swiftification. I made small improvements here and there, but not a lot.
Testing instructions
The best way to test this change is to run the trunk branch and this branch on the iPad and iPhone simulators. Go to a WP.com site and a self-hosted site. You should not notice any difference in the blog details, which can be