Pinned sites - Githubissues

jarvislin commented 1 month ago

See: p1710242775754929-slack-C3EA6SMFH

This is a Hack Week project. We redesign the site picker, add a new feature Edit pinned sites, and deprecate the Show/hide sites feature.

Before	After

To Test:

Choose a site

Sign in JP app
Click on the arrow icon ⌄ at the top-right corner.
It should enter the screen of re-designed site picker (ChooseSiteActivity)
Choose a site
It should back to the main screen with the site you chose

Search

Click on the arrow icon ⌄ at the top-right corner.
Click on the search icon
Type some characters
It should return the matched results
Click on the dismiss icon X at the top-right corner
It should clear your keyword for search
Click on the back arrow icon <-
It should leave the search mode.

Pin sites

Click on the arrow icon ⌄ at the top-right corner.
Click on the Pin icon at the top-right corner.
Each row should display the pin icon.
Click on the pin icon to pin a site
It should move the site you pinned to the Pinned Sites section
Click on the pin icon to unpin the site you just pinned
It should move the site to All Sites section

Reblog

Go to the tab of Reader
Click Reblog on the reader list
It should enter the screen of re-designed site picker (ChooseSiteActivity) if you have multiple sites.
It should hide the pin icon and Add a site button
Choose a site
It should enter the screen of Editor with the post you reblogged.
Back to the reader list
Click on a post to the details view
Click Reblog button in the details view
It should enter the screen of re-designed site picker (ChooseSiteActivity) if you have multiple sites.
It should hide the pin icon and Add a site button
Choose a site
It should enter the screen of Editor with the post you reblogged.

Regression Notes

Potential unintended areas of impact
- Reader (reblogging-related)
- Auth failure (caused by the invalid token with a self-hosted site or a jetpack-connected site)
- Site picker
What I did to test those areas of impact (or what existing automated tests I relied on)
- Manual
What automated tests I added (or what prevented me from doing so)
- I add some unit tests for the view model

PR Submission Checklist:

[x] I have completed the Regression Notes.
[x] I have considered adding accessibility improvements for my changes.
[x] I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

Testing Checklist (strike-out the not-applying and unnecessary ones):

[x] WordPress.com sites and self-hosted Jetpack sites.
[x] Portrait and landscape orientations.
[x] Light and dark modes.
[ ] Fonts: Larger, smaller and bold text.
[ ] High contrast.
[ ] Talkback.
[ ] Languages with large words or with letters/accents not frequently used in English.
[ ] Right-to-left languages. (Even if translation isn’t complete, formatting should still respect the right-to-left layout)
[ ] Large and small screen sizes. (Tablet and smaller phones)
[ ] Multi-tasking: Split screen and Pop-up view. (Android 10 or higher)

dangermattic commented 1 month ago

	4 Warnings
:warning:	`strings.xml` files should only be updated on release branches, when the translations are downloaded by our automation.
:warning:	This PR is larger than 300 lines of changes. Please consider splitting it into smaller PRs for easier and faster reviews.
:warning:	Class `ActionMode` is missing tests, but `unit-tests-exemption` label was set to ignore this.
:warning:	This PR is assigned to the milestone 24.7. This milestone is due in less than 4 days. Please make sure to get it merged by then or assign it to a milestone with a later deadline.

Generated by :no_entry_sign: Danger

wpmobilebot commented 1 month ago

📲 You can test the changes from this Pull Request in WordPress by scanning the QR code below to install the corresponding build.

	App Name	WordPress
	Flavor	Jalapeno
	Build Type	Debug
	Version	pr20521-6b252f9
	Commit	6b252f97504b0e992d5ae1127d639559846093fa
	Direct Download	`wordpress-prototype-build-pr20521-6b252f9.apk`

Note: Google Login is not supported on these builds.

wpmobilebot commented 1 month ago

📲 You can test the changes from this Pull Request in Jetpack by scanning the QR code below to install the corresponding build.

	App Name	Jetpack
	Flavor	Jalapeno
	Build Type	Debug
	Version	pr20521-6b252f9
	Commit	6b252f97504b0e992d5ae1127d639559846093fa
	Direct Download	`jetpack-prototype-build-pr20521-6b252f9.apk`

Note: Google Login is not supported on these builds.

codecov[bot] commented 1 month ago

Codecov Report

Attention: Patch coverage is 40.86957% with 68 lines in your changes are missing coverage. Please review.

Project coverage is 40.32%. Comparing base (90a8f60) to head (e52f0b8).

:exclamation: Current head e52f0b8 differs from pull request most recent head cd859c3. Consider uploading reports for the commit cd859c3 to get more accurate results

Files	Patch %	Lines
.../wordpress/android/ui/main/utils/SiteRecordUtil.kt	0.00%	25 Missing :warning:
...va/org/wordpress/android/ui/main/AddSiteHandler.kt	0.00%	9 Missing :warning:
...org/wordpress/android/ui/main/ChooseSiteAdapter.kt	0.00%	7 Missing :warning:
...n/java/org/wordpress/android/ui/main/SiteRecord.kt	66.66%	6 Missing and 1 partial :warning:
...ava/org/wordpress/android/ui/main/SiteViewModel.kt	82.85%	5 Missing and 1 partial :warning:
...ava/org/wordpress/android/ui/ActivityLauncher.java	0.00%	4 Missing :warning:
.../org/wordpress/android/ui/prefs/AppPrefsWrapper.kt	0.00%	4 Missing :warning:
...rg/wordpress/android/ui/main/SitePickerContract.kt	0.00%	3 Missing :warning:
.../java/org/wordpress/android/ui/prefs/AppPrefs.java	0.00%	3 Missing :warning:

Additional details and impacted files

```diff @@ Coverage Diff @@ ## trunk #20521 +/- ## ========================================== - Coverage 40.50% 40.32% -0.18% ========================================== Files 1474 1478 +4 Lines 67967 67944 -23 Branches 11235 11239 +4 ========================================== - Hits 27529 27398 -131 - Misses 37955 38082 +127 + Partials 2483 2464 -19 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

osullivanchris commented 1 month ago

Current site is not at the top. With the old implementation the current site was always separated at the top but now you may have to scroll to find it in bold. I don't think this is necessarily an issue since it does not affect the functionality but wanted to highlight it since it surprised me at first thinking that the current selection is missing. Looping in @osullivanchris for any design feedback 🙇

Thanks @antonis . We've been continuing to discuss this. We'll have 'all sites' ordered by most recent rather than alphabetical. So you're currently selected site should be near the top either as most recent or pinned.

jarvislin commented 1 month ago

@antonis Thanks for reviewing this PR and reporting those issues, I'm gonna correct them and let you know once those issues have been fixed.

jarvislin commented 3 weeks ago

Hi @antonis The issues you reported have been resolved. Could you check again? Thanks! @osullivanchris Please take a look at the new changes. Let me know if your found anything needs to be corrected.

antonis commented 3 weeks ago

I'd also like to share some notes/enhancement ideas that can be considered on follow up PRs:

It would be nice to add analytics events for the new functionality. This way we could measure the usage and improve as needed in the future. This would also help us to make a comparison with the deprecated show/hide feature that was tracked with the site_switcher_toggled_edit_tapped and the site_switcher_toggle_blog_visible which are not used anymore. The old code tracked the following:
```
site_switcher_toggled_edit_tapped, Properties: {"state":"edit"}
site_switcher_toggle_blog_visible, Properties: {"blog_id":"xxxx","visible":"0"}
site_switcher_toggle_blog_visible, Properties: {"blog_id":"xxxx","visible":"0"}
site_switcher_toggled_edit_tapped, Properties: {"state":"done"}
```
The site_switcher_displayed event is not emitted in the new code when the site switcher opens. Only the my_site_site_switcher_tapped. Thought we were getting two event for the same thing I think adding back the site_switcher_displayed would help on having consistency in the events history.
I noticed that the old "show/hide" functionality persisted the sites after a log out. It would be nice to have the same for the pinned site so that the users won't have to reorganise their sites after a logout on the same device.

jarvislin commented 3 weeks ago

@antonis Thank you so much for the helpful review/reply.

The idea of analytics really makes sense to me. I'll add/update events.

I noticed that the old "show/hide" functionality persisted the sites after a log out. It would be nice to have the same for the pinned site so that the users won't have to reorganise their sites after a logout on the same device.

"Show/Hide" functionality is also implemented on backend, we fetch the visibility of each site from API so it can restore the state after logging out. Currently, "pin sites" is a client-only feature, I think there's still the possibility of making the sites be restored, but it'd be better if we can store the info via API.

jarvislin commented 3 weeks ago

Hi @wordpress-mobile/mobile-ui-testing-squad, I adjusted the UI test because the UI has been redesigned. Could you take a look at the tests? Thanks