-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove MRProgress #21438
Remove MRProgress #21438
Conversation
📲 You can test the changes from this Pull Request in WordPress Alpha by scanning the QR code below to install the corresponding build.
|
📲 You can test the changes from this Pull Request in Jetpack Alpha by scanning the QR code below to install the corresponding build.
|
- (instancetype)initWithCoder:(NSCoder *)coder | ||
{ | ||
NSAssert(false, @"WPProgressTableViewCell can't be created using a nib"); | ||
return [super initWithCoder:coder]; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice addition.
override func tintColorDidChange() { | ||
super.tintColorDidChange() | ||
updateAppearance() | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at the videos in the PR description, the tint color of both implementations stands out as the default iOS color against the rest of the Jetpack app the brand green.
Sort of out of scope for this PR, given that the MRProgress implementation also had this inconsistency, but maybe in building this you might have noticed an appropriate spot in the codebase where to set the tint color?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I noticed that. I didn't look into it though. 😓
- MRProgress (0.8.3): | ||
- MRProgress/ActivityIndicator (= 0.8.3) | ||
- MRProgress/Blur (= 0.8.3) | ||
- MRProgress/Circular (= 0.8.3) | ||
- MRProgress/Icons (= 0.8.3) | ||
- MRProgress/NavigationBarProgress (= 0.8.3) | ||
- MRProgress/Overlay (= 0.8.3) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also great to be getting rid of all these other sub-dependencies that we were carrying around without actually using them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Marius used to be a CocoaPods contributor, so he probably used it for testing subspecs 👀
Background
The last change to the MRProgress library is 7 years ago. I had to do #21235 recently to make it compile on Xcode 15. Considering it's only used in one place (changing a post's feature image), I have implemented the view we needed and removed the library from the codebase.
Changes
The new
StoppableProgressIndicatorView
type is our replacement ofMRActivityIndicatorView
. To make the diff as small as possible, I have made the new type's API compatible with theMRProgress
one.The view is used to display a progress indicator while the feature image is being uploaded. Here is a side by side appearance comparison:
MRActivityIndicatorView
StoppableProgressIndicatorView
Progress-before.mp4
Progress-after.mp4
Test Instructions
Go to a post's "Post Settings" and update the post's featured image. Verify different kinds of operations: stop, cancel, re-upload the image, etc.
If you want to inspect the progress indicator animation, you can make following local changes to keep the animation forever:
git appply /path/to/diff
Regression Notes
Potential unintended areas of impact
None.
What I did to test those areas of impact (or what existing automated tests I relied on)
The "Test Instructions"
What automated tests I added (or what prevented me from doing so)
None.
PR submission checklist:
RELEASE-NOTES.txt
if necessary.UI Changes testing checklist: