[Reader IA] Integrate Top Bar Filter UI to Filter functionality #19856

thomashorta · 2023-12-28T22:55:15Z

Fixes #19855
Fixes #19851

Caution

Should not be merged before #19844

This integrates the new Filter component with the existing Filter functionality, while also updating the Filter Sheet to look a bit closer to the new specs, but some things, like the Edit Button and the style update for site items will be done in future PRs.

As a side effect, this PR also fixes #19851 since the root cause of that bug also caused the feed to refresh when selecting a Filter, which was needed for this PR.

19855-reader-ia-filter-integration.mp4

To Test:

Install and log into Jetpack
Go to the Reader
Go to Feeds that are "filterable", such as "Subscriptions" and "Automattic" (for A12s)
Verify the appropriate filter chips appear, with the correct count inside them
Tap one of the filters
Verify the correct bottom sheet opens (for blogs or tags)
Select a filter item in the list
Verify the correct blog/tag title is shown when selected
Tap the filtered title (NOT the X button)
Verify the bottom sheet opens for the same type of item (blog/tag)
Cancel selection or select something else
Tap the X button in the Filter chip for the selected item
Verify clearing the filter works properly

Regression Notes

Potential unintended areas of impact
- Fetching posts / Loading functionality inside the feeds
What I did to test those areas of impact (or what existing automated tests I relied on)
- Manual Testing
What automated tests I added (or what prevented me from doing so)
- TODO

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.

UI Changes Testing Checklist:

Portrait and landscape orientations.
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)

peril-wordpress-mobile · 2023-12-28T22:55:24Z

	Warnings
⚠️	PR is not assigned to a milestone.
⚠️	PR has more than 300 lines of code changing. Consider splitting into smaller PRs if possible.

Generated by 🚫 dangerJS

wpmobilebot · 2023-12-28T23:13:58Z

📲 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	pr19856-e37902e
	Commit	`e37902e`
	Direct Download	`wordpress-prototype-build-pr19856-e37902e.apk`

Note: Google Login is not supported on these builds.

wpmobilebot · 2023-12-28T23:14:01Z

📲 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	pr19856-e37902e
	Commit	`e37902e`
	Direct Download	`jetpack-prototype-build-pr19856-e37902e.apk`

Note: Google Login is not supported on these builds.

develric

Thanks for this PR @thomashorta 🙇 ! Overall it looks good. I'm reporting an issue I found in one of my test accounts though. While comparing the viewpager tabs and feeds content for before / after, I see a missing menu item for "Followed P2S" (that is a filterable with only Sites and no Tags elements, I think similarly to the Automattic item)

Not sure how common this scenario is, but I think that menu is related to P2 the product sites that I think I created in that account from here if it's useful to create the test scenario.

cc @RenanLukas fyi.

thomashorta · 2024-01-08T12:58:04Z

I'm reporting an issue I found in one of my test accounts though. While comparing the viewpager tabs and feeds content for before / after, I see a missing menu item for "Followed P2S" (that is a filterable with only Sites and no Tags elements, I think similarly to the Automattic item)

I don't think I understand what is the issue you're reporting here. In your comment, I only see a screenshot from the old UI (before) and there doesn't seem to be a report of what is the issue after the changes.

develric · 2024-01-08T13:55:53Z

Hey @thomashorta 👋 , it seems I missed to paste a picture of how it looks for me with the version on this PR, sorry if it was not clear from the description and here is a before/after 👍

While comparing the viewpager tabs and feeds content for before / after, I see a missing menu item for "Followed P2S" (that is a filterable with only Sites and no Tags elements, I think similarly to the Automattic item)

What I was trying to say here is (using the same user for the before/after):

the "Followed P2s" item is missing in the after
If it's helpful, note that the "Followed P2s" filtering had only Sites (and not Tags) and I'd expect this to be similar to the behavior of the Automattic menu (that I verified shows up correctly in the after as well)

About how to get the Followed P2s on your side, what follows is my best guess

I think that menu is related to P2 the product sites that I think I created in that account from here if it's useful to create the test scenario.

but I can share my test account with you if it's quicker (just dm me in slack 👍 )

thomashorta · 2024-01-08T15:03:35Z

Ah, I understand it now, though I don't think it has anything to do with this PR since this is integrating the filter functionality (blogs and tags) with the real data from each field, and not related to the feeds and drop-down menu itself.

Regarding this:

If it's helpful, note that the "Followed P2s" filtering had only Sites (and not Tags) and I'd expect this to be similar to the behavior of the Automattic menu (that I verified shows up correctly in the after as well)

The code in this PR already makes sure that we only show the "Blogs" pill for Automattic and P2 feeds, since I added the same logic used by the subscription management sheet to decide if the tag filter pill should be shown (https://github.com/wordpress-mobile/WordPress-Android/pull/19856/files#diff-2e098785089098d2a88d269ff91d4fb30d8f03c6c0fa67defdd892da6551e063R510-R511).

I think it makes sense to open a different issue for the "Followed P2" feed not showing on the drop-down menu, wdyt?

thomashorta · 2024-01-08T21:16:37Z

@develric I went ahead and created an issue for what you reported since it's not related to the code in this PR (#19898).

…der Following tab" This reverts commit 530bb5c. This revert is needed since now we do rely on information from the Following blogs on the Following feed due to the new filters UI on the Top Bar.

RenanLukas · 2024-01-09T20:34:24Z

Thanks for the PR, @thomashorta . I went through the testing steps and it's working as expected. I'm now moving to the code review.

I wanted to point out that adding this call again did not work for me to fix #19891:

Screen.Recording.2024-01-09.at.5.16.09.PM.mov

I've tried building locally and also with the APK provided by the wpmobilebot.

RenanLukas · 2024-01-09T20:50:09Z

As discussed in DM, there was another bug that was fixed that prevented the subscription blogs from being fetched (fixed here). #19891 will remain open so we can work on it when it's possible as a UX improvement to show something other than "0 Blogs" when there's a fresh install.

RenanLukas

@thomashorta code LGTM 👍

I just didn't approve yet to confirm if the strings with new content and same keys are a problem or not.

RenanLukas · 2024-01-09T21:52:41Z

WordPress/src/main/res/values/strings.xml

-    <string name="reader_filter_sites_title">Sites</string>
-    <string name="reader_filter_tags_title">Topics</string>
-    <string name="reader_filter_main_title">Following</string>
+    <string name="reader_filter_sites_title">Filter by blog</string>


I don't remember by heart: do we have to rename strings when the content changes to they're correctly picked up by GlotPress script?

If I remember correctly, the script (or maybe GlotPress) is smart enough to see the changes and mark the Strings as "untranslated" in the platform. cc @oguzkocer or @AliSoftware to confirm that though.

We should always treat strings as immutable—once you created a key with a value, you should avoid changing its value later on, especially if the first value already made it to a previous release.

This is because otherwise once this change hits trunk it will be sent to GlotPress and the new copy retranslated… but if we then pull the translations from GlotPress into the release/* branch before submitting to Google then that change of the copy would end up making its way to the release branch… despite being a change tied to something in trunk.

In summary modifying the value of an existing key will cause a discrepancy in the release/* branch, between the codebase (which will expect the old copy as that's what it was when code was frozen) and the translations (updated with the new copy when we'll pull the latest from GlotPress)

TL;DR: prefer using a brand new key.

Thanks, @AliSoftware

I'll change the string keys with new content to brand new ones.

Thanks @AliSoftware for the answer, that makes sense!

RenanLukas · 2024-01-09T21:52:51Z

WordPress/src/main/res/values/strings.xml

-    <string name="reader_filter_tags_title">Topics</string>
-    <string name="reader_filter_main_title">Following</string>
+    <string name="reader_filter_sites_title">Filter by blog</string>
+    <string name="reader_filter_tags_title">Filter by tag</string>


Same question about keeping the same string name when content changes.

.../src/main/java/org/wordpress/android/ui/reader/views/compose/filter/ReaderFilterChipGroup.kt

…tegration

dangermattic · 2024-01-10T15:52:19Z

	2 Warnings
⚠️	This PR is larger than 300 lines of changes. Please consider splitting it into smaller PRs for easier and faster reviews.
⚠️	PR is not assigned to a milestone.

Generated by 🚫 Danger

RenanLukas

LGTM

Thomas Horta added 18 commits December 28, 2023 14:43

Initial changes to open the filter bottom sheet

cf47cc1

Don't reset topbar / selection state if list doesn't change

e31adb2

Use the SubFilterViewModel directly from ReaderFragment

55ac1e0

Select and clear filters

23eb426

Make clicking on current filter open the filter list

51b6989

Add small delay on filter update for fluid animation

412c56f

Pass isFilterableScreen via argument to Feed

dc2ebbf

Show filter chip group based on isFilterable

a327835

Rename ReaderViewModel function

cbd839b

Add the ability to show only Blogs or Tags filter

84fc535

Track when filter is cleared

4d251df

Show the Filter Group with correct count for filterable tabs

dbba180

Fix filter count not updating when it was the only state change

1f03e78

Remove old subfilter component

4a15d15

Update bottomsheet UI title and hide pager tabs

798d5d8

Update TODOs

8daf3c1

Update failing existing unit tests

8bb04ac

Fix detekt issues

1387ee2

thomashorta added Reader [Type] Task Reader IA labels Dec 28, 2023

thomashorta requested review from RenanLukas and develric December 28, 2023 22:55

thomashorta self-assigned this Dec 28, 2023

thomashorta changed the base branch from trunk to issue/19605-reader-ia-update-selected-content-stream-based-on-menu-item December 28, 2023 22:59

Remove unused values (fix lint issues)

f31a7ae

thomashorta mentioned this pull request Dec 29, 2023

[Reader IA - Bug] Pull-to-refresh opens Discover Feed #19851

Closed

thomashorta mentioned this pull request Dec 29, 2023

[Reader IA] Open subscription management from Filter sheet #19860

Merged

12 tasks

thomashorta added the [Status] Not Ready for Merge label Dec 29, 2023

Base automatically changed from issue/19605-reader-ia-update-selected-content-stream-based-on-menu-item to feature/reader-ia January 2, 2024 22:46

develric reviewed Jan 3, 2024

View reviewed changes

thomashorta mentioned this pull request Jan 8, 2024

[Reader IA - Bug] Followed P2s feed not showing in drop-down #19898

Closed

wordpress-mobile deleted a comment from wpmobilebot Jan 9, 2024

Revert "Remove unnecessary requests to following/mine endpoint on Rea…

bc19aab

…der Following tab" This reverts commit 530bb5c. This revert is needed since now we do rely on information from the Following blogs on the Following feed due to the new filters UI on the Top Bar.

thomashorta removed the [Status] Not Ready for Merge label Jan 9, 2024

fix broken unit tests

786ca15

RenanLukas reviewed Jan 9, 2024

View reviewed changes

Thomas Horta and others added 5 commits January 9, 2024 19:19

Change boolean variables prefix (code review comments)

14e8b62

Rename Reader strings that changed content

67adf6b

Remove ununsed string reader_filter_tags_title

b8962b6

Remove ununsed string reader_filter_sites_title

23f3f8c

Merge branch 'feature/reader-ia' into issue/19855-reader-ia-filter-in…

e37902e

…tegration

RenanLukas approved these changes Jan 10, 2024

View reviewed changes

RenanLukas merged commit 9ee4b4f into feature/reader-ia Jan 10, 2024
19 checks passed

RenanLukas deleted the issue/19855-reader-ia-filter-integration branch January 10, 2024 17:00

thomashorta mentioned this pull request Jan 18, 2024

fix: Properly space Day One name #19977

Merged

12 tasks

thomashorta mentioned this pull request Feb 3, 2024

[Reader] Improve content navigation and filtering #19978

Merged

12 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Reader IA] Integrate Top Bar Filter UI to Filter functionality #19856

[Reader IA] Integrate Top Bar Filter UI to Filter functionality #19856

thomashorta commented Dec 28, 2023 •

edited

Loading

peril-wordpress-mobile bot commented Dec 28, 2023 •

edited

Loading

wpmobilebot commented Dec 28, 2023 •

edited

Loading

wpmobilebot commented Dec 28, 2023 •

edited

Loading

develric left a comment

thomashorta commented Jan 8, 2024

develric commented Jan 8, 2024

thomashorta commented Jan 8, 2024

thomashorta commented Jan 8, 2024

RenanLukas commented Jan 9, 2024

RenanLukas commented Jan 9, 2024

RenanLukas left a comment

RenanLukas Jan 9, 2024

thomashorta Jan 9, 2024

AliSoftware Jan 9, 2024

RenanLukas Jan 10, 2024

thomashorta Jan 11, 2024

RenanLukas Jan 9, 2024

dangermattic commented Jan 10, 2024

RenanLukas left a comment

[Reader IA] Integrate Top Bar Filter UI to Filter functionality #19856

[Reader IA] Integrate Top Bar Filter UI to Filter functionality #19856

Conversation

thomashorta commented Dec 28, 2023 • edited Loading

To Test:

Regression Notes

PR Submission Checklist:

UI Changes Testing Checklist:

peril-wordpress-mobile bot commented Dec 28, 2023 • edited Loading

wpmobilebot commented Dec 28, 2023 • edited Loading

wpmobilebot commented Dec 28, 2023 • edited Loading

develric left a comment

Choose a reason for hiding this comment

thomashorta commented Jan 8, 2024

develric commented Jan 8, 2024

thomashorta commented Jan 8, 2024

thomashorta commented Jan 8, 2024

RenanLukas commented Jan 9, 2024

RenanLukas commented Jan 9, 2024

RenanLukas left a comment

Choose a reason for hiding this comment

RenanLukas Jan 9, 2024

Choose a reason for hiding this comment

thomashorta Jan 9, 2024

Choose a reason for hiding this comment

AliSoftware Jan 9, 2024

Choose a reason for hiding this comment

RenanLukas Jan 10, 2024

Choose a reason for hiding this comment

thomashorta Jan 11, 2024

Choose a reason for hiding this comment

RenanLukas Jan 9, 2024

Choose a reason for hiding this comment

dangermattic commented Jan 10, 2024

RenanLukas left a comment

Choose a reason for hiding this comment

thomashorta commented Dec 28, 2023 •

edited

Loading

peril-wordpress-mobile bot commented Dec 28, 2023 •

edited

Loading

wpmobilebot commented Dec 28, 2023 •

edited

Loading

wpmobilebot commented Dec 28, 2023 •

edited

Loading