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.

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

Fix Pagination thread count in Discussion #10477

Merged
merged 5 commits into from
Feb 5, 2024
Merged

Conversation

mxdvl
Copy link
Contributor

@mxdvl mxdvl commented Feb 5, 2024

What does this change?

  • add a totalCount which depends on the threads filtering option
  • remove unused total page count from state, as it can be inferred from the filters’ page size
  • make the start and end index immutable

Why?

A user reported that the current thread count was incorrect:

Displaying threads 1 to 100 of 612 They are spread over three pages (I display 100 threads per page). Which cannot be right, so the 612 must relate to something other than threads (presumably posts). Following the update, you can end up on a page that claims, say, threads 200-300 of 620 but only has five posts on it. So it's become impossible to tell where you are in the sequence of posts.

We conflated commentCount and topLevelCommentCount in #10299 (comment)

Screenshots

Before After
before after

@mxdvl mxdvl requested a review from a team as a code owner February 5, 2024 12:02
Copy link

github-actions bot commented Feb 5, 2024

Size Change: -220 B (0%)

Total Size: 751 kB

Filename Size Change
dotcom-rendering/dist/1237.client.web.********************.js 0 B -5.12 kB (removed) 🏆
dotcom-rendering/dist/1412.client.web.********************.js 0 B -4.31 kB (removed) 🏆
dotcom-rendering/dist/1955.client.web.********************.js 0 B -4.71 kB (removed) 🏆
dotcom-rendering/dist/2276.client.web.********************.js 0 B -3.35 kB (removed) 🏆
dotcom-rendering/dist/3314.client.web.********************.js 0 B -4.76 kB (removed) 🏆
dotcom-rendering/dist/3410.client.web.********************.js 0 B -2.68 kB (removed) 🏆
dotcom-rendering/dist/3889.client.web.********************.js 0 B -8.24 kB (removed) 🏆
dotcom-rendering/dist/4002.client.web.********************.js 0 B -4.59 kB (removed) 🏆
dotcom-rendering/dist/4219.client.web.********************.js 0 B -5.74 kB (removed) 🏆
dotcom-rendering/dist/4265.client.web.********************.js 0 B -2.38 kB (removed) 🏆
dotcom-rendering/dist/4982.client.web.********************.js 0 B -3.82 kB (removed) 🏆
dotcom-rendering/dist/5144.client.web.********************.js 0 B -5.08 kB (removed) 🏆
dotcom-rendering/dist/5290.client.web.********************.js 0 B -3.24 kB (removed) 🏆
dotcom-rendering/dist/5499.client.web.********************.js 0 B -2.47 kB (removed) 🏆
dotcom-rendering/dist/5957.client.web.********************.js 0 B -5.93 kB (removed) 🏆
dotcom-rendering/dist/7329.client.web.********************.js 0 B -2.77 kB (removed) 🏆
dotcom-rendering/dist/7554.client.web.********************.js 0 B -3.27 kB (removed) 🏆
dotcom-rendering/dist/7629.client.web.********************.js 0 B -2.87 kB (removed) 🏆
dotcom-rendering/dist/7983.client.web.********************.js 0 B -3.1 kB (removed) 🏆
dotcom-rendering/dist/8113.client.web.********************.js 0 B -3.48 kB (removed) 🏆
dotcom-rendering/dist/8310.client.web.********************.js 0 B -3.74 kB (removed) 🏆
dotcom-rendering/dist/9004.client.web.********************.js 0 B -5.85 kB (removed) 🏆
dotcom-rendering/dist/9343.client.web.********************.js 0 B -2.98 kB (removed) 🏆
dotcom-rendering/dist/958.client.web.********************.js 0 B -3.17 kB (removed) 🏆
dotcom-rendering/dist/9879.client.web.********************.js 0 B -3.38 kB (removed) 🏆
dotcom-rendering/dist/DiscussionContainer-importable.client.web.********************.js 23.8 kB -137 B (-1%)
dotcom-rendering/dist/index.client.web.********************.js 47.1 kB -130 B (0%)
dotcom-rendering/dist/1202.client.web.********************.js 3.82 kB +3.82 kB (new file) 🆕
dotcom-rendering/dist/1465.client.web.********************.js 4.76 kB +4.76 kB (new file) 🆕
dotcom-rendering/dist/1801.client.web.********************.js 2.38 kB +2.38 kB (new file) 🆕
dotcom-rendering/dist/2211.client.web.********************.js 4.71 kB +4.71 kB (new file) 🆕
dotcom-rendering/dist/23.client.web.********************.js 3.17 kB +3.17 kB (new file) 🆕
dotcom-rendering/dist/2467.client.web.********************.js 5.09 kB +5.09 kB (new file) 🆕
dotcom-rendering/dist/2943.client.web.********************.js 5.74 kB +5.74 kB (new file) 🆕
dotcom-rendering/dist/3103.client.web.********************.js 3.24 kB +3.24 kB (new file) 🆕
dotcom-rendering/dist/3186.client.web.********************.js 2.98 kB +2.98 kB (new file) 🆕
dotcom-rendering/dist/3267.client.web.********************.js 2.68 kB +2.68 kB (new file) 🆕
dotcom-rendering/dist/3767.client.web.********************.js 2.88 kB +2.88 kB (new file) 🆕
dotcom-rendering/dist/4217.client.web.********************.js 3.36 kB +3.36 kB (new file) 🆕
dotcom-rendering/dist/4548.client.web.********************.js 3.11 kB +3.11 kB (new file) 🆕
dotcom-rendering/dist/5062.client.web.********************.js 4.31 kB +4.31 kB (new file) 🆕
dotcom-rendering/dist/5162.client.web.********************.js 4.59 kB +4.59 kB (new file) 🆕
dotcom-rendering/dist/5258.client.web.********************.js 3.38 kB +3.38 kB (new file) 🆕
dotcom-rendering/dist/5358.client.web.********************.js 8.24 kB +8.24 kB (new file) 🆕
dotcom-rendering/dist/5652.client.web.********************.js 5.84 kB +5.84 kB (new file) 🆕
dotcom-rendering/dist/5673.client.web.********************.js 5.95 kB +5.95 kB (new file) 🆕
dotcom-rendering/dist/6404.client.web.********************.js 3.48 kB +3.48 kB (new file) 🆕
dotcom-rendering/dist/8156.client.web.********************.js 3.26 kB +3.26 kB (new file) 🆕
dotcom-rendering/dist/8371.client.web.********************.js 3.75 kB +3.75 kB (new file) 🆕
dotcom-rendering/dist/8441.client.web.********************.js 2.78 kB +2.78 kB (new file) 🆕
dotcom-rendering/dist/8638.client.web.********************.js 2.46 kB +2.46 kB (new file) 🆕
dotcom-rendering/dist/9160.client.web.********************.js 5.12 kB +5.12 kB (new file) 🆕
ℹ️ View Unchanged
Filename Size Change
dotcom-rendering/dist/11.client.web.********************.js 5.78 kB -1 B (0%)
dotcom-rendering/dist/112.client.web.********************.js 822 B 0 B
dotcom-rendering/dist/1191.client.web.********************.js 680 B 0 B
dotcom-rendering/dist/1229.client.web.********************.js 8.67 kB -4 B (0%)
dotcom-rendering/dist/1407.client.web.********************.js 640 B 0 B
dotcom-rendering/dist/1459.client.web.********************.js 876 B 0 B
dotcom-rendering/dist/1749.client.web.********************.js 920 B 0 B
dotcom-rendering/dist/2293.client.web.********************.js 582 B 0 B
dotcom-rendering/dist/307.client.web.********************.js 13.2 kB +2 B (0%)
dotcom-rendering/dist/3375.client.web.********************.js 2.7 kB 0 B
dotcom-rendering/dist/393.client.web.********************.js 920 B 0 B
dotcom-rendering/dist/3958.client.web.********************.js 39.9 kB 0 B
dotcom-rendering/dist/3986.client.web.********************.js 495 B 0 B
dotcom-rendering/dist/4040.client.web.********************.js 650 B 0 B
dotcom-rendering/dist/4045.client.web.********************.js 643 B 0 B
dotcom-rendering/dist/405.client.web.********************.js 914 B 0 B
dotcom-rendering/dist/4215.client.web.********************.js 409 B 0 B
dotcom-rendering/dist/4269.client.web.********************.js 668 B 0 B
dotcom-rendering/dist/4298.client.web.********************.js 8.42 kB +1 B (0%)
dotcom-rendering/dist/4390.client.web.********************.js 508 B 0 B
dotcom-rendering/dist/4438.client.web.********************.js 801 B 0 B
dotcom-rendering/dist/4442.client.web.********************.js 714 B 0 B
dotcom-rendering/dist/478.client.web.********************.js 595 B 0 B
dotcom-rendering/dist/4870.client.web.********************.js 748 B 0 B
dotcom-rendering/dist/4911.client.web.********************.js 780 B 0 B
dotcom-rendering/dist/5020.client.web.********************.js 994 B 0 B
dotcom-rendering/dist/5041.client.web.********************.js 3.76 kB -2 B (0%)
dotcom-rendering/dist/5047.client.web.********************.js 779 B 0 B
dotcom-rendering/dist/5242.client.web.********************.js 529 B 0 B
dotcom-rendering/dist/5619.client.web.********************.js 926 B 0 B
dotcom-rendering/dist/5730.client.web.********************.js 954 B 0 B
dotcom-rendering/dist/5772.client.web.********************.js 9.96 kB +1 B (0%)
dotcom-rendering/dist/5985.client.web.********************.js 750 B 0 B
dotcom-rendering/dist/6043.client.web.********************.js 852 B 0 B
dotcom-rendering/dist/6140.client.web.********************.js 853 B 0 B
dotcom-rendering/dist/6651.client.web.********************.js 904 B 0 B
dotcom-rendering/dist/6693.client.web.********************.js 822 B 0 B
dotcom-rendering/dist/6756.client.web.********************.js 525 B 0 B
dotcom-rendering/dist/6853.client.web.********************.js 1 kB 0 B
dotcom-rendering/dist/7018.client.web.********************.js 787 B 0 B
dotcom-rendering/dist/7167.client.web.********************.js 16.5 kB 0 B
dotcom-rendering/dist/7356.client.web.********************.js 1 kB 0 B
dotcom-rendering/dist/7855.client.web.********************.js 788 B 0 B
dotcom-rendering/dist/8002.client.web.********************.js 801 B 0 B
dotcom-rendering/dist/8288.client.web.********************.js 23 kB 0 B
dotcom-rendering/dist/8344.client.web.********************.js 1.56 kB -1 B (0%)
dotcom-rendering/dist/8388.client.web.********************.js 618 B 0 B
dotcom-rendering/dist/841.client.web.********************.js 789 B 0 B
dotcom-rendering/dist/8784.client.web.********************.js 5.09 kB +1 B (0%)
dotcom-rendering/dist/8818.client.web.********************.js 748 B 0 B
dotcom-rendering/dist/8937.client.web.********************.js 888 B 0 B
dotcom-rendering/dist/9106.client.web.********************.js 2.93 kB +1 B (0%)
dotcom-rendering/dist/9173.client.web.********************.js 723 B 0 B
dotcom-rendering/dist/9314.client.web.********************.js 822 B 0 B
dotcom-rendering/dist/9458.client.web.********************.js 4.82 kB +2 B (0%)
dotcom-rendering/dist/9529.client.web.********************.js 3.25 kB +1 B (0%)
dotcom-rendering/dist/9591.client.web.********************.js 1.85 kB +1 B (0%)
dotcom-rendering/dist/9621.client.web.********************.js 723 B 0 B
dotcom-rendering/dist/9676.client.web.********************.js 889 B 0 B
dotcom-rendering/dist/9856.client.web.********************.js 5.75 kB -5 B (0%)
dotcom-rendering/dist/9978.client.web.********************.js 960 B 0 B
dotcom-rendering/dist/AdPortals-importable.client.web.********************.js 3.72 kB 0 B
dotcom-rendering/dist/AlreadyVisited-importable.client.web.********************.js 424 B 0 B
dotcom-rendering/dist/AppsEpic-importable.client.web.********************.js 4.09 kB -1 B (0%)
dotcom-rendering/dist/AppsFooter-importable.client.web.********************.js 3.57 kB -2 B (0%)
dotcom-rendering/dist/AppsLightboxImage-importable.client.web.********************.js 3 kB +1 B (0%)
dotcom-rendering/dist/AppsLightboxImageStore-importable.client.web.********************.js 2.42 kB 0 B
dotcom-rendering/dist/AudioAtomWrapper-importable.client.web.********************.js 3.58 kB -3 B (0%)
dotcom-rendering/dist/AustralianTerritorySwitcher-importable.client.web.********************.js 2.01 kB +4 B (0%)
dotcom-rendering/dist/Branding-importable.client.web.********************.js 2.56 kB -1 B (0%)
dotcom-rendering/dist/braze-web-sdk-core.client.web.********************.js 36.9 kB 0 B
dotcom-rendering/dist/BrazeMessaging-importable.client.web.********************.js 5.26 kB +2 B (0%)
dotcom-rendering/dist/CalloutBlockComponent-importable.client.web.********************.js 6.76 kB +5 B (0%)
dotcom-rendering/dist/CalloutEmbedBlockComponent-importable.client.web.********************.js 5.76 kB -1 B (0%)
dotcom-rendering/dist/CardCommentCount-importable.client.web.********************.js 4.42 kB -6 B (0%)
dotcom-rendering/dist/Carousel-importable.client.web.********************.js 5.51 kB -2 B (0%)
dotcom-rendering/dist/CarouselForNewsletters-importable.client.web.********************.js 5.65 kB -2 B (0%)
dotcom-rendering/dist/ChartAtom-importable.client.web.********************.js 538 B -3 B (-1%)
dotcom-rendering/dist/CommentCount-importable.client.web.********************.js 3.24 kB +1 B (0%)
dotcom-rendering/dist/DiscussionMeta-importable.client.web.********************.js 4.02 kB +3 B (0%)
dotcom-rendering/dist/DocumentBlockComponent-importable.client.web.********************.js 3.57 kB -4 B (0%)
dotcom-rendering/dist/EmbedBlockComponent-importable.client.web.********************.js 4.12 kB -3 B (0%)
dotcom-rendering/dist/EnhancePinnedPost-importable.client.web.********************.js 2.01 kB 0 B
dotcom-rendering/dist/FetchOnwardsData-importable.client.web.********************.js 2.55 kB -2 B (0%)
dotcom-rendering/dist/FilterKeyEventsToggle-importable.client.web.********************.js 3.35 kB 0 B
dotcom-rendering/dist/FocusStyles-importable.client.web.********************.js 611 B -1 B (0%)
dotcom-rendering/dist/FollowWrapper-importable.client.web.********************.js 790 B +1 B (0%)
dotcom-rendering/dist/FooterLabel-importable.client.web.********************.js 343 B 0 B
dotcom-rendering/dist/frameworks.client.web.********************.js 20.7 kB 0 B
dotcom-rendering/dist/GetCricketScoreboard-importable.client.web.********************.js 3.76 kB -4 B (0%)
dotcom-rendering/dist/GetMatchNav-importable.client.web.********************.js 10.8 kB +2 B (0%)
dotcom-rendering/dist/GetMatchStats-importable.client.web.********************.js 1.63 kB +2 B (0%)
dotcom-rendering/dist/GetMatchTabs-importable.client.web.********************.js 2.61 kB -6 B (0%)
dotcom-rendering/dist/guardian-braze-components-banner.client.web.********************.js 14.8 kB +10 B (0%)
dotcom-rendering/dist/guardian-braze-components-end-of-article.client.web.********************.js 9.29 kB +1 B (0%)
dotcom-rendering/dist/GuideAtomWrapper-importable.client.web.********************.js 782 B 0 B
dotcom-rendering/dist/HeaderTopBar-importable.client.web.********************.js 10.9 kB -2 B (0%)
dotcom-rendering/dist/InstagramBlockComponent-importable.client.web.********************.js 3.66 kB -8 B (0%)
dotcom-rendering/dist/InteractiveBlockComponent-importable.client.web.********************.js 6.09 kB -4 B (0%)
dotcom-rendering/dist/InteractiveContentsBlockComponent-importable.client.web.********************.js 4.66 kB -3 B (0%)
dotcom-rendering/dist/InteractiveSupportButton-importable.client.web.********************.js 3.45 kB 0 B
dotcom-rendering/dist/KeyEventsCarousel-importable.client.web.********************.js 4.56 kB +4 B (0%)
dotcom-rendering/dist/KnowledgeQuizAtom-importable.client.web.********************.js 3.53 kB -1 B (0%)
dotcom-rendering/dist/LatestLinks-importable.client.web.********************.js 2.13 kB +3 B (0%)
dotcom-rendering/dist/LightboxHash-importable.client.web.********************.js 432 B 0 B
dotcom-rendering/dist/LightboxLayout-importable.client.web.********************.js 6.46 kB +7 B (0%)
dotcom-rendering/dist/LiveBlogEpic-importable.client.web.********************.js 3.59 kB -1 B (0%)
dotcom-rendering/dist/Liveness-importable.client.web.********************.js 4.94 kB +8 B (0%)
dotcom-rendering/dist/ManyNewsletterSignUp-importable.client.web.********************.js 6.61 kB -1 B (0%)
dotcom-rendering/dist/MapEmbedBlockComponent-importable.client.web.********************.js 5.65 kB 0 B
dotcom-rendering/dist/Metrics-importable.client.web.********************.js 2.29 kB -1 B (0%)
dotcom-rendering/dist/MostViewedFooter-importable.client.web.********************.js 4.04 kB +1 B (0%)
dotcom-rendering/dist/MostViewedFooterData-importable.client.web.********************.js 6.74 kB 0 B
dotcom-rendering/dist/MostViewedRightWrapper-importable.client.web.********************.js 4.45 kB +1 B (0%)
dotcom-rendering/dist/OnwardsUpper-importable.client.web.********************.js 4.26 kB -1 B (0%)
dotcom-rendering/dist/PersonalityQuizAtom-importable.client.web.********************.js 3.65 kB +1 B (0%)
dotcom-rendering/dist/ProfileAtom-importable.client.web.********************.js 543 B 0 B
dotcom-rendering/dist/ProfileAtomWrapper-importable.client.web.********************.js 800 B 0 B
dotcom-rendering/dist/PulsingDot-importable.client.web.********************.js 749 B 0 B
dotcom-rendering/dist/QandaAtom-importable.client.web.********************.js 536 B 0 B
dotcom-rendering/dist/ReaderRevenueDev-importable.client.web.********************.js 469 B 0 B
dotcom-rendering/dist/readerRevenueDevUtils.client.web.********************.js 1.89 kB 0 B
dotcom-rendering/dist/ReaderRevenueLinks-importable.client.web.********************.js 5.78 kB +7 B (0%)
dotcom-rendering/dist/RelativeTime-importable.client.web.********************.js 1.98 kB 0 B
dotcom-rendering/dist/RichLinkComponent-importable.client.web.********************.js 6.21 kB 0 B
dotcom-rendering/dist/SecureSignup-importable.client.web.********************.js 3.5 kB -3 B (0%)
dotcom-rendering/dist/SendAMessage-importable.client.web.********************.js 4.39 kB +3 B (0%)
dotcom-rendering/dist/SendTargetingParams-importable.client.web.********************.js 2.11 kB 0 B
dotcom-rendering/dist/sentry.client.web.********************.js 771 B 0 B
dotcom-rendering/dist/SetABTests-importable.client.web.********************.js 3.42 kB 0 B
dotcom-rendering/dist/SetAdTargeting-importable.client.web.********************.js 481 B 0 B
dotcom-rendering/dist/shimport.client.web.********************.js 2.79 kB 0 B
dotcom-rendering/dist/ShowHideContainers-importable.client.web.********************.js 646 B 0 B
dotcom-rendering/dist/ShowMore-importable.client.web.********************.js 5.71 kB +3 B (0%)
dotcom-rendering/dist/SignInGateMain.client.web.********************.js 3.91 kB +5 B (0%)
dotcom-rendering/dist/SignInGateMainCheckoutComplete.client.web.********************.js 5.01 kB +2 B (0%)
dotcom-rendering/dist/SignInGateSelector-importable.client.web.********************.js 5.64 kB -1 B (0%)
dotcom-rendering/dist/SlotBodyEnd-importable.client.web.********************.js 6.84 kB +4 B (0%)
dotcom-rendering/dist/SpotifyBlockComponent-importable.client.web.********************.js 5.5 kB +5 B (0%)
dotcom-rendering/dist/StickyBottomBanner-importable.client.web.********************.js 5.38 kB +1 B (0%)
dotcom-rendering/dist/SubNav-importable.client.web.********************.js 2.24 kB 0 B
dotcom-rendering/dist/SupportTheG-importable.client.web.********************.js 5.91 kB +7 B (0%)
dotcom-rendering/dist/TableOfContents-importable.client.web.********************.js 3.1 kB -5 B (0%)
dotcom-rendering/dist/TimelineAtom-importable.client.web.********************.js 1.24 kB -4 B (0%)
dotcom-rendering/dist/TweetBlockComponent-importable.client.web.********************.js 1.02 kB -2 B (0%)
dotcom-rendering/dist/UnsafeEmbedBlockComponent-importable.client.web.********************.js 3.67 kB -5 B (0%)
dotcom-rendering/dist/VideoFacebookBlockComponent-importable.client.web.********************.js 5.67 kB +3 B (0%)
dotcom-rendering/dist/VineBlockComponent-importable.client.web.********************.js 3.51 kB -6 B (0%)
dotcom-rendering/dist/WeatherWrapper-importable.client.web.********************.js 5.45 kB -1 B (0%)
dotcom-rendering/dist/YoutubeBlockComponent-importable.client.web.********************.js 3.9 kB +2 B (0%)

compressed-size-action

remove unused total page count from state,
as it can be inferred from the filters
@mxdvl mxdvl force-pushed the mxdvl/discussion/pagination branch from 08336ca to a3bbc62 Compare February 5, 2024 12:09
@mxdvl mxdvl added the run_chromatic Runs chromatic when label is applied label Feb 5, 2024
mxdvl added 2 commits February 5, 2024 13:25
this is the only relevant value for `Filters`
and `Comments` to display with correct pagination
Copy link
Contributor

@cemms1 cemms1 left a comment

Choose a reason for hiding this comment

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

Right so topLevelCommentCount is the number of "comments" not including threads/replies and commentCount is the number of both comments + replies

It all makes sense now but wonder if we can put that in a JSDoc comment somewhere for folk (incl. us) in the future

@mxdvl
Copy link
Contributor Author

mxdvl commented Feb 5, 2024

Right so topLevelCommentCount is the number of "comments" not including threads/replies and commentCount is the number of both comments + replies

Not quite, actually! Thanks for asking as I only realised after you wrote this.

Looking at the API responses, topLevelCommentCount is always the correct number of comments for the current filters. The Discussion API knows whether we are displaying comments threaded or not so we can always use this value rather than creating a new derived one!

mxdvl added 2 commits February 5, 2024 17:15
the API response will correctly report
on the number of top-level comments based
on whether the results should display
threaded or not!
@mxdvl mxdvl merged commit 7247448 into main Feb 5, 2024
28 checks passed
@mxdvl mxdvl deleted the mxdvl/discussion/pagination branch February 5, 2024 17:50
@prout-bot
Copy link

Seen on PROD (merged by @mxdvl 8 minutes and 40 seconds ago) Please check your changes!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dotcom-rendering run_chromatic Runs chromatic when label is applied Seen-on-PROD
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants