Reader: Implement new cells #23670
Reader: Implement new cells #23670
Conversation
kean
force-pushed
the
task/reader-reset-discover-cells
branch
from
4d63084 to
75f5d8f
Compare
Generated by 🚫 Danger |
|
| App Name |  WordPress Alpha |
|
| Configuration | Release-Alpha | |
| Build Number | pr23670-f338663 | |
| Version | 25.4 | |
| Bundle ID | org.wordpress.alpha | |
| Commit | f338663 | |
| App Center Build | WPiOS - One-Offs #10823 |
|
| App Name |  Jetpack Alpha |
|
| Configuration | Release-Alpha | |
| Build Number | pr23670-f338663 | |
| Version | 25.4 | |
| Bundle ID | com.jetpack.alpha | |
| Commit | f338663 | |
| App Center Build | jetpack-installable-builds #9865 |
kean
force-pushed
the
task/reader-reset-discover-cells
branch
from
75f5d8f to
8d62e17
Compare
| import UIKit | ||
|
|
||
| // Fetches URLs for favicons for sites. | ||
| actor FaviconService { |
There was a problem hiding this comment.
It is an experimental implementation to show site icons in "Discover" (icons not presented in the API) and for RSS subscriptions (never supported site icons). For "Discover", we'll might need to revisit this and check with the Loop team if there are any other options. For RSS, we'll still need this, but it might be worth integrating something like https://github.com/will-lumley/FaviconFinder instead.
| import Foundation | ||
|
|
||
| struct ReaderBlockingHelper { | ||
| func blockSite(forPost post: ReaderPost, context: NSManagedObjectContext = ContextManager.shared.mainContext) { |
There was a problem hiding this comment.
I moved this code and not planning to rework it.
| } | ||
| guard (200..<400).contains(response.statusCode) else { | ||
| throw NSError(domain: "FaviconService", code: -1) | ||
| } |
There was a problem hiding this comment.
URLSession handles redirection automatically, so the status code should never be 3xx. What do you think about only checking the 2xx code here?
Also, considering this error is shown out of the FaviconService API, using an Error type might be better than an NSError with hard-code values.
| detailsLabel.font = .preferredFont(forTextStyle: .subheadline) | ||
| detailsLabel.textColor = .secondaryLabel | ||
| detailsLabel.adjustsFontForContentSizeCategory = true | ||
| titleLabel.maximumContentSizeCategory = .accessibilityExtraLarge |
There was a problem hiding this comment.
detailsLabel instead of titleLabel?
| } | ||
|
|
||
| private func kFormatted(_ count: Int) -> String { | ||
| count >= 1000 ? String(format: "%.0fK", Double(count) / 1000) : String(count) |
There was a problem hiding this comment.
It appears Foundation has an API (with localized output) for us.
1> import Foundation
2> 1234.formatted(.number.notation(.compactName))
$R0: Foundation.IntegerFormatStyle<Int>.FormatOutput = "1.2K"
3> 1234567.formatted(.number.notation(.compactName))
$R1: Foundation.IntegerFormatStyle<Int>.FormatOutput = "1.2M"
4> 1234567.formatted(.number.locale(Locale(identifier:"zh_CN")).notation(.compactName))
$R2: Foundation.IntegerFormatStyle<Int>.FormatOutput = "123万"
There was a problem hiding this comment.
Nice, I tried to look for a native API but couldn't find anything.
| configureStyle() | ||
| configureLayout() | ||
| configureActions() | ||
| configureAccessibility() |
There was a problem hiding this comment.
Nitpick: These functions are for initialization only. My impression of the name configure is it can be called multiple times to update appearance. What do you think renaming then to initializeXxx.
There was a problem hiding this comment.
I'm going to update these to setup*.
| if let imageURL = viewModel.imageURL { | ||
| imageView.setImage(with: imageURL) | ||
| } else { | ||
| imageView.isHidden = true |
There was a problem hiding this comment.
Nitpick: I see isHidden is reset in prepareForReuse, but setting it to fasle in the if branch above may help with local reasoning.
| cell.layer.borderWidth = 0.5 | ||
|
|
||
| return vc | ||
| } |
There was a problem hiding this comment.
TIL that you can preview UIKit!
| await self.unsubscribe(subscriptionID, key: siteURL) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
This function seems very carefully coded. What do you think about adding a few simple unit tests to prevent future us from breaking it?
- Call
favicon(forURL:)from multiple threads concurrently and make sure there is only one HTTP request sent out. Task { favicon(forURL:) }.cancel()cancels the HTTP request.
There was a problem hiding this comment.
I'm not sure we'll continue using this service, so I don't think it's worth investing time into it right now. The task coalescing implementation is copied from ImageDownloader that has (some) tests.
| if isP2 { | ||
| self.avatarURL = post.authorAvatarURL.flatMap(URL.init) | ||
| } else if let avatarURL = post.siteIconForDisplay(ofSize: Int(ReaderPostCell.avatarSize)) { | ||
| self.avatarURL = avatarURL |
| self.isCommentsEnabled = true | ||
| self.commentCount = 213 | ||
| self.isLiked = true | ||
| self.post = nil |
There was a problem hiding this comment.
Is it possible to create a ReaderPost instance and use the initialize above? That way we don't have to use the special init() function for preview. We can change to use static func forPreview() { self.init(post: ReaderPost(...)) } instead. Also, the post property would become non-optional.
There was a problem hiding this comment.
I made it non-optional but kept the initializer because it makes it much easier to test different variations. It's much harder to do with ReaderPost that has ~100 properties and with non-functioning autocomplete (presumable because it's an ObjC class).
kean
force-pushed
the
task/reader-reset-discover-cells
branch
from
826db1d to
f338663
Compare
|
I had to rebase but I put the PR comment updates in a separate commit. |
* Implement new ReaderPostCell * Address PR comments
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
This PR adds the initial implementation of the new post cells for the Reader. The new cells use smaller vertical space, have better accessibility support for both dynamic type and voice over, and has new menus.
There is still a little bit more work left, but it's mergable and it's completely under the new feature flag.
Changes
Before and After
The vertical size usage comparison:
Dynamic Type
Context Menus
To test:
Regression Notes
Potential unintended areas of impact
What I did to test those areas of impact (or what existing automated tests I relied on)
What automated tests I added (or what prevented me from doing so)
PR submission checklist:
RELEASE-NOTES.txtif necessary.Testing checklist: