[Reader] Implement announcement card #20846

RenanLukas · 2024-05-18T01:17:06Z

Fixes #20621

Screen.Recording.2024-05-17.at.10.26.02.PM.mov

To Test:

Install JP and log in.
Make sure the reader_announcement_card feature flag is disabled.
Open Reader.
🔍 Verify the announcement card is not shown in any feed.
Enable reader_announcement_card feature flag.
Open Reader.
🔍 Verify the announcement card appears as expected in all the feeds.
🔍 Tap "Done" button: make sure the announcement card disappears and is never shown again (unless the app data is cleared).

Regression Notes

Potential unintended areas of impact
- Reader feeds layouts since I had to change a bit the reader_fragment_layout file
What I did to test those areas of impact (or what existing automated tests I relied on)
- Manual and unit testing.
What automated tests I added (or what prevented me from doing so)
- Updated ReaderViewModelTest.kt.

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.

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

dangermattic · 2024-05-18T01:32:46Z

	3 Warnings
⚠️	`strings.xml` files should only be updated on release branches, when the translations are downloaded by our automation.
⚠️	This PR is larger than 300 lines of changes. Please consider splitting it into smaller PRs for easier and faster reviews.
⚠️	Class `ReaderAnnouncementCardItemData` is missing tests, but `unit-tests-exemption` label was set to ignore this.

Generated by 🚫 Danger

RenanLukas · 2024-05-18T01:46:20Z

Reviewer: I have no idea why SonarCloud Quality Gate is complaining about this line.

wpmobilebot · 2024-05-18T01:53:01Z

📲 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	pr20846-681ab48
	Commit	`681ab48`
	Direct Download	`wordpress-prototype-build-pr20846-681ab48.apk`

Note: Google Login is not supported on these builds.

wpmobilebot · 2024-05-18T01:57:21Z

📲 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	pr20846-681ab48
	Commit	`681ab48`
	Direct Download	`jetpack-prototype-build-pr20846-681ab48.apk`

Note: Google Login is not supported on these builds.

codecov · 2024-05-20T15:36:30Z

Codecov Report

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

Project coverage is 40.83%. Comparing base (8255e5b) to head (681ab48).
Report is 18 commits behind head on feature/tags-ia.

Files	Patch %	Lines
.../java/org/wordpress/android/ui/prefs/AppPrefs.java	0.00%	3 Missing ⚠️
...ss/android/ui/reader/viewmodels/ReaderViewModel.kt	88.46%	3 Missing ⚠️
.../org/wordpress/android/ui/prefs/AppPrefsWrapper.kt	0.00%	2 Missing ⚠️
...ions/insights/usecases/TagsAndCategoriesUseCase.kt	0.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@                 Coverage Diff                 @@
##           feature/tags-ia   #20846      +/-   ##
===================================================
+ Coverage            40.81%   40.83%   +0.02%     
===================================================
  Files                 1493     1494       +1     
  Lines                68736    68771      +35     
  Branches             11346    11349       +3     
===================================================
+ Hits                 28057    28086      +29     
- Misses               38131    38139       +8     
+ Partials              2548     2546       -2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

develric

Hey @RenanLukas 👋 , thanks for the PR 🙇 (also thanks @thomashorta for taking care of the CI issues while Renan was afk 🙇 ).

Code LGTM, I just left a minor np if you want to consider it 🙇

About the card behavior, I'm not sure if this was discussed before (so in case bear with me 🙇) I had 2 thoughts while interacting with it:

By instinct I'd expect it to be part of the stream instead of staying in a fixed position on top while you scroll the feed; OTOH maybe this works in terms of users noticing it 🤔
Should we avoid to show the card in the following scenarios? Of the two scenarios below, what I'm most unsure of is maybe the empty state case (including onboarding "getting started" state).

Empty states, including onboarding

Blogs or Tagged filtered feeds

-------

Wdyt? I mostly wanted to be sure we considered the items I reported 🙇

develric · 2024-05-21T11:14:45Z

WordPress/src/main/java/org/wordpress/android/ui/reader/viewmodels/ReaderViewModel.kt

+    fun onAnnouncementCardDoneClick() {
+        readerTracker.track(AnalyticsTracker.Stat.READER_ANNOUNCEMENT_CARD_DISMISSED)
+        appPrefsWrapper.setShouldShowReaderAnnouncementCard(false)
+        loadAnnouncementCard()


np: maybe I'd rename this something like setAnnouncementCardState, since in the end we use it for that and in this scope here for example we are not actually loading the cards but setting the "hide" state. wdyt?

Good point, that threw me off as well when I worked on the code yesterday. Maybe updateAnnouncementCard would be better.

RenanLukas · 2024-05-21T14:15:54Z

Thanks for the review, @develric

By instinct I'd expect it to be part of the stream instead of staying in a fixed position on top while you scroll the feed; OTOH maybe this works in terms of users noticing it 🤔

I also thought about it while implementing it and it definitely seems to make sense. The problem is that we have at least three places in the reader that we'd have to implement the card: discover feed screen, tags feed screen (which is the new one) and post list screen, which is basically the other feeds.

Since implementing it in at least three places would significantly increase the effort, I thought the gain we'd have wasn't worth it considering is something shown just once until the user taps "Done". Please let me know if you disagree.

Should we avoid to show the card in the following scenarios? Of the two scenarios below, what I'm most unsure of is maybe the empty state case (including onboarding "getting started" state).

If we're willing to spend time implementing it, yes, it should be possible. I'll have a look at the empty states because we might need some specific logic depending on the screen (like for "Discover", for example).

I'm not sure about the filtered tag/blog scenario, though: if we're showing the announcement card when no filter is selected and the UI is exactly the same (vertical post list), why shouldn't we also show it when a filter is selected and also considering the user is one click away from never seeing it again in case they're annoyed by it?

daniloercoli · 2024-05-21T15:45:07Z

I also thought about it while implementing it and it definitely seems to make sense. The problem is that we have at least three places in the reader that we'd have to implement the card: discover feed screen, tags feed screen (which is the new one) and post list screen, which is basically the other feeds.

+1 on injecting the card into the feed. (For the record, Posting Prompts will be added into the feed).

To restrict the effort, I think we can limit it to the Discovery feed, as it's where the vast majority of people are in the Reader.

…tCard

sonarcloud · 2024-05-21T16:53:50Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

develric

As per team slack discussion/alignment, we are merging this PR since it's going into a feature branch and it's still behind an off feature flag. We'll use this as a plan B while exploring having the announcement card injected in the feeds 👍

RenanLukas added 12 commits May 16, 2024 16:08

Add reader announcement card feature config

c2fa592

Add show Reader card announcement methods to AppPrefs

42fa1c8

WIP reader announcement card UI

9643e59

Fix reader announcement card text style

1c3dddc

WIP Reader announcement card UI integration

2b7e362

Fix scrolling of reader announcement card

5c9f9bc

Add real data to reader announcement card

b45ef93

Implement reader announcement card done button

e37d1db

Implement done button analytics event

2c7c46b

Implement unit tests for reader announcement card

b8b3dac

Fix dark theme colors

c80c983

Merge branch 'feature/tags-ia' into issue/20621-tags-feed-onboarding

5efab23

Fix detekt

06f28a7

RenanLukas added Reader Tags IA labels May 18, 2024

RenanLukas requested review from daniloercoli, thomashorta and develric May 18, 2024 01:44

RenanLukas marked this pull request as ready for review May 18, 2024 01:47

RenanLukas added the unit-tests-exemption label May 18, 2024

Remove unused resource

e0ebd70

Thomas Horta added 3 commits May 20, 2024 12:46

Fix BuildConfig constant being true

d9b2bb1

Replace LazyColumn with Column for announcement items

8c20b4b

Add the Tags feed announcement item if it's enabled only

9d1a4b9

develric reviewed May 21, 2024

View reviewed changes

Apply PR suggestion: rename loadAnnouncementCard to updateAnnouncemen…

681ab48

…tCard

develric approved these changes May 21, 2024

View reviewed changes

RenanLukas merged commit 5dde4db into feature/tags-ia May 21, 2024
21 checks passed

RenanLukas deleted the issue/20621-tags-feed-onboarding branch May 21, 2024 21:58

RenanLukas restored the issue/20621-tags-feed-onboarding branch May 22, 2024 03:34

RenanLukas deleted the issue/20621-tags-feed-onboarding branch May 22, 2024 04:21

RenanLukas mentioned this pull request May 22, 2024

[Reader] Implement announcement card improvements #20861

Merged

13 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Reader] Implement announcement card #20846

[Reader] Implement announcement card #20846

RenanLukas commented May 18, 2024 •

edited by thomashorta

Loading

dangermattic commented May 18, 2024 •

edited

Loading

RenanLukas commented May 18, 2024 •

edited

Loading

wpmobilebot commented May 18, 2024 •

edited

Loading

wpmobilebot commented May 18, 2024 •

edited

Loading

codecov bot commented May 20, 2024 •

edited

Loading

develric left a comment

develric May 21, 2024

thomashorta May 21, 2024

RenanLukas commented May 21, 2024 •

edited

Loading

daniloercoli commented May 21, 2024

sonarcloud bot commented May 21, 2024

develric left a comment

[Reader] Implement announcement card #20846

[Reader] Implement announcement card #20846

Conversation

RenanLukas commented May 18, 2024 • edited by thomashorta Loading

To Test:

Regression Notes

PR Submission Checklist:

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

dangermattic commented May 18, 2024 • edited Loading

RenanLukas commented May 18, 2024 • edited Loading

wpmobilebot commented May 18, 2024 • edited Loading

wpmobilebot commented May 18, 2024 • edited Loading

codecov bot commented May 20, 2024 • edited Loading

Codecov Report

develric left a comment

Choose a reason for hiding this comment

develric May 21, 2024

Choose a reason for hiding this comment

thomashorta May 21, 2024

Choose a reason for hiding this comment

RenanLukas commented May 21, 2024 • edited Loading

daniloercoli commented May 21, 2024

sonarcloud bot commented May 21, 2024

Quality Gate passed

develric left a comment

Choose a reason for hiding this comment

RenanLukas commented May 18, 2024 •

edited by thomashorta

Loading

dangermattic commented May 18, 2024 •

edited

Loading

RenanLukas commented May 18, 2024 •

edited

Loading

wpmobilebot commented May 18, 2024 •

edited

Loading

wpmobilebot commented May 18, 2024 •

edited

Loading

codecov bot commented May 20, 2024 •

edited

Loading

RenanLukas commented May 21, 2024 •

edited

Loading