Skip to content
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.

Sign up for GitHub

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

Merged
merged 5 commits into from
Jan 10, 2025
Merged

Reader: enable pagination controls for Recent Feed #97674

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
3cdde6c
Use padding items
holdercp Jan 9, 2025
03303df
Unhide controls
holdercp Jan 9, 2025
e4c60f3
Account for streamKey being undefined.
holdercp Jan 10, 2025
41b2ba6
Account for no streamKey... for real this time
holdercp Jan 10, 2025
2fae39b
Make payload prop access consistent
holdercp Jan 10, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 32 additions & 6 deletions client/reader/recent/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import NavigationHeader from 'calypso/components/navigation-header';
import { getPostByKey } from 'calypso/state/reader/posts/selectors';
import { requestPaginatedStream } from 'calypso/state/reader/streams/actions';
import { viewStream } from 'calypso/state/reader-ui/actions';
import Skeleton from '../components/skeleton';
import EngagementBar from './engagement-bar';
import RecentPostField from './recent-post-field';
import RecentPostSkeleton from './recent-post-skeleton';
Expand All @@ -25,6 +26,15 @@ interface RecentProps {
viewToggle?: React.ReactNode;
}

interface PaddingItem {
isPadding: true;
postId: string;
}

function isPaddingItem( item: ReaderPost | PaddingItem ): item is PaddingItem {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TIL about this pattern

Copy link
Contributor Author

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.

return 'isPadding' in item;
}

const Recent = ( { viewToggle }: RecentProps ) => {
const dispatch = useDispatch< ThunkDispatch< AppState, void, UnknownAction > >();
const [ selectedItem, setSelectedItem ] = useState< ReaderPost | null >( null );
Expand Down Expand Up @@ -65,7 +75,10 @@ const Recent = ( { viewToggle }: RecentProps ) => {
return {};
}

return items.reduce( ( acc: Record< string, PostItem >, item: ReaderPost ) => {
return items.reduce( ( acc: Record< string, PostItem >, item: ReaderPost | PaddingItem ) => {
if ( isPaddingItem( item ) ) {
return acc;
}
const post = getPostByKey( state, {
feedId: item.feedId,
postId: item.postId,
Expand All @@ -90,7 +103,10 @@ const Recent = ( { viewToggle }: RecentProps ) => {
{
id: 'icon',
label: translate( 'Icon' ),
render: ( { item }: { item: ReaderPost } ) => {
render: ( { item }: { item: ReaderPost | PaddingItem } ) => {
if ( isPaddingItem( item ) ) {
return <Skeleton height="24px" width="24px" shape="circle" />;
}
const post = getPostFromItem( item );
const iconUrl = post?.site_icon?.img || post?.author?.avatar_URL || '';
return iconUrl ? <ReaderAvatar siteIcon={ iconUrl } iconSize={ 24 } /> : null;
Expand All @@ -101,9 +117,19 @@ const Recent = ( { viewToggle }: RecentProps ) => {
{
id: 'post',
label: translate( 'Post' ),
getValue: ( { item }: { item: ReaderPost } ) =>
`${ getPostFromItem( item )?.title ?? '' } - ${ item?.site_name ?? '' }`,
render: ( { item }: { item: ReaderPost } ) => {
getValue: ( { item }: { item: ReaderPost | PaddingItem } ) =>
isPaddingItem( item )
? ''
: `${ getPostFromItem( item )?.title ?? '' } - ${ item?.site_name ?? '' }`,
render: ( { item }: { item: ReaderPost | PaddingItem } ) => {
if ( isPaddingItem( item ) ) {
return (
<>
<Skeleton height="10px" width="100%" style={ { marginBottom: '8px' } } />
<Skeleton height="8px" width="50%" />
</>
);
}
return (
<div onFocus={ () => handleItemFocus( item.postId?.toString() ) }>
<RecentPostField
Expand Down Expand Up @@ -181,7 +207,7 @@ const Recent = ( { viewToggle }: RecentProps ) => {
const focusedItem = shownData.find(
( item ) => item.postId?.toString() === focusedIndexRef.current
);
if ( focusedItem ) {
if ( focusedItem && ! isPaddingItem( focusedItem ) ) {
setSelectedItem( focusedItem );
setTimeout( () => {
postColumnRef.current?.focus();
Expand Down
10 changes: 0 additions & 10 deletions client/reader/recent/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -184,16 +184,6 @@ body.is-reader-full-post {
padding: $recent-feed-spacing;
}
}

// 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 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉

display: none;

& + div {
margin-left: auto;
}
}
}

&__post-column {
Expand Down
4 changes: 3 additions & 1 deletion client/state/data-layer/wpcom/read/streams/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -427,7 +427,7 @@ function get_page_handle( streamType, action, data ) {

export function handlePage( action, data ) {
const { posts, sites, cards } = data;
const { streamKey, query, isPoll, gap, streamType } = action.payload;
const { streamKey, query, isPoll, gap, streamType, page, perPage } = action.payload;
const pageHandle = get_page_handle( streamType, action, data );
const { dateProperty } = streamApis[ streamType ];

Expand Down Expand Up @@ -502,6 +502,8 @@ export function handlePage( action, data ) {
gap,
totalItems,
totalPages,
page,
perPage,
} )
);
}
Expand Down
13 changes: 12 additions & 1 deletion client/state/reader/streams/actions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,16 @@ export function requestPage( {
};
}

export function receivePage( { streamKey, pageHandle, streamItems, gap, totalItems, totalPages } ) {
export function receivePage( {
streamKey,
pageHandle,
streamItems,
gap,
totalItems,
totalPages,
page,
perPage,
} ) {
return {
type: READER_STREAMS_PAGE_RECEIVE,
payload: {
Expand All @@ -57,6 +66,8 @@ export function receivePage( { streamKey, pageHandle, streamItems, gap, totalIte
gap,
totalItems,
totalPages,
page,
perPage,
},
};
}
Expand Down
39 changes: 39 additions & 0 deletions client/state/reader/streams/reducer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,16 +36,55 @@ export const items = ( state = [], action ) => {
let gap;
let newState;
let newXPosts;
let perPage;
let page;

switch ( action.type ) {
case READER_STREAMS_PAGE_RECEIVE:
gap = action.payload.gap;
streamItems = action.payload.streamItems;
perPage = action.payload.perPage;
Copy link
Member

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?

Copy link
Contributor Author

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.

page = action.payload.page;

if ( ! Array.isArray( streamItems ) ) {
return state;
}

// For the Recent feeds, we need to pad the stream with empty items
// for the DataViews pagination to work correctly
// see Automattic/loop#238
if (
action.payload?.streamKey?.startsWith( 'recent' ) &&
streamItems.length > 0 &&
perPage &&
page > 1
) {
// Calculate where new items should start
const startIndex = ( page - 1 ) * perPage;
const existingLength = state.length;
const paddingNeeded = startIndex - existingLength;

// Case 1: Need to add padding before new items
if ( paddingNeeded > 0 ) {
const paddingItems = Array( paddingNeeded )
.fill( undefined )
.map( ( _, index ) => ( {
isPadding: true,
postId: `padding-${ index }`,
} ) );

return combineXPosts( [ ...state, ...paddingItems, ...streamItems ] );
}

// Case 2: Replace existing items at correct index
const updatedState = [ ...state ];
streamItems.forEach( ( item, index ) => {
updatedState[ startIndex + index ] = item;
} );

return combineXPosts( updatedState );
}

if ( gap ) {
const beforeGap = takeWhile( state, ( postKey ) => ! keysAreEqual( postKey, gap ) );
const afterGap = takeRightWhile( state, ( postKey ) => ! keysAreEqual( postKey, gap ) );
Expand Down
Loading