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

Integrate TipKit #23470

Merged
merged 12 commits into from
Aug 13, 2024
Merged

Integrate TipKit #23470

merged 12 commits into from
Aug 13, 2024

Conversation

kean
Copy link
Contributor

@kean kean commented Aug 2, 2024

TipKit

In #23469, we removed the LoginEpilogue screen, and the app now automatically selects one of your sites after login, defaulting to your primary site. To make sure the users learn how to switch to a different site, this PR integrate Apple's TipKit.

The first time you log in with an existing account with two or more sites, the app will show the "Your Sites" tip. The tip is shown only once. If you log in on a different device with the same Apple ID, the previously shown tips are not shown again.

Screenshot 2024-08-02 at 3 39 20 PM

Resources

Quick Start

TipKit will also be used as a replacement for the existing Quick Start tours.

This PR removes the "What would you like to focus on first?" screen. It was added for the following reasons:

[**] We'll now ask users logging in which area of the app they'd like to focus on to build towards a more personalized experience. [#18385]"

There was no follow-up, and this screen does nothing other than simply opening one of the selected primary areas in the app, but only after showing the "Enable Notifications?" screen.

Screenshot 2024-08-12 at 1 29 09 PM

This screen is also the entry point for the "existing site" Quick Start tour – the "Not sure, show me around" option. By removing it, you can no longer initiate this tour.

Screenshot 2024-08-12 at 1 40 48 PM

The next step would be to consider if there are any good candidates for being replaced with TipKit.

Debug Options

I added the following launch arguments to the schemes.

Screenshot 2024-08-12 at 10 07 47 AM

The same options are also added to the Debug Menu.

Screenshot 2024-08-12 at 1 27 49 PM

Testing

  • Verify that the "Enable Notifications" screen is shown on the first login. Noting that in the production version, if you tapped "Skip" on the "Onboarding Questions" screen, the app would never prompt you to enable notifications 😨

Regression Notes

  1. Potential unintended areas of impact

  2. What I did to test those areas of impact (or what existing automated tests I relied on)

  3. What automated tests I added (or what prevented me from doing so)

PR submission checklist:

  • I have completed the Regression Notes.
  • I have considered adding unit tests for my changes.
  • I have considered adding accessibility improvements for my changes.
  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

Testing checklist:

  • WordPress.com sites and self-hosted Jetpack sites.
  • Portrait and landscape orientations.
  • Light and dark modes.
  • Fonts: Larger, smaller and bold text.
  • High contrast.
  • VoiceOver.
  • Languages with large words or with letters/accents not frequently used in English.
  • Right-to-left languages. (Even if translation isn’t complete, formatting should still respect the right-to-left layout)
  • iPhone and iPad.
  • Multi-tasking: Split view and Slide over. (iPad)

@kean kean mentioned this pull request Aug 2, 2024
14 tasks
@wpmobilebot
Copy link
Contributor

wpmobilebot commented Aug 2, 2024

WordPress Alpha📲 You can test the changes from this Pull Request in WordPress Alpha by scanning the QR code below to install the corresponding build.
App NameWordPress Alpha WordPress Alpha
ConfigurationRelease-Alpha
Build Numberpr23470-f83dfeb
Version25.2
Bundle IDorg.wordpress.alpha
Commitf83dfeb
App Center BuildWPiOS - One-Offs #10472
Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

@wpmobilebot
Copy link
Contributor

wpmobilebot commented Aug 2, 2024

Jetpack Alpha📲 You can test the changes from this Pull Request in Jetpack Alpha by scanning the QR code below to install the corresponding build.
App NameJetpack Alpha Jetpack Alpha
ConfigurationRelease-Alpha
Build Numberpr23470-f83dfeb
Version25.2
Bundle IDcom.jetpack.alpha
Commitf83dfeb
App Center Buildjetpack-installable-builds #9512
Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

@kean kean force-pushed the task/remove-login-epiloguevc branch 4 times, most recently from 30cb2bc to 2f220ec Compare August 8, 2024 17:42
@kean kean changed the base branch from task/remove-login-epiloguevc to trunk August 8, 2024 20:28
@kean kean force-pushed the task/integrate-tipkit branch 2 times, most recently from aaf8eff to 7e99347 Compare August 12, 2024 19:54
@kean kean marked this pull request as ready for review August 12, 2024 19:55
extension UIViewController {
/// Registers a popover to be displayed for the given tip.
@available(iOS 17, *)
func registerTipPopover(
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TipKit API for showing popovers was a bit too low-level, so I decided to add this convenience API on top of it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The term "register" gives of nothing will actually happen after this function is called. However, if I understand correctly, the function shows a tip if it certain criteria match. What do you think changing the name to something like presentTipPopoverIfNeeded?

Copy link
Contributor Author

@kean kean Aug 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm open to suggestions, but presentTipPopoverIfNeeded sounds like a one-shot method. This method subscribes to a tip.shouldDisplayUpdates async sequence and shows/hides the popover as needed. So, yeah, nothing happens unless it fires an event.

The SwiftUI method with the same functionality is called popoverTip(_:arrowEdge:action:).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I misunderstood how the tip.shouldDisplayUpdates API works. The function name makes more sense to me now.

@@ -102,6 +102,18 @@
argument = "-debugJetpackConnectionFlow"
isEnabled = "NO">
</CommandLineArgument>
<CommandLineArgument
argument = "-com.apple.TipKit.ShowAllTips 1"
isEnabled = "NO">
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These options are all disabled by default.

@kean kean changed the title Integrate TipKit (Experimental) Integrate TipKit Aug 12, 2024
@kean kean requested a review from crazytonyli August 12, 2024 19:57
@kean kean added the General label Aug 12, 2024
@kean kean added this to the Pending milestone Aug 12, 2024
@kean
Copy link
Contributor Author

kean commented Aug 12, 2024

Here's an example of a complete flow:

Simulator.Screen.Recording.-.iPhone.15.Pro.-.2024-08-12.at.15.53.34.mp4

} else {
if self?.presentedViewController is TipUIPopoverViewController {
self?.dismiss(animated: true)
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you need to break out of the for loop when present or dismiss is called?

extension UIViewController {
/// Registers a popover to be displayed for the given tip.
@available(iOS 17, *)
func registerTipPopover(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The term "register" gives of nothing will actually happen after this function is called. However, if I understand correctly, the function shows a tip if it certain criteria match. What do you think changing the name to something like presentTipPopoverIfNeeded?

static func initialize() {
guard Feature.enabled(.tipKit), #available(iOS 17, *) else { return }
do {
try Tips.configure([.displayFrequency(.daily)])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there are many Tips, does this daily frequency configuration mean other tips won't be displayed today if one of them has been displayed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I forgot to remove it after testing – done.
It might be something we go for in the future, but there is no need for it now.

return nil
}
let task = Task { @MainActor [weak self] in
for await shouldDisplay in tip.shouldDisplayUpdates {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you'll need a if Task.isCancelled { break } in this for-loop? task.cancel() alone probably won't stop the loop.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code is from the Apple sample code. I'm not sure if it's clearly documented anywhere, but tip.shouldDisplayUpdates async sequence (AsyncMapSequence) receives cancellation from the task, aka cooperative cancellation.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah okay. That's quite possible. Ignore my comments then, if the task can be properly cancelled. 🙂

@kean kean requested a review from crazytonyli August 13, 2024 15:53
@kean kean added this pull request to the merge queue Aug 13, 2024
Merged via the queue into trunk with commit 25909ec Aug 13, 2024
24 checks passed
@kean kean deleted the task/integrate-tipkit branch August 13, 2024 22:47
@crazytonyli crazytonyli mentioned this pull request Aug 13, 2024
14 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants