-
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: enable pagination controls for Recent Feed #97674
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: Sections (~246 bytes added 📈 [gzipped])
Sections contain code specific for a given set of routes. Is downloaded and parsed only when a particular route is navigated to. Async-loaded Components (~136 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. |
I welcome any pushback on the approach I've taken here. I think I've made the better tradeoff but I'd love to hear some more opinions. |
3f2a1a9
to
37a9663
Compare
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 |
37a9663
to
03303df
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 is working for me. ✅ I did see the request hang when initially changing the number of items to display per page (from 10 to 100, in the gear icon) but I couldn't replicate it.
I wish we could fix this on the back-end, but it's great that this allows us to enable pagination again. 🙌
postId: string; | ||
} | ||
|
||
function isPaddingItem( item: ReaderPost | PaddingItem ): item is PaddingItem { |
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.
TIL about this pattern
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's really useful, especially when using Array.filter(). If you search around for "type guard using type predicate" you can learn more. Here's the section from the TS guide.
|
||
// This is a temporary fix to disable the pagination page selector because of this issue: Automattic/loop#247 | ||
// Once that issue is fixed, this code should be removed. | ||
.dataviews-pagination__page-select { |
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.
🎉
|
||
switch ( action.type ) { | ||
case READER_STREAMS_PAGE_RECEIVE: | ||
gap = action.payload.gap; | ||
streamItems = action.payload.streamItems; | ||
perPage = action.payload.perPage; |
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.
Do we need optional chaining here like on action.payload?.streamKey?
below?
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 don't think it would make a difference because perPage
would be set to undefined
either way.
I do notice now that I broke from this "destructuring" pattern when accessing action.payload.streamKey
so I'll change that to make it consistent.
Fixes Automattic/loop#238
Fixes Automattic/loop#247
Proposed Changes
"Pads" the stream item results so the correct number of items are present for the DataViews pagination to work correctly.
Screen.Recording.2025-01-09.at.4.43.01.PM.mov
There were two solutions I explored in fixing this issue:
See "Why are these changes being made" for an explanation of the issue.
I implemented approach 1 because it provided the best UX. The major downside is the additional code/types to handle a stream including "padding" items.
Why are these changes being made?
4
is selected in the DataViews component with a page size of10
, DataViews needs itsitems
prop to have40
or more items in it for the pagination logic to work correctly./read/streams/following
endpoint would request a single page at a time of a given size and add those results to the store.4
after initial load the endpoint would return page4
of the results and those items would be stored in the store. With a page size of10
, this would mean that the store had20
items in it: page1
(from initial load) and page4
.20
items would be passed to DataViews and when it tried to render page4
with a page size of10
with only20
items, it would break.Testing Instructions
/read
)Pre-merge Checklist