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

Add Jetpack powered banner to Reader Search #19059

Merged
merged 12 commits into from
Jul 21, 2022

Conversation

Gio2018
Copy link
Contributor

@Gio2018 Gio2018 commented Jul 15, 2022

Fixes #NA

This PR adds a "Jetpack powered" banner in the reader search of the WordPress app (see screenshots).
The banner won't be visible on iPhone landscape.
This PR also removes (temporarily) the "Search WordPress for a site or a post" label that appeared below the "Clear search history" button.

Newer iPhone portrait iPhone landscape older iPhone portrait with larger fonts

To test:

  • build/run the WordPress target from this branch
  • Go to Reder and select search in the top-right corner
  • on iPad and iPhone portrait: make sure that the "Jetpack powered" banner appears
  • Rotating to landscape on iPhone (or starting the app in landscape) make sure that the banner does not appear
  • Stop the execution and build/run the Jetpack target from this branch
  • Repeat the above steps and make sure that the "Jetpack powered" banner does not show in any step.

Regression Notes

  1. Potential unintended areas of impact
    Low, it adds an inputAccessoryView to the serchBar in ReaderSearchViewController
    This PR also adjusts heights calculation in ReaderSearchSuggestionsViewController and removes a label in ReaderSearchViewController

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

  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. Not released.

@Gio2018 Gio2018 added this to the 20.4 milestone Jul 15, 2022
@Gio2018 Gio2018 requested review from alpavanoglu and sla8c July 15, 2022 22:55
@Gio2018 Gio2018 self-assigned this Jul 15, 2022
@wpmobilebot
Copy link
Contributor

wpmobilebot commented Jul 15, 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 pr19059-30f0d87 on your iPhone

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

@wpmobilebot
Copy link
Contributor

wpmobilebot commented Jul 15, 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 pr19059-30f0d87 on your iPhone

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

@Gio2018 Gio2018 requested a review from shaunandrews July 15, 2022 23:26
@Gio2018 Gio2018 requested a review from twstokes July 19, 2022 02:28
@twstokes
Copy link
Contributor

👋 @Gio2018, we may need to do some tweaks since this PR added the banner to some areas of Reader (including the main search view).

Running the latest on this branch I see:

@Gio2018
Copy link
Contributor Author

Gio2018 commented Jul 19, 2022

👋 @Gio2018, we may need to do some tweaks since this PR added the banner to some areas of Reader (including the main search view).

Thanks for bringing this up @twstokes ! I am now hiding the bar from reader stream (if present). That issue though is still there on newer iPhones. It's the accessory view still showing up when an hardware keyboard is connected. Does not happen on iPad. Not sure if we want to add logic to handle that, or treat it as an edge case, considering that it does not seem to happen on iPad? Wdyt?

@Gio2018 Gio2018 requested a review from sla8c July 19, 2022 22:54
@sla8c
Copy link
Contributor

sla8c commented Jul 20, 2022

@Gio2018 FYI I've noticed unit tests were failing; I re-triggered in case it was a build kite issue

@twstokes
Copy link
Contributor

👋 @Gio2018 - how are we treating Reader search for self-hosted sites not logged into WordPress.com?

Since being logged into .com + Jetpack would enable the ability to star / favorite / re-blog, should we not show the banner in this scenario?

@twstokes
Copy link
Contributor

👋 @Gio2018, we may need to do some tweaks since this PR added the banner to some areas of Reader (including the main search view).

Thanks for bringing this up @twstokes ! I am now hiding the bar from reader stream (if present). That issue though is still there on newer iPhones. It's the accessory view still showing up when an hardware keyboard is connected. Does not happen on iPad. Not sure if we want to add logic to handle that, or treat it as an edge case, considering that it does not seem to happen on iPad? Wdyt?

Thanks for the info! It sounds like we're probably OK to keep it like it is, then. 👍

Copy link
Contributor

@twstokes twstokes left a comment

Choose a reason for hiding this comment

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

LGTM @Gio2018 🚀 - I just had one comment on if this should be shown for self-hosted users who aren't logged into .com / connected to Jetpack.

I tested:

  • Jetpack app
  • iPad
  • Dark mode
  • a11y
  • Different orientations

@Gio2018
Copy link
Contributor Author

Gio2018 commented Jul 20, 2022

👋 @Gio2018 - how are we treating Reader search for self-hosted sites not logged into WordPress.com?

Since being logged into .com + Jetpack would enable the ability to star / favorite / re-blog, should we not show the banner in this scenario?

Great point @twstokes ! As discussed on Slack, this will be covered once we move to the single flag that performs all the due checks.

@Gio2018
Copy link
Contributor Author

Gio2018 commented Jul 20, 2022

@Gio2018 FYI I've noticed unit tests were failing; I re-triggered in case it was a build kite issue

Thanks for re-running the tests @sla8c ! it seems that they succeeded this time.

@Gio2018 Gio2018 merged commit 1bb3298 into trunk Jul 21, 2022
@Gio2018 Gio2018 deleted the task/reader-search-jetpack-banner branch July 21, 2022 03:20
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.

4 participants