-
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
In-App Updates: Show notice (flexible) or modal (blocking) #23195
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.
|
WordPress/Classes/Services/In-App Update/AppStoreSearchService.swift
Outdated
Show resolved
Hide resolved
@@ -68,6 +68,28 @@ public struct NormalNoticeStyle: NoticeStyle { | |||
public let dismissGesture: NoticeDismissGesture? = nil | |||
} | |||
|
|||
public struct InAppUpdateNoticeStyle: NoticeStyle { |
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.
It looks like it's identical to QuickStartNoticeStyle
.
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.
It is, but I wasn't sure if I'd want to extend it when I implement showing a dismissed notice again after a certain interval. If I don't end up doing that, I'll get rid of it.
WordPress/Classes/Services/In-App Update/InAppUpdateCoordinator.swift
Outdated
Show resolved
Hide resolved
beacc2c
to
cbfe8b1
Compare
Generated by 🚫 Danger |
cbfe8b1
to
a15e93a
Compare
49ef048
to
52c6b34
Compare
Hey @kean, can you re-review this PR? Thanks! 🙇♀️ |
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.
I tested both of the scenarios. The UI did show up + I tested blocking updates and accessibility features.
Feedback:
- (Nit) the button alignment the snackbar is a bit odd. I'd suggest centering them.
- Should there be a feature flag?
- (Noting) The app performs the check on every launch. Ideally, it should only check it once in a while and only if there popup can be shown (based on the business logic – time interval, number of attempts, etc)
- (Note) "Update" button doesn't work, which seems expected based on the TODOs in the code
- I suggest using https://github.com/apple/swift-package-manager/blob/main/Sources/PackageDescription/Version.swift for version parsing and comparison. It supports pre-release identifiers and a couple of other things. It will be a valuable addition to the app.
WordPress/Classes/Services/InAppUpdate/InAppUpdateCoordinator.swift
Outdated
Show resolved
Hide resolved
WordPress/Classes/ViewRelated/InAppUpdate/AppStoreInfoViewModel.swift
Outdated
Show resolved
Hide resolved
WordPress/Classes/ViewRelated/InAppUpdate/AppStoreInfoViewModel.swift
Outdated
Show resolved
Hide resolved
WordPress/Classes/ViewRelated/InAppUpdate/BlockingUpdateView.swift
Outdated
Show resolved
Hide resolved
@kean, thanks for the feedback!
I'll look into this later!
Yes - #23202 (thanks for approving 🙇♀️)
These will be addressed in a separate PR :)
Good shout. Will address this in a future PR |
WordPress/Classes/Services/AppUpdate/AppStoreSearchService.swift
Outdated
Show resolved
Hide resolved
WordPress/Classes/Services/AppUpdate/AppUpdateCoordinator.swift
Outdated
Show resolved
Hide resolved
4c09a91
to
89e1488
Compare
Part of https://github.com/Automattic/wordpress-mobile/issues/55
Part of https://github.com/Automattic/wordpress-mobile/issues/56
Description
How to test
Flexible update
Precondition: On Xcode, change the app version to something lower than the current app store version
Blocking update
Precondition: On Xcode, change the app version to something lower than the current app store version
Minimum OS version
Precondition: On Xcode, change the app version to something lower than the current app store version
InAppCoordinator.swift L62
to "14.0"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.txt
if necessary.Testing checklist: