Conversation
…ger like this string.
…ing a dedicated TrainOfFacesUiState.
|
You can trigger optional UI/connected tests for these changes by visiting CircleCI here. |
|
You can test the changes on this Pull Request by downloading the APKs: |
Generated by 🚫 dangerJS |
ashiagr
left a comment
There was a problem hiding this comment.
Great job @develric! I absolutely loved this "train of likes" animation and the current user avatar at the end. It feels so much better from the previous one 🎉 .
Also KUDOS for handling animation direction in RTL language, and taking care of accessibility. Everything worked as described in my testing. I'm approving this PR as nothing is blocking functionality-wise. I just left super minor code-level suggestions, so not merging it. Shouldn't take much of your time🤞.
Another thing, since this is a user-facing change, let's add it to release notes, I'm sure users will love to know about this cute little improvement :).
...ss/src/main/java/org/wordpress/android/ui/reader/viewholders/BloggingLikersTextViewHolder.kt
Outdated
Show resolved
Hide resolved
WordPress/src/main/java/org/wordpress/android/ui/reader/adapters/ReaderPostLikersAdapter.kt
Outdated
Show resolved
Hide resolved
WordPress/src/main/java/org/wordpress/android/ui/engagement/EngagementUtils.kt
Outdated
Show resolved
Hide resolved
WordPress/src/main/java/org/wordpress/android/ui/reader/ReaderPostDetailFragment.kt
Outdated
Show resolved
Hide resolved
WordPress/src/main/java/org/wordpress/android/ui/reader/viewmodels/ReaderPostDetailViewModel.kt
Outdated
Show resolved
Hide resolved
WordPress/src/main/java/org/wordpress/android/ui/reader/viewmodels/ReaderPostDetailViewModel.kt
Outdated
Show resolved
Hide resolved
WordPress/src/main/java/org/wordpress/android/ui/reader/adapters/TrainOfFacesItem.kt
Show resolved
Hide resolved
WordPress/src/test/java/org/wordpress/android/ui/engagement/EngagedPeopleListViewModelTest.kt
Outdated
Show resolved
Hide resolved
|
Thanks for the review @ashiagr, from a quick reading all your suggestions makes sense to me 👍. I ended up running out of time today but will take care on Monday. Have a great we 😎. |
|
Hi @ashiagr 👋 , followed up to your suggestions, should be ready for another pass 🙇 . |
|
Thank you for all the updates @develric! 🙇♀️ Smoke-tested once again, everything looks good. Great work! Merging it now. |
Fixes #15251
Internal Ref:
pctCYC-8w-p2iOS main PR: wordpress-mobile/WordPress-iOS#16936
Due to AFKs and Sabbaticals pinging folks outside of my team, thanks for any support you can give; trying to give a bit more context in the description below 🙇 .
The following is the final result (avatar duplication intended to not show other users avatar)
Screen.Recording.2021-08-26.at.16.19.51.mov
The original implementation (without the animation) had the "n bloggers like this" text outside the recycler; since we need a synched animation it seemed fair to bring the text inside the recycler and leverage the fade in/out recycler built in animation.
Files involved are a few in number but the changes were pretty related and hopefully are not too heavy to review 🤞 (in GH at least for me it rendered better using the split diff view); just adding a bit of grouping in the hope that it helps:
android.ui.readerpackage are the main modification. They:ReaderPostDetailViewModelandEngagedPeopleListViewModelintroducing a dedicated TrainOfFacesUiState + relevant Diff Callback and View Holdersandroid.ui.engagementpackage) are changes that remove unused fields fromEngagedPeopleListUiState+ the "limit and retry" logic that is no more needed given we use pageLength for the limit and manage the like/unlike operation locally without requesting the likers data from the endpointA last note about the "n bloggers like this"; given the
Youis the current user, possible state we expect are:To test
Borrowing below the test steps from the iOS linked PR:
Smoke test likers list by tapping on the train of faces and accessing from Notifications > Likes
Regression Notes
Potential unintended areas of impact
List of likes in ReaderPostDetails and Notifications > Likes
What I did to test those areas of impact (or what existing automated tests I relied on)
Manual testing + already existing unit testing
What automated tests I added (or what prevented me from doing so)
Added a few unit tests where applicable
PR submission checklist:
RELEASE-NOTES.txtif necessary.