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

[Jetpack Focus] Add Jetpack banner to People view #19797

Merged
merged 5 commits into from
Jan 5, 2023

Conversation

twstokes
Copy link
Contributor

@twstokes twstokes commented Dec 21, 2022

Part of #19448

Description

  • Adds the Jetpack banner to the People view in the WordPress app if JetpackBrandingCoordinator.shouldShowBannerForJetpackDependentFeatures() returns true
  • Adds the Jetpack badge to the Person view in the WordPress app if JetpackBrandingCoordinator.shouldShowBannerForJetpackDependentFeatures() returns true
My Site People Person
image image image

Known issues:

  • When testing with three people added to a site and an iPhone 14 in landscape, this issue occurred.
  • For large Dynamic Type sizes extra padding can be added to the right side of the badge.

Testing

Banner and badge only appear during phases 2 and 3

Toggle the Jetpack Features Removal feature flags and confirm that:

  1. Only when phases Two and / or Three are enabled (and are the highest phases) do the banner and badge appear.
  2. All other feature flag combinations result in the banner and badge being hidden or the view being inaccessible (e.g. all off, One enabled, Four enabled, New Users).

Banner appears in the People view in WordPress

  1. Load the WordPress app.
  2. From My Site, tap People in the Configure section.
  3. Expect: The Jetpack banner to appear at the bottom of the screen.
  4. Expect: Tapping on the Jetpack banner shows the Jetpack overlay.
  5. Expect: The event 🔵 Tracked: jetpack_powered_banner_tapped <screen: people> to be fired.

Badge appears in the Person view in WordPress

  1. Load the WordPress app.
  2. From My Site, tap People in the Configure section.
  3. Tap a person in the list.
  4. Expect: The Jetpack badge to appear at the bottom of the screen.
  5. Expect: Tapping on the Jetpack badge shows the Jetpack overlay.
  6. Expect: The event 🔵 Tracked: jetpack_powered_badge_tapped <screen: person> to be fired.

Banner and badge do not appear in Jetpack

  1. Load the Jetpack app.
  2. From My Site, tap People in the Configure section.
  3. Expect: No Jetpack banner to be shown for any phase combination.
  4. Tap a person in the list.
  5. Expect: No Jetpack badge to be shown for any phase combination.

Banner visibility behavior

  • Initially the banner should be shown when the screen appears.
  • Scrolling down hides the banner and the banner reappears only when the view is scrolled to the top.
  • If there isn't enough content to scroll, the banner doesn't show / hide on scroll.

Items to test:

  • Light / Dark mode
  • Dynamic Type / a11y
  • Different screen orientations
  • iPhone / iPad

Regression Notes

  1. Potential unintended areas of impact

    • People view functionality.
    • There's a risk I missed something when adding this conditional, but my manual tests all passed. It would be good to test viewing users with different roles so the "Remove" button's visibility is toggled.
  2. What I did to test those areas of impact (or what existing automated tests I relied on)

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

    • None.

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.

@twstokes
Copy link
Contributor Author

Cross-referencing in case it's of any use: #19103

@wpmobilebot
Copy link
Contributor

wpmobilebot commented Dec 21, 2022

You can test the changes in Jetpack from this Pull Request by:
  • Clicking here or scanning the QR code below to access App Center
  • Then installing the build number pr19797-25e5cbe on your iPhone

If you need access to App Center, please ask a maintainer to add you.

@wpmobilebot
Copy link
Contributor

wpmobilebot commented Dec 21, 2022

You can test the changes in WordPress from this Pull Request by:
  • Clicking here or scanning the QR code below to access App Center
  • Then installing the build number pr19797-25e5cbe on your iPhone

If you need access to App Center, please ask a maintainer to add you.

@twstokes twstokes self-assigned this Jan 3, 2023
@twstokes twstokes marked this pull request as ready for review January 3, 2023 22:40
Copy link
Contributor

@hassaanelgarem hassaanelgarem left a comment

Choose a reason for hiding this comment

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

I've tested all the listed scenarios and it works as expected 🚀

Regarding the two know issues you've mentioned, I was able to reproduce both. These look like low priority issues, but I was curios, do you think we should track them somewhere in case we have time to tackle them in the future?

@twstokes
Copy link
Contributor Author

twstokes commented Jan 4, 2023

I've tested all the listed scenarios and it works as expected 🚀

Thanks @hassaanelgarem! I just found a minor issue that I'll tweak, the view's title is missing now that it's wrapped.

Regarding the two know issues you've mentioned, I was able to reproduce both. These look like low priority issues, but I was curios, do you think we should track them somewhere in case we have time to tackle them in the future?

I think that's a good idea and will create issues for them. 👍

@twstokes
Copy link
Contributor Author

twstokes commented Jan 4, 2023

Thanks @hassaanelgarem! I just found a minor issue that I'll tweak, the view's title is missing now that it's wrapped.

@hassaanelgarem this should be resolved with 25e5cbe. I've also updated the screenshot in this PR to match.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants