-
Notifications
You must be signed in to change notification settings - Fork 2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Reader: Update action button link of an empty list of a user profile #98706
base: trunk
Are you sure you want to change the base?
Reader: Update action button link of an empty list of a user profile #98706
Conversation
Jetpack Cloud live (direct link)
Automattic for Agencies live (direct link)
|
Here is how your PR affects size of JS and CSS bundles shipped to the user's browser: Async-loaded Components (~176 bytes added 📈 [gzipped])
React components that are loaded lazily, when a certain part of UI is displayed for the first time. 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. Generated by performance advisor bot at iscalypsofastyet.com. |
This PR modifies the release build for the following Calypso Apps: For info about this notification, see here: PCYsg-OT6-p2
To test WordPress.com changes, run |
bdd3e9a
to
ac8eff3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This part doesn't seem to be working for me on Calypso Live. I'm still seeing "Back to Follow" with a link to /read.
- Go to the Lists tab of any user profile (example: /read/users/218803706/lists.
- Click on a list that have no posts yet (example: A Third List).
- Verify action button is showing "Back to User Profile" text with link /read/users/:id/lists.
Could we always link to the list owner's "Lists" page now? Is there any reason we should link to /read now that we have a user profile page?
It is fixed now.
If I navigate to the list from |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can see the button content updating and the href changing, but it loads into a wsod for me currently.
Repro:
- visited /read/users/218803706/lists
- clicked on "A third list" and let the empty list load. (URL on third list /read/list/artemiosans/a-third-list?last_page=/read/users/218803706/lists)
- clicked on "Back to user profile". This is a WSOD as it loads /read/list/artemiosans/%2Fread%2Fusers%2F218803706%2Flists instead of /read/users/218803706/lists
Note - the above is on calypso live. When running the branch locally im seeing the query param added to the url, but the button content and href not changing (continues to go to following). 😕
Are you seeing either of these issues?
client/reader/list-stream/empty.tsx
Outdated
if ( lastPageLink ) { | ||
return; | ||
} | ||
|
||
recordAction( 'clicked_following_on_empty' ); | ||
recordGaEvent( 'Clicked Following on EmptyContent' ); | ||
dispatch( recordReaderTracksEvent( 'calypso_reader_following_on_empty_list_stream_clicked' ) ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like we are avoiding firing the events around clicking for the following feed. I wonder if there is any value to these types of events, and if we should consider sending separate ones in the case of profile (or alternatively removing them all if not valuable).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also thought of removing this. Let me know if we need to remove this.
For now I am just removing the if
condition as firing the events for both cases is fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed in 376a54f
client/reader/list-stream/empty.tsx
Outdated
const dispatch = useDispatch(); | ||
const translate = useTranslate(); | ||
const queryParams = new URLSearchParams( location.search ); | ||
const lastPageLink = queryParams.get( 'last_page' ) ?? ''; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we pull this out of the URL and avoid keeping extra query params in the URL? We could probably look for the value on mount, remove it from the url, and use that initial info for the text/href.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove it from the url
I avoided it because it will create a new entry in the browser history.
ec3f72a
to
1249bb2
Compare
This should be fixed now (1249bb2). |
Related to https://github.com/Automattic/loop/issues/351
Proposed Changes
The issue was that on an empty list page, the "Back to Following" button with link
/read
was being displayed even when navigating from a user profile. This PR fixes this by updating the implementation to display a "Back to User Profile" button with link/read/users/:id/lists
instead, allowing users to easily navigate back to the user profile.Other than this I have also refactored
ListEmptyContent
to functional component so for easier review each commit can be reviewed separately.Why are these changes being made?
To improve the navigation.
Testing Instructions
/read/list/artemiosans/a-third-list
./read
./read/users/218803706/lists
.A Third List
)./read/users/:id/lists
.Pre-merge Checklist