-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Jetpack Focus: Fix UI issues by removing the WPTabBarController dependency
#19817
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
Conversation
…ushNotificationsManager`
with `RootViewPresenter`
…sting the rootVC
staskus
left a comment
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.
👏 a clean refactoring, thanks! I appreciate splitting the PR into small and clear commits, much easier to review.
For most part, it looks like 1 to 1 change from directly accessing VC to accessing VC through a RootViewCoordinator so it increases confidence in solution, even with so many changes. I haven't seen any major flaws looking from the code side.
I left some comments and questions and moving on to a smoke testing.
WordPress/Classes/System/MySitesCoordinator+RootViewPresenter.swift
Outdated
Show resolved
Hide resolved
WordPress/Classes/System/MySitesCoordinator+RootViewPresenter.swift
Outdated
Show resolved
Hide resolved
WordPress/Classes/ViewRelated/Notifications/Controllers/NotificationsViewController.swift
Outdated
Show resolved
Hide resolved
WordPress/Classes/ViewRelated/Reader/Detail/ReaderDetailViewController.swift
Outdated
Show resolved
Hide resolved
WordPress/Classes/ViewRelated/Stats/Insights/SiteStatsInsightsTableViewController.swift
Outdated
Show resolved
Hide resolved
WordPress/Classes/ViewRelated/Blog/My Site/MySiteViewController+OnboardingPrompt.swift
Show resolved
Hide resolved
…FeaturesEnabled()` only
staskus
left a comment
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.
Looks good to me 🚀 👏
I smoke tested described scenarios on WP and JP apps with iPhone, iPad and with feature flags enabled and disabled. I also clicked around all the different menu items but I couldn't find a scenario where the views wouldn't open or open in an unexpected way.
I'm leaving one remaining comment about nullable vs nonnull. Other than that, I think it's worth waiting for at least a second review to have more confidence that I haven't missed any critical scenario.
dvdchr
left a comment
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.
The changes look good, and I haven't encountered any issues. But I did have some notes:
-
When logged in to a WP.com account, there's still the Jetpack-powered banner in the Me screen. Is this intended?
-
I also noticed that with phase four removal enabled, the WP app still "supports" universal link — it doesn't lead anywhere since the
MySitesCoordinatorone is used as the root presenter instance, but it'll trigger thejetpackFeatureIncorrectlyAccessedanalytics.I'm assuming that you're setting up the analytics as some alarm or detection mechanism in case there are unhandled entry points to Jetpack features. Still, since the issue is now identified, I wonder if we should handle it here 🤔 . Then again, I'm not sure if this is already planned for a separate PR, so just let me know if you've already planned for it! 🙂
| RootViewCoordinator.sharedPresenter.showNotificationsTab() | ||
| RootViewCoordinator.sharedPresenter.popNotificationsTabToRoot() |
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.
Does phase four include disabling route handlers that lead to Jetpack-powered features? With the feature flag enabled, triggering a universal link for a Jetpack feature leads to MySitesCoordinator.fallbackBehavior().
@dvdchr We're still in discussion on whether to remove it or not. I'll remove it in a separate PR if we ultimately decide to remove it. (Ref: p1672272774859159-slack-C04564QS0V7)
I'm working on another PR related to this point. Right now, some Jetpack-powered deep links are still functional. For example, if you open a stats deep link, it will show the stats page. I'm working on a PR that'll disable those. However, for all unsupported deep links, it will open the app and trigger the |
WPTabBarController dependencyWPTabBarController dependency
WPTabBarController dependencyWPTabBarController dependency
Generated by 🚫 dangerJS |
|
Sounds good @hassaanelgarem. Thanks for elaborating! 👍🏼 |
Closes #19810
Part of #19812
Description
In the simplified UI, we no longer display a WPTabBarController. However, many areas of the app depend on
WPTabBarController.sharedInstanceto perform some actions. In this PR, we completely remove this dependency and instead depend on the newly addedRootViewCoordinator. By doing so, all issues in the simplified UI that were present in #19816 should be fixed.The PR touches many parts of the app, but most changes are systematic.
Some Jetpack features are still accessible from external sources, like deep linking, widgets, and the 3D touch menu. This will be addressed in a following PR.
Also, blogging reminders and weekly roundups are still not disabled.
Testing Instructions
This PR touches many areas of the app. Therefore there aren't specific testing instructions. Instead, please smoke test the following areas, both in the normal app state and in the simplified UI state (if applicable):
To get to the normal app state, open the debug menu and disable all the Jetpack Features Removal flags. Then kill and relaunch the app.
To get to the simplified UI state, open the debug menu and enable "Jetpack Features Removal Phase Four". Then kill and relaunch the app.
Regression Notes
Potential unintended areas of impact
Mentioned above.
What I did to test those areas of impact (or what existing automated tests I relied on)
Manual testing
What automated tests I added (or what prevented me from doing so)
N/A
PR submission checklist:
RELEASE-NOTES.txtif necessary.