Skip to content

Jetpack focus update rtl animation resources#17039

Merged
ravishanker merged 5 commits intotrunkfrom
Jetpack-focus-Update-RTL-animation-resources
Aug 17, 2022
Merged

Jetpack focus update rtl animation resources#17039
ravishanker merged 5 commits intotrunkfrom
Jetpack-focus-Update-RTL-animation-resources

Conversation

@ravishanker
Copy link
Copy Markdown
Contributor

@ravishanker ravishanker commented Aug 16, 2022 •
edited
Loading

This PR updates Lottie animation to cater for RTL locales

Fixes #16978

device-2022-08-16-124001.mp4
device-2022-08-16-124045.mp4

To test:

Setup

  • Build and run
  • Launch WordPress app
  • Go to App Settings (Tap on Avatar at the top right hand corner on My Site -> Find App Settings)
  • Select Debug Settings
  • Find JetpackPoweredBottomFeatureConfig under Features in development Re-launch app (scroll down if necessary)

Test
Find and tap on Jetpack powered badge or banner on any of the following screens

Badge

  1. Home
  2. Activity Log detail
  3. Reader detail
  4. App Settings
  5. Notifications Settings
  6. Me

Banner

  1. Stats
  2. Notifications
  3. Reader
  4. Reader Search
  5. Activity Log
  6. Sharing

Ensure bottom sheet pops up

  • Test in LTR languages like English. Ensure animation work like in the first video above
  • Test in RTL languages like Arabic or Hebrew. Ensure animation works like in the second video above

Regression Notes

  1. Potential unintended areas of impact
    None

  2. What I did to test those areas of impact (or what existing automated tests I relied on)
    Tested different orientations

  3. What automated tests I added (or what prevented me from doing so)
    Existing unit tests

PR submission checklist:

  • I have completed the Regression Notes.
  • 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.

@ravishanker ravishanker requested a review from AjeshRPai August 16, 2022 03:46
@ravishanker ravishanker self-assigned this Aug 16, 2022
@ravishanker ravishanker added this to the 20.6 milestone Aug 16, 2022
@wpmobilebot
Copy link
Copy Markdown
Contributor

wpmobilebot commented Aug 16, 2022 •
edited
Loading

You can test the WordPress changes on this Pull Request by downloading an installable build (wordpress-installable-build-pr17039-c140292.apk), or scanning this QR code:

@wpmobilebot
Copy link
Copy Markdown
Contributor

wpmobilebot commented Aug 16, 2022 •
edited
Loading

You can test the Jetpack changes on this Pull Request by downloading an installable build (jetpack-installable-build-pr17039-c140292.apk), or scanning this QR code:

@ravishanker ravishanker requested review from mkevins and removed request for AjeshRPai August 16, 2022 04:31
mkevins
mkevins approved these changes Aug 16, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@mkevins mkevins left a comment

Choose a reason for hiding this comment

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

Nice work Ravi 👍 . Code changes look good. Also, I tested these flows with both LTR and RTL, and the animations are working as expected.

One thing I am wondering: I didn't see a difference between the animations on the various screens. I might be missing something subtle, so please don't consider this a blocker, and feel free to merge as is if needed. Could these all be using the same animation (i.e. only have two total, one for each of LTR and RTL)?

Also, another thought came to mind - could we make use of a new raw-ldrtl directory to make these resource changes happen automatically? Or is there some limitation with Lottie for doing that?

@ravishanker
Update jetpack_powered_bottom_sheet.xml
c140292 
increase font size to 32sp
decreased the top margin to 40dp
@ravishanker
Copy link
Copy Markdown
Contributor Author

ravishanker commented Aug 17, 2022 •
edited
Loading

Nice work Ravi 👍 . Code changes look good. Also, I tested these flows with both LTR and RTL, and the animations are working as expected.

🙇

One thing I am wondering: I didn't see a difference between the animations on the various screens. I might be missing something subtle, so please don't consider this a blocker, and feel free to merge as is if needed. Could these all be using the same animation (i.e. only have two total, one for each of LTR and RTL)?

This bottom sheet has a full screen variation that uses feature specific animation and text which is now suppressed until next sprint.

Also, another thought came to mind - could we make use of a new raw-ldrtl directory to make these resource changes happen automatically? Or is there some limitation with Lottie for doing that?

raw doesn't seem to work with -ldrtl, not sure if it is Lottie issue and also we are going to need to dynamically update the animation for full screen feature specific popu-ups!

@ravishanker ravishanker merged commit 7ff6a00 into trunk Aug 17, 2022
@ravishanker ravishanker deleted the Jetpack-focus-Update-RTL-animation-resources branch August 17, 2022 02:20
@ParaskP7 ParaskP7 mentioned this pull request May 18, 2023
Merged
18 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@mkevins mkevins mkevins approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@ravishanker ravishanker

Labels

[Type] Task

Projects

None yet

Milestone

20.6

Development

Successfully merging this pull request may close these issues.

[Android] Add logo animation to "Jetpack powered" overlay

3 participants

@ravishanker @wpmobilebot @mkevins