Hide empty content in favor of Reader onboarding. #96988

allilevine · 2024-12-02T20:19:08Z

Proposed Changes

Change the Scrolling Feed view to show Reader Onboarding when there are no subscriptions (like the Full Post view does now).

Before	After

Why are these changes being made?

Clean up the empty content state on /read.

Testing Instructions

Remove all of your subscriptions.
Go to /read?flags=reader/recent-feed-overhaul
Check that you see onboarding and not the "Welcome to Reader" content. The view toggle shouldn't be visible, either.
Go to /read without the feature flag and check that you see onboarding and not the "Welcome to Reader" content.
Subscribe to your own site(s). Check that you still see Reader Onboarding with and without the flag.
Complete onboarding or add a subscription to a site you don't own, and check that you see your feeds and can toggle views with the flag applied.

Pre-merge Checklist

Has the general commit checklist been followed? (PCYsg-hS-p2)
Have you written new tests for your changes?
Have you tested the feature in Simple (P9HQHe-k8-p2), Atomic (P9HQHe-jW-p2), and self-hosted Jetpack sites (PCYsg-g6b-p2)?
Have you checked for TypeScript, React or other console errors?
Have you used memoizing on expensive computations? More info in Memoizing with create-selector and Using memoizing selectors and Our Approach to Data
Have we added the "[Status] String Freeze" label as soon as any new strings were ready for translation (p4TIVU-5Jq-p2)?
- For UI changes, have we tested the change in various languages (for example, ES, PT, FR, or DE)? The length of text and words vary significantly between languages.
For changes affecting Jetpack: Have we added the "[Status] Needs Privacy Updates" label if this pull request changes what data or activity we track or use (p4TIVU-aUh-p2)?

github-actions · 2024-12-02T20:19:21Z

Calypso Live (direct link)

	https://calypso.live?image=registry.a8c.com/calypso/app:build-126345

Jetpack Cloud live (direct link)

	https://calypso.live?image=registry.a8c.com/calypso/app:build-126345&env=jetpack

Automattic for Agencies live (direct link)

	https://calypso.live?image=registry.a8c.com/calypso/app:build-126345&env=a8c-for-agencies

matticbot · 2024-12-02T20:20:15Z

Here is how your PR affects size of JS and CSS bundles shipped to the user's browser:

Sections (~52 bytes added 📈 [gzipped])

name    parsed_size           gzip_size
reader       +252 B  (+0.0%)      +52 B  (+0.0%)

Sections contain code specific for a given set of routes. Is downloaded and parsed only when a particular route is navigated to.

Legend

What is parsed and gzip size?

Parsed Size: Uncompressed size of the JS and CSS files. This much code needs to be parsed and stored in memory.
Gzip Size: Compressed size of the JS and CSS files. This much data needs to be downloaded over network.

Generated by performance advisor bot at iscalypsofastyet.com.

holdercp · 2024-12-02T20:32:21Z

@allilevine I just had a thought: not everyone sees Reader onboarding, correct? I think we only show it to new accounts. So this change could show a "blank" screen if someone doesn't have any subscriptions and is not presented with onboarding.

allilevine · 2024-12-02T21:04:09Z

@allilevine I just had a thought: not everyone sees Reader onboarding, correct? I think we only show it to new accounts. So this change could show a "blank" screen if someone doesn't have any subscriptions and is not presented with onboarding.

@holdercp Thanks for catching that! What about adding the forceShow flag to this view, like on the new view, so that onboarding shows whenever subscriptions are 0?

matticbot · 2024-12-02T23:00:50Z

This PR modifies the release build for the following Calypso Apps:

For info about this notification, see here: PCYsg-OT6-p2

notifications
wpcom-block-editor

To test WordPress.com changes, run install-plugin.sh $pluginSlug update/reader-recent-empty-state on your sandbox.

eoigal · 2024-12-02T23:07:06Z

I noticed a small difference between production and this branch. It looks like if the account has no subscriptions (except the users own site), in production we show the Welcome to Reader content. With this branch it doesn't show the Welcome to Reader content OR the new onboarding. I had to delete the subscription to the users own site too to get it work as expected.

Screen.Recording.2024-12-02.at.23.02.23.mov

allilevine · 2024-12-03T14:36:24Z

I noticed a small difference between production and this branch. It looks like if the account has no subscriptions (except the users own site), in production we show the Welcome to Reader content. With this branch it doesn't show the Welcome to Reader content OR the new onboarding. I had to delete the subscription to the users own site too to get it work as expected.

Screen.Recording.2024-12-02.at.23.02.23.mov

@eoigal Nice catch! I added a check for subscriptions to sites that the user is an owner of. I also added back "Welcome to Reader" for conditions we may have missed. Do you happen to know where we filter out the users' own blogs? I'm wondering if it's just owners, or if they're a member of the site.

…tes.

holdercp · 2024-12-03T15:41:36Z

client/reader/following/main.tsx

+	const isLoading = isLoadingCount || isLoadingSiteSubscriptions;
+
+	const hasNonSelfSubscriptions = useMemo( () => {
+		if ( isLoading ) {


Why is this check in this function? I think having it within the hasNonSelfSubscriptions function muddies the intent of the function, meaning the value really represents hasNonSelfSubscriptionsAndNotLoadingSubscriptions 😂.

Can we move the loading check outside of this function? It looks like we're checking isLoading again on line 50 so maybe this check is redundant anyway.

You're right, it's redundant. I went ahead and removed it. 👍

holdercp · 2024-12-03T15:47:50Z

client/reader/following/main.tsx

+		}
+
+		return subscriptionsCount.blogs > 0;
+	}, [ subscriptionsCount?.blogs, siteSubscriptions, isLoading ] );


Does using the optional chaining here impact useMemo? What if it resolves to undefined? I would suspect it would be fine and would work like we expect and re-run the function if that first item changes from undefined to an array or object.

Yeah if subscriptionsCount is undefined, it would just re-run. I updated to explicitly check the object.

holdercp · 2024-12-03T15:50:36Z

client/reader/recent/index.tsx

-							} }
-						/>
-					) }
+					<DataViews


💯 I think pulling all the subscription logic up into main makes this much easier to reason about.

holdercp · 2024-12-03T16:13:42Z

I'm still seeing reader onboarding even if I have subscriptions:

I did this by manually following sites from discover or search. Based on this change, I would expect onboarding to be hidden in the scroll view.

allilevine · 2024-12-03T16:32:08Z

I'm still seeing reader onboarding even if I have subscriptions:
I did this by manually following sites from discover or search. Based on this change, I would expect onboarding to be hidden in the scroll view.

@holdercp That's expected when you meet the normal conditions for Reader Onboarding: new account that hasn't completed onboarding yet. It's still called here.

When I create a new user I do have some initial subscriptions, so seeing it there might be common (as opposed to a new account with no subscriptions at all).

holdercp

allilevine · 2024-12-03T18:43:25Z

I'm noticing now that completing onboarding in the empty state doesn't refresh the view and show your subscriptions. I can follow up on that.

allilevine self-assigned this Dec 2, 2024

allilevine marked this pull request as ready for review December 2, 2024 20:20

allilevine requested a review from a team as a code owner December 2, 2024 20:20

matticbot added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Dec 2, 2024

holdercp approved these changes Dec 2, 2024

View reviewed changes

allilevine added 5 commits December 3, 2024 09:38

Hide empty content in favor of Reader onboarding.

c714e55

Refactor so that both views show the same ReaderOnboarding component.

864bcce

Add back welcome message.

7c03e46

Remove hasSubscriptions prop, not needed.

8d0bd58

Show Reader Onboarding if the user is only subscribed to their own si…

5d7f75d

…tes.

allilevine force-pushed the update/reader-recent-empty-state branch from 3edb91e to 5d7f75d Compare December 3, 2024 14:42

allilevine requested review from holdercp and eoigal December 3, 2024 14:57

Prevent the flash of onboarding before the streams load.

559555c

holdercp reviewed Dec 3, 2024

View reviewed changes

Remove redundant isLoading check.

4ecd617

holdercp approved these changes Dec 3, 2024

View reviewed changes

allilevine merged commit 514ebfd into trunk Dec 3, 2024
11 checks passed

allilevine deleted the update/reader-recent-empty-state branch December 3, 2024 18:39

github-actions bot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Dec 3, 2024

This was referenced Dec 3, 2024

Reader Recent Feed Overhaul: Fix empty state header padding on mobile screens #97029

Merged

Reader Recent Feed: Refetch subscriptions from empty state #97030

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Hide empty content in favor of Reader onboarding. #96988

Hide empty content in favor of Reader onboarding. #96988

allilevine commented Dec 2, 2024 •

edited

Loading

github-actions bot commented Dec 2, 2024 •

edited by matticbot

Loading

matticbot commented Dec 2, 2024 •

edited

Loading

holdercp commented Dec 2, 2024

allilevine commented Dec 2, 2024

matticbot commented Dec 2, 2024

eoigal commented Dec 2, 2024

allilevine commented Dec 3, 2024

holdercp Dec 3, 2024

allilevine Dec 3, 2024

holdercp Dec 3, 2024

allilevine Dec 3, 2024

holdercp Dec 3, 2024

holdercp commented Dec 3, 2024

allilevine commented Dec 3, 2024

holdercp left a comment

allilevine commented Dec 3, 2024

Hide empty content in favor of Reader onboarding. #96988

Hide empty content in favor of Reader onboarding. #96988

Conversation

allilevine commented Dec 2, 2024 • edited Loading

Proposed Changes

Why are these changes being made?

Testing Instructions

Pre-merge Checklist

github-actions bot commented Dec 2, 2024 • edited by matticbot Loading

matticbot commented Dec 2, 2024 • edited Loading

holdercp commented Dec 2, 2024

allilevine commented Dec 2, 2024

matticbot commented Dec 2, 2024

eoigal commented Dec 2, 2024

allilevine commented Dec 3, 2024

holdercp Dec 3, 2024

Choose a reason for hiding this comment

allilevine Dec 3, 2024

Choose a reason for hiding this comment

holdercp Dec 3, 2024

Choose a reason for hiding this comment

allilevine Dec 3, 2024

Choose a reason for hiding this comment

holdercp Dec 3, 2024

Choose a reason for hiding this comment

holdercp commented Dec 3, 2024

allilevine commented Dec 3, 2024

holdercp left a comment

Choose a reason for hiding this comment

allilevine commented Dec 3, 2024

allilevine commented Dec 2, 2024 •

edited

Loading

github-actions bot commented Dec 2, 2024 •

edited by matticbot

Loading

matticbot commented Dec 2, 2024 •

edited

Loading