Reader: Add Discover Channels #23676
Conversation
Generated by 🚫 Danger |
|
| App Name |  WordPress Alpha |
|
| Configuration | Release-Alpha | |
| Build Number | pr23676-0f924a0 | |
| Version | 25.4 | |
| Bundle ID | org.wordpress.alpha | |
| Commit | 0f924a0 | |
| App Center Build | WPiOS - One-Offs #10836 |
|
| App Name |  Jetpack Alpha |
|
| Configuration | Release-Alpha | |
| Build Number | pr23676-0f924a0 | |
| Version | 25.4 | |
| Bundle ID | com.jetpack.alpha | |
| Commit | 0f924a0 | |
| App Center Build | jetpack-installable-builds #9878 |
| /// Pins edges of the view to the edges of the given container. By default, | ||
| /// pins to the superview. | ||
| @discardableResult | ||
| public func pinEdges( |
There was a problem hiding this comment.
Nitpick: update the signature to be pin(edges: .all, to: view, ...) instead of pinEdges(.all, to: view, ...)?
There was a problem hiding this comment.
It won't be clear what it does with a default argument view.pin(to: view). It has to have the default argument because you'll use .all 99% of the time.
| assertionFailure("view has to be installed in the view hierarchy") | ||
| return [] | ||
| } | ||
| translatesAutoresizingMaskIntoConstraints = false |
There was a problem hiding this comment.
Add an assertion (self.isDescendant(of: container)) to make sure self is a subview of container, because this line may not be appropriate, depending on the relationship between self and container.
// OK to set `subview.translatesAutoresizingMaskIntoConstraints = false`
// because subview's frame is set by auto-layout.
subview.pinEdges(to: superview)
// Not okay to set `superview.translatesAutoresizingMaskIntoConstraints = false`
// because we don't know if superview's frame is set by auto-layout or not.
superview.pinEdges(to: subview)There was a problem hiding this comment.
make sure self is a subview of container
It's not a strict requirement. You can pin a view to a layout guide or to a sibling view. It should be pretty clear from the method's name what it does. SnapKit, PureLayout, and a couple of other frameworks all pretty much work the same way, so it shouldn't surprise that that's how it works.
| @discardableResult | ||
| public func pinEdges( | ||
| _ edges: Edge.Set = .all, | ||
| to container: AutoLayoutItem? = nil, |
There was a problem hiding this comment.
It might be good to make this container part of function signature, so that the relationship is clearer: view.pinEdges(toContainer: anotherView) instead of view.pinEdges(to: anotherView).
There was a problem hiding this comment.
The "container" isn't the best term to use because you can pin the edges to a layout guide view.pinEdges(to: view.safeAreaLayoutGuide) or to a sibling view (not a container). I also replaced the variable name it a more generic term "target".
| pin(.top, topAnchor.constraint(greaterThanOrEqualTo: container.topAnchor, constant: insets.top)) | ||
| pin(.trailing, trailingAnchor.constraint(lessThanOrEqualTo: container.trailingAnchor, constant: -insets.right)) | ||
| pin(.bottom, bottomAnchor.constraint(lessThanOrEqualTo: container.bottomAnchor, constant: -insets.bottom)) | ||
| pin(.leading, leadingAnchor.constraint(greaterThanOrEqualTo: container.leadingAnchor, constant: insets.left)) |
There was a problem hiding this comment.
It doesn't really make a difference in runtime, but using NSDirectionalEdgeInsets instead of UIEdgeInsets would make this line looks more consistent: pin(.leading, ....insets.leading).
| let fetchRequest: NSFetchRequest<NSFetchRequestResult> = NSFetchRequest(entityName: ReaderCard.entityName()) | ||
| let deleteRequest = NSBatchDeleteRequest(fetchRequest: fetchRequest) | ||
| let coordinator = persistentContainer.persistentStoreCoordinator | ||
| try coordinator.execute(deleteRequest, with: mainContext) |
There was a problem hiding this comment.
NSBatchDeleteRequest API doc says we need to manually merge the changes to the context object. Does this API do that for us? Its API doc is pretty short, and doesn't mention that.
There was a problem hiding this comment.
I found an existing method that removes cards in ReaderCardService and replaced batch delete with it. There aren't going to be many of these cards, so the performance should be fine.
| scrollView.addSubview(channelsStackView) | ||
| scrollView.showsHorizontalScrollIndicator = false | ||
| scrollView.clipsToBounds = false | ||
| channelsStackView.pinEdges() |
There was a problem hiding this comment.
Nitpick: it's not obvious what views are involved from this line alone. The default parameter values may save us from typing a few words, but personally, I'd prefer the better readability of having necessary parts in a statement. i.e. channelsStackView.pinEdges(toContainer: self)
Alternatively, using margins instead of pinEdges might be better for conveying the idea that insets are adding between the view and its parent: channelsStackView.margins()
There was a problem hiding this comment.
We could add a separate pinEdgesToSuperview method but I didn't want to duplicate this entire signature.
|
Updates:
|
pinEdgesandpinCenterAutoLayout APIs to replace the now deprecated previous inflexible oneschangeLayoutMarginsreaderDiscoverEndpointFFRegression 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: