Reader: Add announcement card #23197

dvdchr · 2024-05-13T19:44:12Z

Adds the announcement card to the Reader stream. The card is only shown when:

There's no other header being displayed. The announcement card should have the lowest display priority.
There are no active filters.
There are contents to display. It shouldn't be displayed when the no results or error view is displayed.
The announcement card hasn't been dismissed yet.
Lastly, of course, the remote feature flag reader_announcement_card needs to be enabled.

Preview:

Light	Dark

To test

Tip

To avoid dismissing the card and never seeing it again, you could temporarily disable the dismiss logic by commenting out the line in ReaderStreamViewController+Helper.swift:L179:

    self?.readerAnnouncementCoordinator.isDismissed = false

This way, the announcement card will reappear on the next stream after you dismiss it.

Make sure the readerAnnouncementCard flag is enabled.
Launch the Jetpack app.
Go to the Reader tab.
🔎 Verify that the announcement card is displayed in the Discover stream.
Navigate to other (non-empty) streams.
🔎 Verify that the announcement card is visible.
Navigate to an empty stream.
🔎 Verify that the announcement card is NOT visible.
Go to Subscriptions or Your Tags and filter the stream.
🔎 Verify that the announcement card is NOT visible.

Offline case

Turn on airplane mode and relaunch the Jetpack app.
Go to the Reader stream > Discover.
🔎 Verify that the announcement card is NOT visible.

Spam test

Context

During development, I stumbled on some crashes while switching to Discover, and it crashed with this message:

Terminating app due to uncaught exception 'NSInternalInconsistencyException', reason: 'unable to dequeue a cell with identifier ReaderSitesCell - must register a nib or a class for the identifier or connect a prototype cell in a storyboard'

WordPress-iOS/WordPress/Classes/ViewRelated/Reader/ReaderCardsStreamViewController.swift

Line 93 in c055c21

    
           let cell = tableView.dequeueReusableCell(withIdentifier: readerCardSitesIdentifier) as! ReaderSitesCardCell

However, after a while, it seems like the crash stopped happening... I'm guessing it may be related to this commit: 45d58e4 since the refresh method calls layoutIfNeeded on the table view, and perhaps there might be a retain cycle somewhere.

Ensure that the connectivity is enabled.
Go to the Reader stream > Discover.
Quickly switch to different streams.
- Note: you could also try activating the Network Link Conditioner to switch streams while the request is in progress.
🔎 Ensure that the app does not crash.

Regression Notes

Potential unintended areas of impact
Should be none. The card is hidden behind a flag.
What I did to test those areas of impact (or what existing automated tests I relied on)
Manually tested the changes.
What automated tests I added (or what prevented me from doing so)
N/A.

PR submission checklist:

I have completed the Regression Notes.
I have considered adding unit tests for my changes.
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:

I've noticed that `configureStreamHeader()` had too many layout constraints juggling, so this simplifies it a bit. * `ReaderSiteHeaderView` now inherits from `UITableViewHeaderFooterView`, so that it has the same readableContentGuide of table views. This removes the need to put the SwiftUI view in a container view and align the horizontal anchors manually to the table view. * Moved the border-adding logic to each own views.

This is a workaround that avoids unsatisfiable constraint warnings in the console. By reducing the constraint priority to 999, this allows the the layout engine to prioritize the constraints from `UIView-Encapsulated-Layout-Width` (or height).

The `configureStreamHeader()` method is currently only called when the `readerTopic` is not nil. This needs to be modified because there are streams without a topic (Saved, Tags).

…tate

wpmobilebot · 2024-05-13T19:53:04Z

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

	App Name	Jetpack Alpha
	Configuration	Release-Alpha
	Build Number	pr23197-be6ec3d
	Version	24.8
	Bundle ID	com.jetpack.alpha
	Commit	`be6ec3d`
	App Center Build	jetpack-installable-builds #8941

Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

wargcm

One minor thing, the icons are accessible and it reads the image name:

LGTM!

wargcm · 2024-05-13T23:42:40Z

WordPress/Classes/ViewRelated/Reader/ReaderAnnouncementHeaderView.swift

Looks like this file needs to be added to the WordPress target.

Good catch. I've added it in e2a312a.

dvdchr · 2024-05-14T11:00:19Z

One minor thing, the icons are accessible and it reads the image name:

Good catch! I've manually excluded the image views in be6ec3d.

Thanks for the review! 👍

wpmobilebot · 2024-05-14T11:04:12Z

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

	App Name	WordPress Alpha
	Configuration	Release-Alpha
	Build Number	pr23197-be6ec3d
	Version	24.8
	Bundle ID	org.wordpress.alpha
	Commit	`be6ec3d`
	App Center Build	WPiOS - One-Offs #9892

Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

dvdchr added 11 commits May 11, 2024 14:07

Minor refactor to prep for the announcement header insertion

f978026

The `configureStreamHeader()` method is currently only called when the `readerTopic` is not nil. This needs to be modified because there are streams without a topic (Saved, Tags).

Implement announcement card UI

d03b4ad

Localize strings, refactor view generation

133a742

Hook Done button implementation and animate dismissal

daa8358

Tweak announcement card display logic for the Discover stream

45d58e4

Add remote feature flag and convenience object for checking feature s…

437b4f0

…tate

Implement dismiss flag functionality

660f3f2

Update announcement card background color

55102ff

Don't show announcement card when content is filtered

0b55919

dvdchr added [Type] Enhancement Reader labels May 13, 2024

dvdchr added this to the 25.0 milestone May 13, 2024

dvdchr requested a review from wargcm May 13, 2024 19:44

dvdchr self-assigned this May 13, 2024

wargcm approved these changes May 14, 2024

View reviewed changes

dvdchr added 2 commits May 14, 2024 17:50

Add ReaderAnnouncementHeaderView to WordPress target

e2a312a

Remove image from a11y hierarchy

be6ec3d

dvdchr enabled auto-merge May 14, 2024 11:01

dvdchr merged commit 5c64b83 into trunk May 14, 2024
21 of 22 checks passed

dvdchr deleted the issue/23069-reader-whats-new branch May 14, 2024 11:17

dvdchr mentioned this pull request May 14, 2024

Reader: Tags iA tasks #23069

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reader: Add announcement card #23197

Reader: Add announcement card #23197

dvdchr commented May 13, 2024

wpmobilebot commented May 13, 2024 •

edited

Loading

wargcm left a comment

wargcm May 13, 2024

dvdchr May 14, 2024

dvdchr commented May 14, 2024

wpmobilebot commented May 14, 2024

Reader: Add announcement card #23197

Reader: Add announcement card #23197

Conversation

dvdchr commented May 13, 2024

To test

Offline case

Spam test

Regression Notes

wpmobilebot commented May 13, 2024 • edited Loading

wargcm left a comment

Choose a reason for hiding this comment

wargcm May 13, 2024

Choose a reason for hiding this comment

dvdchr May 14, 2024

Choose a reason for hiding this comment

dvdchr commented May 14, 2024

wpmobilebot commented May 14, 2024

wpmobilebot commented May 13, 2024 •

edited

Loading