Skip to content
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

Fix Quick Start 'Follow other sites' tour #13092

Merged
merged 4 commits into from Dec 13, 2019
Merged

Conversation

@ScoutHarris
Copy link
Contributor

ScoutHarris commented Dec 13, 2019

Fixes #13068

In iOS13, ManyDelegateNavigationController no longer worked, causing tourGuide.visited(.readerBack) to not be called. Meaning the tour was never notified the readerBack step was completed.

To fix this:

  • ManyDelegateNavigationController has been removed.
  • QuickStartNavigationWatcher is now QuickStartNavigationSettings, and is no longer a UINavigationControllerDelegate.
  • QuickStartNavigationSettings is updated in WPSplitViewController's navigationController:willShow.

To test:

  • Follow the repro steps noted on #13068 . (Thank you @rachelmcr for excellent repro steps.)
    • Hack: you can show the tour checklist by returning true in QuickStartTourGuide:shouldShowChecklist.
  • Verify:
    • After tapping the Reader tab, the nav bar button < Reader is spotlighted.
      • In split view (like on an iPad) < Reader is not shown, so this step is skipped (this is existing functionality).
    • The Tap < Reader to continue hint is dismissed.
    • When you finish the tour (i.e. by tapping Reader > Search) the tour is marked as complete.

PR submission checklist:

  • I have considered adding unit tests where possible.

  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

@ScoutHarris ScoutHarris added this to the 13.9 milestone Dec 13, 2019
@ScoutHarris ScoutHarris requested a review from frosty Dec 13, 2019
@ScoutHarris ScoutHarris self-assigned this Dec 13, 2019
@peril-wordpress-mobile

This comment has been minimized.

Copy link

peril-wordpress-mobile bot commented Dec 13, 2019

You can trigger an installable build for these changes by visiting CircleCI here.

@frosty
frosty approved these changes Dec 13, 2019
Copy link
Contributor

frosty left a comment

Thank you for fixing this!

@@ -14,6 +14,8 @@ import WordPressShared

class WPSplitViewController: UISplitViewController {

private let quickStartNavigationSettings = QuickStartNavigationSettings()

This comment has been minimized.

Copy link
@frosty

frosty Dec 13, 2019

Contributor

In an ideal world, WPSplitViewController wouldn't need to know anything about quick start settings (hence the previous implementation), but as that's not working any more and there's a chance we may revisit WPSplitViewController in the not too distant future I think this is okay for now.

@ScoutHarris ScoutHarris merged commit 0291056 into develop Dec 13, 2019
7 checks passed
7 checks passed
Hound No violations found. Woof!
Peril All green. Nice work.
Details
ci/circleci: Build Tests Your tests passed on CircleCI!
Details
ci/circleci: Installable Build/Hold Your job is on hold on CircleCI!
Details
ci/circleci: UI Tests (iPad Air 3rd generation) Your tests passed on CircleCI!
Details
ci/circleci: UI Tests (iPhone 11) Your tests passed on CircleCI!
Details
ci/circleci: Unit Tests Your tests passed on CircleCI!
Details
@ScoutHarris ScoutHarris deleted the fix/13068-quick_start_follow branch Dec 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.