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

About screen: Disable haptics when app logos cell isn't visible #17686

Merged
merged 2 commits into from Dec 21, 2021

Conversation

frosty
Copy link
Contributor

@frosty frosty commented Dec 18, 2021

This PR can be used to test the associated AutomatticAbout-Swift PR: Automattic/AutomatticAbout-Swift#4. It makes some changes to the haptics implementation in the about screen so that haptics are only active when the app logos cell on the new About screen is visible.

To test

Presenting new screens

  • Build and run on a device
  • Navigate to the new About screen (in the Me menu)
  • Ensure you can see the app logos cell containing the bubbles, and that you can feel the haptic feedback
  • Tap the Legal and More menu item, which pushes a new screen. Check that you can no longer feel the haptics. Tap Back, and ensure that the haptics come back.
  • Tap the Blog menu item, which presents a webview. Check that you can no longer feel the haptics. Tap Back, and ensure that the haptics come back.

Scrolling on and offscreen

  • In Xcode, go to AppAboutScreenConfiguration.swift and add the following line about 10 times to the first sub-array in the sections property (line 31) to pad out the top section in the screen with a lot of extra content:
AboutItem(title: "Testing"),

(this should go above the existing AboutItem(title: TextContent.rateUs, action: { [weak self] context in line)

  • Relaunch the app and navigate to the About screen again. The app logos cell should be offscreen and you shouldn't feel any haptics.
  • Scroll down so that the logo cell is visible. You should start feeling haptics.
  • Scroll back up so that the logo cell is no longer visible. You shouldn't feel the haptics any more.

Regression Notes

  1. Potential unintended areas of impact

None

  1. What I did to test those areas of impact (or what existing automated tests I relied on)
  2. What automated tests I added (or what prevented me from doing so)

N/A

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.

@frosty frosty added this to the 19.0 milestone Dec 18, 2021
@frosty frosty requested a review from Gio2018 December 18, 2021 22:42
@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Dec 18, 2021

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

@frosty frosty changed the title About screen: Haptics improvements About screen: Disable haptics when app logos cell isn't visible Dec 18, 2021
@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Dec 18, 2021

You can trigger optional UI/connected tests for these changes by visiting CircleCI here.

Copy link
Contributor

@Gio2018 Gio2018 left a comment

Choose a reason for hiding this comment

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

Hey @frosty , tested this on iPhone 12 Pro Max device with iOS 15.2; I noticed that the haptics do not pause when opening a modal with any web view from the about screen (Blog, Automattic Family and Work With Us). They correctly pause when offscreen or when navigating to legal and more.

@frosty
Copy link
Contributor Author

frosty commented Dec 20, 2021

Hmm you're right, it's not working in the case of modals like the webviews, because they don't trigger the willAppear / disappear view lifecycle methods. As I'm currently wrapping up for the holidays, I'd like to go ahead and merge this as-is for now (as it's still an improvement over what we had already) and I can see if there's anything we can do about the modals in a future update. Is that okay?

@Gio2018 Gio2018 self-requested a review December 21, 2021 00:20
Copy link
Contributor

@Gio2018 Gio2018 left a comment

Choose a reason for hiding this comment

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

Hey @frosty , sure let's merge this one. Since it's not a blocker, we can open an issue for the haptic still happening when the modal web view shows up and treat it in a separate PR. Happy Holidays!!

@frosty frosty merged commit 479b8ca into develop Dec 21, 2021
@frosty frosty deleted the fix/about-screen-haptics branch December 21, 2021 13:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

2 participants