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 badge to Activity log detail #19007

Merged
merged 25 commits into from
Jul 13, 2022

Conversation

Gio2018
Copy link
Contributor

@Gio2018 Gio2018 commented Jul 7, 2022

Fixes #NA

This PR adds the "Jetpack powered" badge to activity log detail.
It also optimizes code between JetpackButton and JetpackBannerView

To test:

on any iPhone or iPad

WordPress app

  1. Go to My Site -> Menu then select Activity log (make sure to have at least one activity: if you don't, you can, for example, change your site name to generate one)
  2. Tap on any activity
  3. Make sure the badge shows up in the activity detail screen (see screenshot above).

Jetpack app

  1. Repeat the above steps and make sure that the Jetpack powered badge does not show in any of the steps.

Regression Notes

  1. Potential unintended areas of impact
    Low impacts on activity detail UI

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

  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.3 milestone Jul 7, 2022
@Gio2018 Gio2018 requested review from guarani and a team July 7, 2022 20:20
@Gio2018 Gio2018 self-assigned this Jul 7, 2022
@wpmobilebot
Copy link
Contributor

wpmobilebot commented Jul 7, 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 pr19007-725dfec on your iPhone

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

@wpmobilebot
Copy link
Contributor

wpmobilebot commented Jul 7, 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 pr19007-7bc522d on your iPhone

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

@Gio2018 Gio2018 modified the milestones: 20.3, 20.4 Jul 8, 2022
@twstokes twstokes self-requested a review July 11, 2022 19:27
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.

👋 @Gio2018! This looks good to me and passed the manual tests you provided. I've added some comments below.

  1. ✅ "Jetpack powered" badge appears at the bottom of My Site -> Menu on iPhone 13 (WPiOS)
  2. ✅ "Jetpack powered" badge does not appear at the bottom of My Site -> Menu when using a larger phone and rotated to landscape (iPhone 13 Pro Max) or an iPad (WPiOS)
  3. ✅ "Jetpack powered" badge appears in the activity detail screen
  4. ✅ "Jetpack powered" badge does not appear at the bottom of My Site -> Menu or under an activity for any device running Jetpack app
  5. ✅ Badge looks good in dark / light mode
  6. ⚠️ Padding around the text is lost for larger font sizes. Text will show ellipses for very large fonts.
Padding lost Ellipses
  1. ⚠️ Badge appears on self-hosted sites that doesn't have Jetpack installed

  1. ⚠️ Badge's top margin isn't slightly different between the My Site menu and activity detail view
My Site Activity Detail
Screen Shot 2022-07-11 at 15 59 26 Screen Shot 2022-07-11 at 16 00 02
  1. ⚠️ Tapping the badge on the My Site menu shows the outline but does nothing

Screen Shot 2022-07-11 at 15 59 37

Copy link
Contributor

@guarani guarani left a comment

Choose a reason for hiding this comment

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

Make sure the "Jetpack powered" badge appears at the bottom of the list (see screenshot above)

Hi @Gio2018, I think the "Home" tab within "My Site" is Jetpack-powered. The "Menu" tab is not related to Jetpack AFAIK. For example, logging into a self-hosted shows the "Menu" tab only.

I haven't done a complete review, do you want me to review this fully, or is one review (@twstokes's review) what you're looking for?

@Gio2018
Copy link
Contributor Author

Gio2018 commented Jul 12, 2022

Make sure the "Jetpack powered" badge appears at the bottom of the list (see screenshot above)

Hi @Gio2018, I think the "Home" tab within "My Site" is Jetpack-powered. The "Menu" tab is not related to Jetpack AFAIK. For example, logging into a self-hosted shows the "Menu" tab only.

I haven't done a complete review, do you want me to review this fully, or is one review (@twstokes's review) what you're looking for?

Thank you @guarani ! Looking at the Figma designs, the badge also appears in Menu under View Site. I have also just added code to exclude that badge from self hosted sites. Does it make sense?

Giorgio Ruscigno added 2 commits July 13, 2022 10:09
@Gio2018 Gio2018 changed the title Add Jetpack badge to My Site menu and Activity log Add Jetpack badge to Activity log detail Jul 13, 2022
@Gio2018 Gio2018 requested review from guarani and twstokes July 13, 2022 15:17
@Gio2018
Copy link
Contributor Author

Gio2018 commented Jul 13, 2022

Thanks for reviewing @twstokes @guarani ! This is now ready for another pass!

override init(frame: CGRect) {
super.init(frame: frame)
setup()

Copy link
Contributor

Choose a reason for hiding this comment

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

Micro, optional nit: Extra spacing.

titleLabel?.adjustsFontSizeToFitWidth = true
setImage(.gridicon(.plans), for: .normal)
contentVerticalAlignment = .fill
contentHorizontalAlignment = .center
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like this may be a default?

}

func setup() {

Copy link
Contributor

Choose a reason for hiding this comment

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

Micro, optional nit: Extra spacing.

@@ -36,6 +36,9 @@ class ActivityDetailViewController: UIViewController, StoryboardLoadable {
}
}

@IBOutlet weak var jetpackBadgeView: UIView!


Copy link
Contributor

Choose a reason for hiding this comment

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

Micro, optional nite: Not sure if this extra space was intentional.

@twstokes
Copy link
Contributor

👋 @Gio2018, I noticed for very large accessibility sizes the badge will exceed the bounds of the screen. Possibly we could add constraints to prevent this?

@Gio2018 Gio2018 requested a review from twstokes July 13, 2022 16:10
@Gio2018 Gio2018 merged commit a865286 into trunk Jul 13, 2022
@Gio2018 Gio2018 deleted the feature/jetpack-badge-my-site-menu-activity-log branch July 13, 2022 18:45
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