From a3bbc624d5dd735d1640322525c4a3038dfa2af0 Mon Sep 17 00:00:00 2001 From: Max Duval Date: Sat, 3 Feb 2024 18:35:01 +0000 Subject: [PATCH 1/5] fix(Pagination): display correct thread count remove unused total page count from state, as it can be inferred from the filters --- .../manual/discussionWithTwoComments.ts | 6 +-- .../src/components/Discussion.tsx | 27 +++++++----- .../Discussion/Comments.stories.tsx | 34 ++++++++++---- .../src/components/Discussion/Comments.tsx | 17 ++++--- .../Discussion/Pagination.stories.tsx | 36 +++++---------- .../components/Discussion/Pagination.test.tsx | 3 +- .../src/components/Discussion/Pagination.tsx | 44 ++++++++++++------- 7 files changed, 91 insertions(+), 76 deletions(-) diff --git a/dotcom-rendering/fixtures/manual/discussionWithTwoComments.ts b/dotcom-rendering/fixtures/manual/discussionWithTwoComments.ts index d56c428ae56..9efe645e4c2 100644 --- a/dotcom-rendering/fixtures/manual/discussionWithTwoComments.ts +++ b/dotcom-rendering/fixtures/manual/discussionWithTwoComments.ts @@ -3,15 +3,15 @@ import type { GetDiscussionSuccess } from '../../src/types/discussion'; export const discussionWithTwoComments = { status: 'ok', currentPage: 1, - pages: 0, + pages: 1, pageSize: 100, orderBy: 'oldest', discussion: { key: '/p/39f5x', webUrl: 'https://www.theguardian.com/science/grrlscientist/2012/aug/07/3', apiUrl: 'https://discussion.guardianapis.com/discussion-api/discussion//p/39f5x', - commentCount: 0, - topLevelCommentCount: 0, + commentCount: 2, + topLevelCommentCount: 2, isClosedForComments: false, isClosedForRecommendation: false, isThreaded: true, diff --git a/dotcom-rendering/src/components/Discussion.tsx b/dotcom-rendering/src/components/Discussion.tsx index f6d6214f6d1..184a002f1e6 100644 --- a/dotcom-rendering/src/components/Discussion.tsx +++ b/dotcom-rendering/src/components/Discussion.tsx @@ -9,7 +9,6 @@ import { getCommentContext, initFiltersFromLocalStorage, } from '../lib/getCommentContext'; -import { useCommentCount } from '../lib/useCommentCount'; import { palette as themePalette } from '../palette'; import type { CommentForm, @@ -96,9 +95,10 @@ type State = { isClosedForComments: boolean; isExpanded: boolean; commentPage: number; + commentCount: number; + topLevelCommentCount: number; filters: FilterOptions; hashCommentId: number | undefined; - totalPages: number; loading: boolean; topForm: CommentForm; replyForm: CommentForm; @@ -115,9 +115,10 @@ const initialState: State = { isClosedForComments: false, isExpanded: false, commentPage: 1, + commentCount: 0, + topLevelCommentCount: 0, filters: initFiltersFromLocalStorage(), hashCommentId: undefined, - totalPages: 0, loading: true, topForm: initialCommentFormState, replyForm: initialCommentFormState, @@ -129,7 +130,8 @@ type Action = type: 'commentsLoaded'; comments: CommentType[]; isClosedForComments: boolean; - totalPages: number; + commentCount: number; + topLevelCommentCount: number; } | { type: 'expandComments' } | { type: 'addComment'; comment: CommentType } @@ -151,7 +153,8 @@ const reducer = (state: State, action: Action): State => { ...state, comments: action.comments, isClosedForComments: action.isClosedForComments, - totalPages: action.totalPages, + commentCount: action.commentCount, + topLevelCommentCount: action.topLevelCommentCount, loading: false, }; case 'addComment': @@ -261,17 +264,16 @@ export const Discussion = ({ commentPage, filters, hashCommentId, - totalPages, loading, topForm, replyForm, bottomForm, + topLevelCommentCount, + commentCount, }, dispatch, ] = useReducer(reducer, initialState); - const commentCount = useCommentCount(discussionApiUrl, shortUrlId); - useEffect(() => { const newHashCommentId = commentIdFromUrl(); if (newHashCommentId !== undefined) { @@ -291,13 +293,14 @@ export const Discussion = ({ return; } - const { pages, discussion } = result.value; + const { discussion } = result.value; dispatch({ type: 'commentsLoaded', comments: discussion.comments, isClosedForComments: discussion.isClosedForComments, - totalPages: pages, + topLevelCommentCount: discussion.topLevelCommentCount, + commentCount: discussion.commentCount, }); }) .catch(() => { @@ -400,9 +403,9 @@ export const Discussion = ({ }); }} filters={validFilters} - commentCount={commentCount ?? 0} + commentCount={commentCount} + topLevelCommentCount={topLevelCommentCount} loading={loading} - totalPages={totalPages} comments={comments} setComment={(comment: CommentType) => { dispatch({ type: 'addComment', comment }); diff --git a/dotcom-rendering/src/components/Discussion/Comments.stories.tsx b/dotcom-rendering/src/components/Discussion/Comments.stories.tsx index ad16ebc5110..cdea49244a8 100644 --- a/dotcom-rendering/src/components/Discussion/Comments.stories.tsx +++ b/dotcom-rendering/src/components/Discussion/Comments.stories.tsx @@ -74,8 +74,10 @@ export const LoggedOutHiddenPicks = () => ( setPage={() => {}} filters={filters} commentCount={discussionMock.discussion.commentCount} + topLevelCommentCount={ + discussionMock.discussion.topLevelCommentCount + } loading={false} - totalPages={discussionMock.pages} comments={discussionMock.discussion.comments} setComment={() => {}} handleFilterChange={() => {}} @@ -124,8 +126,10 @@ export const InitialPage = () => ( setPage={() => {}} filters={filters} commentCount={discussionMock.discussion.commentCount} + topLevelCommentCount={ + discussionMock.discussion.topLevelCommentCount + } loading={false} - totalPages={discussionMock.pages} comments={discussionMock.discussion.comments} setComment={() => {}} handleFilterChange={() => {}} @@ -178,8 +182,10 @@ export const LoggedInHiddenNoPicks = () => { setPage={() => {}} filters={filters} commentCount={discussionMock.discussion.commentCount} + topLevelCommentCount={ + discussionMock.discussion.topLevelCommentCount + } loading={false} - totalPages={discussionMock.pages} comments={discussionMock.discussion.comments} setComment={() => {}} handleFilterChange={() => {}} @@ -228,8 +234,10 @@ export const LoggedIn = () => { setPage={() => {}} filters={filters} commentCount={discussionMock.discussion.commentCount} + topLevelCommentCount={ + discussionMock.discussion.topLevelCommentCount + } loading={false} - totalPages={discussionMock.pages} comments={discussionMock.discussion.comments} setComment={() => {}} handleFilterChange={() => {}} @@ -283,8 +291,10 @@ export const LoggedInShortDiscussion = () => { setPage={() => {}} filters={filters} commentCount={discussionWithTwoComments.discussion.commentCount} + topLevelCommentCount={ + discussionWithTwoComments.discussion.topLevelCommentCount + } loading={false} - totalPages={discussionWithTwoComments.pages} comments={discussionWithTwoComments.discussion.comments} setComment={() => {}} handleFilterChange={() => {}} @@ -329,8 +339,10 @@ export const LoggedOutHiddenNoPicks = () => ( setPage={() => {}} filters={filters} commentCount={discussionMock.discussion.commentCount} + topLevelCommentCount={ + discussionMock.discussion.topLevelCommentCount + } loading={false} - totalPages={0} comments={discussionMock.discussion.comments} setComment={() => {}} handleFilterChange={() => {}} @@ -381,8 +393,10 @@ export const Closed = () => ( setPage={() => {}} filters={filters} commentCount={discussionMock.discussion.commentCount} + topLevelCommentCount={ + discussionMock.discussion.topLevelCommentCount + } loading={false} - totalPages={discussionMock.pages} comments={discussionMock.discussion.comments} setComment={() => {}} handleFilterChange={() => {}} @@ -431,8 +445,8 @@ export const NoComments = () => ( setPage={() => {}} filters={filters} commentCount={0} + topLevelCommentCount={0} loading={false} - totalPages={0} comments={[]} setComment={() => {}} handleFilterChange={() => {}} @@ -483,8 +497,10 @@ export const LegacyDiscussion = () => ( commentCount={ legacyDiscussionWithoutThreading.discussion.commentCount } + topLevelCommentCount={ + legacyDiscussionWithoutThreading.discussion.topLevelCommentCount + } loading={false} - totalPages={legacyDiscussionWithoutThreading.pages} comments={legacyDiscussionWithoutThreading.discussion.comments} setComment={() => {}} handleFilterChange={() => {}} diff --git a/dotcom-rendering/src/components/Discussion/Comments.tsx b/dotcom-rendering/src/components/Discussion/Comments.tsx index 54689ecc03e..20d6d4c4de3 100644 --- a/dotcom-rendering/src/components/Discussion/Comments.tsx +++ b/dotcom-rendering/src/components/Discussion/Comments.tsx @@ -39,8 +39,8 @@ type Props = { setPage: (page: number, shouldExpand: boolean) => void; filters: FilterOptions; commentCount: number; + topLevelCommentCount: number; loading: boolean; - totalPages: number; comments: CommentType[]; setComment: (comment: CommentType) => void; handleFilterChange: (newFilters: FilterOptions, page?: number) => void; @@ -122,8 +122,8 @@ export const Comments = ({ setPage, filters, commentCount, + topLevelCommentCount, loading, - totalPages, comments, setComment, handleFilterChange, @@ -236,7 +236,9 @@ export const Comments = ({ initialiseApi({ additionalHeaders, baseUrl, apiKey, idApiUrl }); - const showPagination = totalPages > 1; + const totalCount = + filters.threads === 'unthreaded' ? commentCount : topLevelCommentCount; + const showPagination = totalCount > filters.pageSize; if (!expanded && loading) { return ; @@ -262,10 +264,9 @@ export const Comments = ({ /> {showPagination && ( )} @@ -355,10 +356,9 @@ export const Comments = ({ /> {showPagination && ( )} @@ -407,10 +407,9 @@ export const Comments = ({ {showPagination && (
diff --git a/dotcom-rendering/src/components/Discussion/Pagination.stories.tsx b/dotcom-rendering/src/components/Discussion/Pagination.stories.tsx index df293e1ddaf..cb1f1d212b6 100644 --- a/dotcom-rendering/src/components/Discussion/Pagination.stories.tsx +++ b/dotcom-rendering/src/components/Discussion/Pagination.stories.tsx @@ -22,11 +22,10 @@ export const Default = () => { const [page, setCurrentPage] = useState(1); return ( ); }; @@ -37,11 +36,10 @@ export const TwoPages = () => { const [page, setCurrentPage] = useState(1); return ( ); }; @@ -52,11 +50,10 @@ export const ThreePages = () => { const [page, setCurrentPage] = useState(1); return ( ); }; @@ -67,11 +64,10 @@ export const FourPages = () => { const [page, setCurrentPage] = useState(1); return ( ); }; @@ -82,11 +78,10 @@ export const FivePages = () => { const [page, setCurrentPage] = useState(1); return ( ); }; @@ -97,11 +92,10 @@ export const SixPages = () => { const [page, setCurrentPage] = useState(1); return ( ); }; @@ -112,11 +106,10 @@ export const SevenPages = () => { const [page, setCurrentPage] = useState(1); return ( ); }; @@ -127,11 +120,10 @@ export const TwelvePages = () => { const [page, setCurrentPage] = useState(1); return ( ); }; @@ -142,11 +134,10 @@ export const LotsOfPages = () => { const [page, setCurrentPage] = useState(1); return ( ); }; @@ -157,11 +148,10 @@ export const WhenExpanded = () => { const [page, setCurrentPage] = useState(1); return ( ); }; @@ -172,11 +162,10 @@ export const WhenCollapsed = () => { const [page, setCurrentPage] = useState(1); return ( ); }; @@ -187,11 +176,10 @@ export const WhenUnthreaded = () => { const [page, setCurrentPage] = useState(1); return ( ); }; diff --git a/dotcom-rendering/src/components/Discussion/Pagination.test.tsx b/dotcom-rendering/src/components/Discussion/Pagination.test.tsx index f5ab6e7b711..c95e64343e4 100644 --- a/dotcom-rendering/src/components/Discussion/Pagination.test.tsx +++ b/dotcom-rendering/src/components/Discussion/Pagination.test.tsx @@ -12,11 +12,10 @@ describe('Pagination', () => { it('Render Ophan Data Components as expected', () => { const { asFragment } = render( {}} filters={DEFAULT_FILTERS} - commentCount={56} + totalCount={222} // 9 pages: 8.88 = 222 / 25 />, ); diff --git a/dotcom-rendering/src/components/Discussion/Pagination.tsx b/dotcom-rendering/src/components/Discussion/Pagination.tsx index 40ed46a6571..98c71f1ff06 100644 --- a/dotcom-rendering/src/components/Discussion/Pagination.tsx +++ b/dotcom-rendering/src/components/Discussion/Pagination.tsx @@ -14,10 +14,9 @@ import { palette as schemedPalette } from '../../palette'; import type { FilterOptions } from '../../types/discussion'; type Props = { - totalPages: number; currentPage: number; setCurrentPage: (currentPage: number) => void; - commentCount: number; + totalCount: number; filters: FilterOptions; }; @@ -232,12 +231,12 @@ const decideForthPage = ({ }; export const Pagination = ({ - totalPages, currentPage, setCurrentPage, - commentCount, + totalCount, filters, }: Props) => { + const totalPages = Math.ceil(totalCount / filters.pageSize); // Make decisions aobut which pagination elements to show const showBackButton = totalPages > 4 && currentPage > 1; const showFirstElipsis = totalPages > 4 && currentPage > 3; @@ -252,10 +251,25 @@ export const Pagination = ({ const showForwardButton = totalPages > 4 && currentPage !== totalPages; // Pagination Text - const { pageSize } = filters; // e.g: pageSize of 25 - let endIndex = pageSize * currentPage; // e.g: currentPage is page 2, endIndex is 25 * 2 = 50 - const startIndex = endIndex - (pageSize - 1); // e.g: 50 - (25 - 1) startIndex is 26. Example page 2 has comments 26 - 50 on it. - if (endIndex > commentCount) endIndex = commentCount; // If there are less total comments than allowed on the page, endIndex is total comment count + const { pageSize } = filters; + /** + * Starting index of **1** + * + * For example, with a page size of 25, and 63 comments total: + * 1. from 1 + * 2. from 26 + * 3. from 51 + */ + const startIndex = 1 + pageSize * (currentPage - 1); + /** + * Starting index of **1** + * + * For example, with a page size of 25, and 63 comments total: + * 1. until 25 + * 2. until 50 + * 3. until 63 + */ + const endIndex = Math.min(pageSize * currentPage, totalCount); return (
@@ -306,15 +320,11 @@ export const Pagination = ({ /> )}
- {!!commentCount && ( -
- {`Displaying ${ - filters.threads === 'unthreaded' - ? 'comments' - : 'threads' - } ${startIndex} to ${endIndex} of ${commentCount}`} -
- )} +
+ {`Displaying ${ + filters.threads === 'unthreaded' ? 'comments' : 'threads' + } ${startIndex} to ${endIndex} of ${totalCount}`} +
); }; From f12c2e24247530c4393671ade8fc95dba44ef6e2 Mon Sep 17 00:00:00 2001 From: Max Duval Date: Sat, 3 Feb 2024 18:35:01 +0000 Subject: [PATCH 2/5] test(Pagination): adjust props --- dotcom-rendering/src/components/Discussion/Comments.stories.tsx | 2 +- .../src/components/Discussion/Pagination.stories.tsx | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/dotcom-rendering/src/components/Discussion/Comments.stories.tsx b/dotcom-rendering/src/components/Discussion/Comments.stories.tsx index cdea49244a8..1c34c5b9566 100644 --- a/dotcom-rendering/src/components/Discussion/Comments.stories.tsx +++ b/dotcom-rendering/src/components/Discussion/Comments.stories.tsx @@ -491,7 +491,7 @@ export const LegacyDiscussion = () => ( onPermalinkClick={() => {}} apiKey="" idApiUrl="https://idapi.theguardian.com" - page={3} + page={2} setPage={() => {}} filters={filters} commentCount={ diff --git a/dotcom-rendering/src/components/Discussion/Pagination.stories.tsx b/dotcom-rendering/src/components/Discussion/Pagination.stories.tsx index cb1f1d212b6..ce35e307cb9 100644 --- a/dotcom-rendering/src/components/Discussion/Pagination.stories.tsx +++ b/dotcom-rendering/src/components/Discussion/Pagination.stories.tsx @@ -39,7 +39,7 @@ export const TwoPages = () => { currentPage={page} setCurrentPage={setCurrentPage} filters={DEFAULT_FILTERS} - totalCount={56} + totalCount={49} /> ); }; From c22a3837db696a9389cef1ef39cbcd21e70df926 Mon Sep 17 00:00:00 2001 From: Max Duval Date: Sat, 3 Feb 2024 18:35:01 +0000 Subject: [PATCH 3/5] fix(Discussion): only pass `totalCount` prop this is the only relevant value for `Filters` and `Comments` to display with correct pagination --- .../src/components/Discussion.tsx | 16 +++++--- .../Discussion/Comments.stories.tsx | 41 ++++--------------- .../src/components/Discussion/Comments.tsx | 14 +++---- .../components/Discussion/Filters.stories.tsx | 2 +- .../src/components/Discussion/Filters.tsx | 10 ++--- 5 files changed, 30 insertions(+), 53 deletions(-) diff --git a/dotcom-rendering/src/components/Discussion.tsx b/dotcom-rendering/src/components/Discussion.tsx index 184a002f1e6..4ee0255ac37 100644 --- a/dotcom-rendering/src/components/Discussion.tsx +++ b/dotcom-rendering/src/components/Discussion.tsx @@ -95,8 +95,8 @@ type State = { isClosedForComments: boolean; isExpanded: boolean; commentPage: number; - commentCount: number; - topLevelCommentCount: number; + commentCount: number | undefined; + topLevelCommentCount: number | undefined; filters: FilterOptions; hashCommentId: number | undefined; loading: boolean; @@ -115,8 +115,8 @@ const initialState: State = { isClosedForComments: false, isExpanded: false, commentPage: 1, - commentCount: 0, - topLevelCommentCount: 0, + commentCount: undefined, + topLevelCommentCount: undefined, filters: initFiltersFromLocalStorage(), hashCommentId: undefined, loading: true, @@ -274,6 +274,11 @@ export const Discussion = ({ dispatch, ] = useReducer(reducer, initialState); + const totalCount = + (filters.threads === 'unthreaded' + ? commentCount + : topLevelCommentCount) ?? 0; + useEffect(() => { const newHashCommentId = commentIdFromUrl(); if (newHashCommentId !== undefined) { @@ -403,8 +408,7 @@ export const Discussion = ({ }); }} filters={validFilters} - commentCount={commentCount} - topLevelCommentCount={topLevelCommentCount} + totalCount={totalCount} loading={loading} comments={comments} setComment={(comment: CommentType) => { diff --git a/dotcom-rendering/src/components/Discussion/Comments.stories.tsx b/dotcom-rendering/src/components/Discussion/Comments.stories.tsx index 1c34c5b9566..416c27bb00d 100644 --- a/dotcom-rendering/src/components/Discussion/Comments.stories.tsx +++ b/dotcom-rendering/src/components/Discussion/Comments.stories.tsx @@ -73,10 +73,7 @@ export const LoggedOutHiddenPicks = () => ( page={3} setPage={() => {}} filters={filters} - commentCount={discussionMock.discussion.commentCount} - topLevelCommentCount={ - discussionMock.discussion.topLevelCommentCount - } + totalCount={discussionMock.discussion.topLevelCommentCount} loading={false} comments={discussionMock.discussion.comments} setComment={() => {}} @@ -125,10 +122,7 @@ export const InitialPage = () => ( page={1} setPage={() => {}} filters={filters} - commentCount={discussionMock.discussion.commentCount} - topLevelCommentCount={ - discussionMock.discussion.topLevelCommentCount - } + totalCount={discussionMock.discussion.topLevelCommentCount} loading={false} comments={discussionMock.discussion.comments} setComment={() => {}} @@ -181,10 +175,7 @@ export const LoggedInHiddenNoPicks = () => { page={3} setPage={() => {}} filters={filters} - commentCount={discussionMock.discussion.commentCount} - topLevelCommentCount={ - discussionMock.discussion.topLevelCommentCount - } + totalCount={discussionMock.discussion.topLevelCommentCount} loading={false} comments={discussionMock.discussion.comments} setComment={() => {}} @@ -233,10 +224,7 @@ export const LoggedIn = () => { page={3} setPage={() => {}} filters={filters} - commentCount={discussionMock.discussion.commentCount} - topLevelCommentCount={ - discussionMock.discussion.topLevelCommentCount - } + totalCount={discussionMock.discussion.topLevelCommentCount} loading={false} comments={discussionMock.discussion.comments} setComment={() => {}} @@ -290,8 +278,7 @@ export const LoggedInShortDiscussion = () => { page={3} setPage={() => {}} filters={filters} - commentCount={discussionWithTwoComments.discussion.commentCount} - topLevelCommentCount={ + totalCount={ discussionWithTwoComments.discussion.topLevelCommentCount } loading={false} @@ -338,10 +325,7 @@ export const LoggedOutHiddenNoPicks = () => ( page={3} setPage={() => {}} filters={filters} - commentCount={discussionMock.discussion.commentCount} - topLevelCommentCount={ - discussionMock.discussion.topLevelCommentCount - } + totalCount={discussionMock.discussion.topLevelCommentCount} loading={false} comments={discussionMock.discussion.comments} setComment={() => {}} @@ -392,10 +376,7 @@ export const Closed = () => ( page={3} setPage={() => {}} filters={filters} - commentCount={discussionMock.discussion.commentCount} - topLevelCommentCount={ - discussionMock.discussion.topLevelCommentCount - } + totalCount={discussionMock.discussion.topLevelCommentCount} loading={false} comments={discussionMock.discussion.comments} setComment={() => {}} @@ -444,8 +425,7 @@ export const NoComments = () => ( page={3} setPage={() => {}} filters={filters} - commentCount={0} - topLevelCommentCount={0} + totalCount={0} loading={false} comments={[]} setComment={() => {}} @@ -494,12 +474,9 @@ export const LegacyDiscussion = () => ( page={2} setPage={() => {}} filters={filters} - commentCount={ + totalCount={ legacyDiscussionWithoutThreading.discussion.commentCount } - topLevelCommentCount={ - legacyDiscussionWithoutThreading.discussion.topLevelCommentCount - } loading={false} comments={legacyDiscussionWithoutThreading.discussion.comments} setComment={() => {}} diff --git a/dotcom-rendering/src/components/Discussion/Comments.tsx b/dotcom-rendering/src/components/Discussion/Comments.tsx index 20d6d4c4de3..970e2cdcb3c 100644 --- a/dotcom-rendering/src/components/Discussion/Comments.tsx +++ b/dotcom-rendering/src/components/Discussion/Comments.tsx @@ -38,8 +38,7 @@ type Props = { page: number; setPage: (page: number, shouldExpand: boolean) => void; filters: FilterOptions; - commentCount: number; - topLevelCommentCount: number; + totalCount: number; loading: boolean; comments: CommentType[]; setComment: (comment: CommentType) => void; @@ -121,8 +120,7 @@ export const Comments = ({ page, setPage, filters, - commentCount, - topLevelCommentCount, + totalCount, loading, comments, setComment, @@ -197,7 +195,7 @@ export const Comments = ({ */ const maxPagePossible = Math.ceil( - commentCount / newFilterObject.pageSize, + totalCount / newFilterObject.pageSize, ); if (page > maxPagePossible) { @@ -236,8 +234,6 @@ export const Comments = ({ initialiseApi({ additionalHeaders, baseUrl, apiKey, idApiUrl }); - const totalCount = - filters.threads === 'unthreaded' ? commentCount : topLevelCommentCount; const showPagination = totalCount > filters.pageSize; if (!expanded && loading) { @@ -260,7 +256,7 @@ export const Comments = ({ {showPagination && ( {showPagination && ( { ); }; diff --git a/dotcom-rendering/src/components/Discussion/Filters.tsx b/dotcom-rendering/src/components/Discussion/Filters.tsx index fdb7d85e54e..53c49a1102f 100644 --- a/dotcom-rendering/src/components/Discussion/Filters.tsx +++ b/dotcom-rendering/src/components/Discussion/Filters.tsx @@ -11,7 +11,7 @@ import { Dropdown } from './Dropdown'; type Props = { filters: FilterOptions; onFilterChange: (newFilterObject: FilterOptions) => void; - commentCount: number; + totalCount: number; }; const filterBar = css` @@ -43,7 +43,7 @@ const filterPadding = css` padding-right: ${space[3]}px; `; -export const Filters = ({ filters, onFilterChange, commentCount }: Props) => ( +export const Filters = ({ filters, onFilterChange, totalCount }: Props) => (
( { title: '25', value: '25', - disabled: commentCount <= 25, + disabled: totalCount <= 25, isActive: filters.pageSize === 25, }, { title: '50', value: '50', - disabled: commentCount <= 50, + disabled: totalCount <= 50, isActive: filters.pageSize === 50, }, { title: '100', value: '100', - disabled: commentCount <= 100, + disabled: totalCount <= 100, isActive: filters.pageSize === 100, }, ]} From 54f3b707842ef880ce8f1c75d97917fbf3a5ae33 Mon Sep 17 00:00:00 2001 From: Max Duval Date: Sat, 3 Feb 2024 18:35:01 +0000 Subject: [PATCH 4/5] refactor(totalCount): use topLevelCommentCount the API response will correctly report on the number of top-level comments based on whether the results should display threaded or not! --- .../src/components/Discussion.tsx | 11 ++----- .../Discussion/Comments.stories.tsx | 30 +++++++++++++------ .../src/components/Discussion/Comments.tsx | 18 +++++------ .../components/Discussion/Filters.stories.tsx | 2 +- .../src/components/Discussion/Filters.tsx | 14 +++++---- .../Discussion/Pagination.stories.tsx | 24 +++++++-------- .../components/Discussion/Pagination.test.tsx | 2 +- .../src/components/Discussion/Pagination.tsx | 10 +++---- 8 files changed, 61 insertions(+), 50 deletions(-) diff --git a/dotcom-rendering/src/components/Discussion.tsx b/dotcom-rendering/src/components/Discussion.tsx index 4ee0255ac37..2c8fafc3a6a 100644 --- a/dotcom-rendering/src/components/Discussion.tsx +++ b/dotcom-rendering/src/components/Discussion.tsx @@ -96,7 +96,7 @@ type State = { isExpanded: boolean; commentPage: number; commentCount: number | undefined; - topLevelCommentCount: number | undefined; + topLevelCommentCount: number; filters: FilterOptions; hashCommentId: number | undefined; loading: boolean; @@ -116,7 +116,7 @@ const initialState: State = { isExpanded: false, commentPage: 1, commentCount: undefined, - topLevelCommentCount: undefined, + topLevelCommentCount: 0, filters: initFiltersFromLocalStorage(), hashCommentId: undefined, loading: true, @@ -274,11 +274,6 @@ export const Discussion = ({ dispatch, ] = useReducer(reducer, initialState); - const totalCount = - (filters.threads === 'unthreaded' - ? commentCount - : topLevelCommentCount) ?? 0; - useEffect(() => { const newHashCommentId = commentIdFromUrl(); if (newHashCommentId !== undefined) { @@ -408,7 +403,7 @@ export const Discussion = ({ }); }} filters={validFilters} - totalCount={totalCount} + topLevelCommentCount={topLevelCommentCount} loading={loading} comments={comments} setComment={(comment: CommentType) => { diff --git a/dotcom-rendering/src/components/Discussion/Comments.stories.tsx b/dotcom-rendering/src/components/Discussion/Comments.stories.tsx index 416c27bb00d..362a4676ab7 100644 --- a/dotcom-rendering/src/components/Discussion/Comments.stories.tsx +++ b/dotcom-rendering/src/components/Discussion/Comments.stories.tsx @@ -73,7 +73,9 @@ export const LoggedOutHiddenPicks = () => ( page={3} setPage={() => {}} filters={filters} - totalCount={discussionMock.discussion.topLevelCommentCount} + topLevelCommentCount={ + discussionMock.discussion.topLevelCommentCount + } loading={false} comments={discussionMock.discussion.comments} setComment={() => {}} @@ -122,7 +124,9 @@ export const InitialPage = () => ( page={1} setPage={() => {}} filters={filters} - totalCount={discussionMock.discussion.topLevelCommentCount} + topLevelCommentCount={ + discussionMock.discussion.topLevelCommentCount + } loading={false} comments={discussionMock.discussion.comments} setComment={() => {}} @@ -175,7 +179,9 @@ export const LoggedInHiddenNoPicks = () => { page={3} setPage={() => {}} filters={filters} - totalCount={discussionMock.discussion.topLevelCommentCount} + topLevelCommentCount={ + discussionMock.discussion.topLevelCommentCount + } loading={false} comments={discussionMock.discussion.comments} setComment={() => {}} @@ -224,7 +230,9 @@ export const LoggedIn = () => { page={3} setPage={() => {}} filters={filters} - totalCount={discussionMock.discussion.topLevelCommentCount} + topLevelCommentCount={ + discussionMock.discussion.topLevelCommentCount + } loading={false} comments={discussionMock.discussion.comments} setComment={() => {}} @@ -278,7 +286,7 @@ export const LoggedInShortDiscussion = () => { page={3} setPage={() => {}} filters={filters} - totalCount={ + topLevelCommentCount={ discussionWithTwoComments.discussion.topLevelCommentCount } loading={false} @@ -325,7 +333,9 @@ export const LoggedOutHiddenNoPicks = () => ( page={3} setPage={() => {}} filters={filters} - totalCount={discussionMock.discussion.topLevelCommentCount} + topLevelCommentCount={ + discussionMock.discussion.topLevelCommentCount + } loading={false} comments={discussionMock.discussion.comments} setComment={() => {}} @@ -376,7 +386,9 @@ export const Closed = () => ( page={3} setPage={() => {}} filters={filters} - totalCount={discussionMock.discussion.topLevelCommentCount} + topLevelCommentCount={ + discussionMock.discussion.topLevelCommentCount + } loading={false} comments={discussionMock.discussion.comments} setComment={() => {}} @@ -425,7 +437,7 @@ export const NoComments = () => ( page={3} setPage={() => {}} filters={filters} - totalCount={0} + topLevelCommentCount={0} loading={false} comments={[]} setComment={() => {}} @@ -474,7 +486,7 @@ export const LegacyDiscussion = () => ( page={2} setPage={() => {}} filters={filters} - totalCount={ + topLevelCommentCount={ legacyDiscussionWithoutThreading.discussion.commentCount } loading={false} diff --git a/dotcom-rendering/src/components/Discussion/Comments.tsx b/dotcom-rendering/src/components/Discussion/Comments.tsx index 970e2cdcb3c..5bc2b650c36 100644 --- a/dotcom-rendering/src/components/Discussion/Comments.tsx +++ b/dotcom-rendering/src/components/Discussion/Comments.tsx @@ -38,7 +38,7 @@ type Props = { page: number; setPage: (page: number, shouldExpand: boolean) => void; filters: FilterOptions; - totalCount: number; + topLevelCommentCount: number; loading: boolean; comments: CommentType[]; setComment: (comment: CommentType) => void; @@ -120,7 +120,7 @@ export const Comments = ({ page, setPage, filters, - totalCount, + topLevelCommentCount, loading, comments, setComment, @@ -195,7 +195,7 @@ export const Comments = ({ */ const maxPagePossible = Math.ceil( - totalCount / newFilterObject.pageSize, + topLevelCommentCount / newFilterObject.pageSize, ); if (page > maxPagePossible) { @@ -234,7 +234,7 @@ export const Comments = ({ initialiseApi({ additionalHeaders, baseUrl, apiKey, idApiUrl }); - const showPagination = totalCount > filters.pageSize; + const showPagination = topLevelCommentCount > filters.pageSize; if (!expanded && loading) { return ; @@ -256,13 +256,13 @@ export const Comments = ({ {showPagination && ( )} @@ -348,13 +348,13 @@ export const Comments = ({ {showPagination && ( )} @@ -405,7 +405,7 @@ export const Comments = ({ diff --git a/dotcom-rendering/src/components/Discussion/Filters.stories.tsx b/dotcom-rendering/src/components/Discussion/Filters.stories.tsx index 729451642c6..249981d8b80 100644 --- a/dotcom-rendering/src/components/Discussion/Filters.stories.tsx +++ b/dotcom-rendering/src/components/Discussion/Filters.stories.tsx @@ -16,7 +16,7 @@ export const Default = () => { ); }; diff --git a/dotcom-rendering/src/components/Discussion/Filters.tsx b/dotcom-rendering/src/components/Discussion/Filters.tsx index 53c49a1102f..0803dbe5c93 100644 --- a/dotcom-rendering/src/components/Discussion/Filters.tsx +++ b/dotcom-rendering/src/components/Discussion/Filters.tsx @@ -11,7 +11,7 @@ import { Dropdown } from './Dropdown'; type Props = { filters: FilterOptions; onFilterChange: (newFilterObject: FilterOptions) => void; - totalCount: number; + topLevelCommentCount: number; }; const filterBar = css` @@ -43,7 +43,11 @@ const filterPadding = css` padding-right: ${space[3]}px; `; -export const Filters = ({ filters, onFilterChange, totalCount }: Props) => ( +export const Filters = ({ + filters, + onFilterChange, + topLevelCommentCount, +}: Props) => (
( { title: '25', value: '25', - disabled: totalCount <= 25, + disabled: topLevelCommentCount <= 25, isActive: filters.pageSize === 25, }, { title: '50', value: '50', - disabled: totalCount <= 50, + disabled: topLevelCommentCount <= 50, isActive: filters.pageSize === 50, }, { title: '100', value: '100', - disabled: totalCount <= 100, + disabled: topLevelCommentCount <= 100, isActive: filters.pageSize === 100, }, ]} diff --git a/dotcom-rendering/src/components/Discussion/Pagination.stories.tsx b/dotcom-rendering/src/components/Discussion/Pagination.stories.tsx index ce35e307cb9..d2d1f8b55e9 100644 --- a/dotcom-rendering/src/components/Discussion/Pagination.stories.tsx +++ b/dotcom-rendering/src/components/Discussion/Pagination.stories.tsx @@ -25,7 +25,7 @@ export const Default = () => { currentPage={page} setCurrentPage={setCurrentPage} filters={DEFAULT_FILTERS} - totalCount={200} + topLevelCommentCount={200} /> ); }; @@ -39,7 +39,7 @@ export const TwoPages = () => { currentPage={page} setCurrentPage={setCurrentPage} filters={DEFAULT_FILTERS} - totalCount={49} + topLevelCommentCount={49} /> ); }; @@ -53,7 +53,7 @@ export const ThreePages = () => { currentPage={page} setCurrentPage={setCurrentPage} filters={DEFAULT_FILTERS} - totalCount={75} + topLevelCommentCount={75} /> ); }; @@ -67,7 +67,7 @@ export const FourPages = () => { currentPage={page} setCurrentPage={setCurrentPage} filters={DEFAULT_FILTERS} - totalCount={100} + topLevelCommentCount={100} /> ); }; @@ -81,7 +81,7 @@ export const FivePages = () => { currentPage={page} setCurrentPage={setCurrentPage} filters={DEFAULT_FILTERS} - totalCount={124} + topLevelCommentCount={124} /> ); }; @@ -95,7 +95,7 @@ export const SixPages = () => { currentPage={page} setCurrentPage={setCurrentPage} filters={DEFAULT_FILTERS} - totalCount={149} + topLevelCommentCount={149} /> ); }; @@ -109,7 +109,7 @@ export const SevenPages = () => { currentPage={page} setCurrentPage={setCurrentPage} filters={DEFAULT_FILTERS} - totalCount={159} + topLevelCommentCount={159} /> ); }; @@ -123,7 +123,7 @@ export const TwelvePages = () => { currentPage={page} setCurrentPage={setCurrentPage} filters={DEFAULT_FILTERS} - totalCount={288} + topLevelCommentCount={288} /> ); }; @@ -137,7 +137,7 @@ export const LotsOfPages = () => { currentPage={page} setCurrentPage={setCurrentPage} filters={DEFAULT_FILTERS} - totalCount={490_000} + topLevelCommentCount={490_000} /> ); }; @@ -151,7 +151,7 @@ export const WhenExpanded = () => { currentPage={page} setCurrentPage={setCurrentPage} filters={{ ...DEFAULT_FILTERS, threads: 'expanded' }} - totalCount={100} + topLevelCommentCount={100} /> ); }; @@ -165,7 +165,7 @@ export const WhenCollapsed = () => { currentPage={page} setCurrentPage={setCurrentPage} filters={{ ...DEFAULT_FILTERS, threads: 'collapsed' }} - totalCount={100} + topLevelCommentCount={100} /> ); }; @@ -179,7 +179,7 @@ export const WhenUnthreaded = () => { currentPage={page} setCurrentPage={setCurrentPage} filters={{ ...DEFAULT_FILTERS, threads: 'unthreaded' }} - totalCount={100} + topLevelCommentCount={100} /> ); }; diff --git a/dotcom-rendering/src/components/Discussion/Pagination.test.tsx b/dotcom-rendering/src/components/Discussion/Pagination.test.tsx index c95e64343e4..ddbaf20dd0d 100644 --- a/dotcom-rendering/src/components/Discussion/Pagination.test.tsx +++ b/dotcom-rendering/src/components/Discussion/Pagination.test.tsx @@ -15,7 +15,7 @@ describe('Pagination', () => { currentPage={2} setCurrentPage={() => {}} filters={DEFAULT_FILTERS} - totalCount={222} // 9 pages: 8.88 = 222 / 25 + topLevelCommentCount={222} // 9 pages: 8.88 = 222 / 25 />, ); diff --git a/dotcom-rendering/src/components/Discussion/Pagination.tsx b/dotcom-rendering/src/components/Discussion/Pagination.tsx index 98c71f1ff06..846520edd9b 100644 --- a/dotcom-rendering/src/components/Discussion/Pagination.tsx +++ b/dotcom-rendering/src/components/Discussion/Pagination.tsx @@ -16,7 +16,7 @@ import type { FilterOptions } from '../../types/discussion'; type Props = { currentPage: number; setCurrentPage: (currentPage: number) => void; - totalCount: number; + topLevelCommentCount: number; filters: FilterOptions; }; @@ -233,10 +233,10 @@ const decideForthPage = ({ export const Pagination = ({ currentPage, setCurrentPage, - totalCount, + topLevelCommentCount, filters, }: Props) => { - const totalPages = Math.ceil(totalCount / filters.pageSize); + const totalPages = Math.ceil(topLevelCommentCount / filters.pageSize); // Make decisions aobut which pagination elements to show const showBackButton = totalPages > 4 && currentPage > 1; const showFirstElipsis = totalPages > 4 && currentPage > 3; @@ -269,7 +269,7 @@ export const Pagination = ({ * 2. until 50 * 3. until 63 */ - const endIndex = Math.min(pageSize * currentPage, totalCount); + const endIndex = Math.min(pageSize * currentPage, topLevelCommentCount); return (
@@ -323,7 +323,7 @@ export const Pagination = ({
{`Displaying ${ filters.threads === 'unthreaded' ? 'comments' : 'threads' - } ${startIndex} to ${endIndex} of ${totalCount}`} + } ${startIndex} to ${endIndex} of ${topLevelCommentCount}`}
); From 8e17570393c2a66909254cc5e74cf3380c660b7e Mon Sep 17 00:00:00 2001 From: Max Duval Date: Sat, 3 Feb 2024 18:35:01 +0000 Subject: [PATCH 5/5] fix: discussion count --- dotcom-rendering/fixtures/manual/short-discussion.ts | 4 ++-- dotcom-rendering/src/components/Discussion.tsx | 1 + 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/dotcom-rendering/fixtures/manual/short-discussion.ts b/dotcom-rendering/fixtures/manual/short-discussion.ts index 5a146683fa0..90ce363e1c2 100644 --- a/dotcom-rendering/fixtures/manual/short-discussion.ts +++ b/dotcom-rendering/fixtures/manual/short-discussion.ts @@ -8,13 +8,13 @@ export const shortDiscussion = { orderBy: 'oldest', discussion: { apiUrl: 'https://discussion.guardianapis.com/discussion-api/discussion//p/4v8kk', - commentCount: 7, + commentCount: 2, isClosedForComments: true, isClosedForRecommendation: true, isThreaded: true, key: '/p/4v8kk', title: 'Stevie Nicks to release double album of songs from her past', - topLevelCommentCount: 6, + topLevelCommentCount: 2, webUrl: 'https://www.theguardian.com/music/2014/jul/25/stevie-nicks-ro-release-double-album-of-songs-from-her-past', comments: [ { diff --git a/dotcom-rendering/src/components/Discussion.tsx b/dotcom-rendering/src/components/Discussion.tsx index 2c8fafc3a6a..59a818ce4de 100644 --- a/dotcom-rendering/src/components/Discussion.tsx +++ b/dotcom-rendering/src/components/Discussion.tsx @@ -354,6 +354,7 @@ export const Discussion = ({ }, []); useEffect(() => { + console.log({ commentCount }); // There's no point showing the view more button if there isn't much more to view if (commentCount === 0 || commentCount === 1 || commentCount === 2) { dispatch({ type: 'expandComments' });