jetpack icon indicating copy to clipboard operation
jetpack copied to clipboard

[RNMobile] Update Get Apps card to reflect the Jetpack app

Open SiobhyB opened this issue 3 years ago • 5 comments

Changes proposed in this Pull Request:

The following changes have been made to the "get apps" section found via JetpackDashboard:

  • The section is no longer dismissable.
  • A QR code is available for users to scan when viewed on desktop, with buttons on mobile.
  • Some updates have been made to the design, including tweaks to padding, the new app icon, etc.

Other information:

  • [ ] Have you written new tests for your changes, if applicable?
  • [x] Have you checked the E2E test CI results, and verified that your changes do not break them?

Jetpack product discussion

pcdRpT-F0-p2

Does this pull request change what data or activity we track or use?

Yes, downloads from the app buttons may decrease, due to the fact that they'll only available from mobile devices following this change.

Testing instructions:

  • With this branch checked out, navigate to JetpackDashboard.
  • Scroll down to the mobile app card and verify that the design has been updated.

Screenshots:

Design comparison (web) ⤵️

Note, the design in Figma used the SF Pro font. I didn't specify that in the design to keep consistency with the default font used in other sections on the page, but am happy to change if required. I aimed to match all other sizes/dimensions provided in the Figma.

Design:

Screenshot 2022-09-20 at 16 49 20

Actual:

Screenshot 2022-09-20 at 17 07 18
Before/after (web) ⤵️

These before/after screenshots are taken in Google Chrome (105.0.5195.125) on macOS (12.6).

Before:

Screenshot 2022-09-19 at 13 40 50

After:

Screenshot 2022-09-20 at 17 07 18
Mobile web ⤵️

It's worth noting that these screenshots were taken in emulators, rather than from actual devices.

Google Pixel 5 iPhone XR
x-browser testing ⤵️

The main browser I used for development was Chrome, for which screenshots have already been provided above. In addition, I tested this change across the latest versions of Opera, Firefox, Edge, and Safari on macOS (12.6).

Opera Firefox
Safari Edge

SiobhyB avatar Sep 19 '22 10:09 SiobhyB

Are you an Automattician? You can now test your Pull Request on WordPress.com. On your sandbox, run bin/jetpack-downloader test jetpack rnmobile/update-get-apps-section to get started. More details: p9dueE-5Nn-p2

github-actions[bot] avatar Sep 19 '22 10:09 github-actions[bot]

Thank you for your PR!

When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:

  • :white_check_mark: Include a description of your PR changes.
  • :white_check_mark: All commits were linted before commit.
  • :white_check_mark: Add a "[Status]" label (In Progress, Needs Team Review, ...).
  • :white_check_mark: Add testing instructions.
  • :white_check_mark: Specify whether this PR includes any changes to data or privacy.
  • :white_check_mark: Add changelog entries to affected projects

This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation :robot:


The e2e test report can be found here. Please note that it can take a few minutes after the e2e tests checks are complete for the report to be available.


Once your PR is ready for review, check one last time that all required checks (other than "Required review") appearing at the bottom of this PR are passing or skipped. Then, add the "[Status] Needs Team review" label and ask someone from your team review the code. Once you’ve done so, switch to the "[Status] Needs Review" label; someone from Jetpack Crew will then review this PR and merge it to be included in the next Jetpack release.


Jetpack plugin:

  • Next scheduled release: October 4, 2022.
  • Scheduled code freeze: September 27, 2022.

github-actions[bot] avatar Sep 19 '22 10:09 github-actions[bot]

I noticed the QR code is blurry:

CleanShot 2022-09-26 at 15 47 21@2x

Maybe this is an artifact of you screenshots (BTW, thank you for the excellent screenshots!) or perhaps the QR code wasn't exported properly? Actually, it looks like a different QR from the one in the Figma; Is that intentional?

Here's an SVG if it helps:

jp-com-app

shaunandrews avatar Sep 26 '22 19:09 shaunandrews

@shaunandrews, thank you for checking! The code was blurry as I was using a custom component to generate it, rather than your .svg. I've gone ahead to update that now, and updated the screenshots also. Let me know how that looks to you and if any further changes are needed.

SiobhyB avatar Sep 27 '22 11:09 SiobhyB

Thank you again for your review @dcalhoun, I believe I've addressed all your points now, including optimizing the SVGs (done in https://github.com/Automattic/jetpack/pull/26276/commits/e80f5e60db205f6859391226e40faaed438df742). I've also made a change in https://github.com/Automattic/jetpack/pull/26276/commits/6d5ccf45f41e44a286f1041d0109e74250a23f25, which addresses an issue with the way I was handling the strings. If the latest version looks good, I'll aim to request a final review from the Jetpack crew tomorrow. 🙇‍♀️

SiobhyB avatar Sep 29 '22 17:09 SiobhyB

@SiobhyB This is looking good and thanks for the screenshots!

Recently we merged a PR that introduces device detection functionality that helps display a correct App badge on mobile web. It's used on the Recommendations tab. Do you think it could be useful in this case as well?

PR: https://github.com/Automattic/jetpack/pull/26093

bindlegirl avatar Sep 30 '22 10:09 bindlegirl

@bindlegirl, love that suggestion! I've gone ahead to implement it in https://github.com/Automattic/jetpack/pull/26276/commits/6e17e6f06344327f7c75fa5e06da997365d2cd34. Let me know how that seems to you:

Pixel 5 iPhone XR

SiobhyB avatar Sep 30 '22 13:09 SiobhyB

cherry-picked to jetpack/branch-11.4 in 3dbedf0e47b24e95247f137de90f525da4d76a98

samiff avatar Oct 03 '22 20:10 samiff