Improve pagination documentation #9488

ManorSailor · 2024-09-25T15:05:10Z

Description (required)

Following the discussion at #9442; This PR does the following:

Adds type info to Page prop.
- As per Page page is a generic type. As a tradeoff, TData
- Where should we mention TData is of type any? Type? Or in description? Should we mention it at all? TData is generic enough...
  name was decided to balance the documentation for TS & non-TS folks alike.
Adds the same type to page.data property
Adds type information for the getStaticPaths function
- As the said function can be used not only for content collection, but also for data fetched via APIs... I believe adding it under the current section makes sense. I have a doubt though, how will it infer the types for data returned via API?
- Added the block just above the caution block. Let me know how it looks.
& 5. The paginate function's type definition was too complex to be easily explained in the documentation. Therefore, I thought of describing the function signature in the docs instead.
Removed redundant Page type reference from the Routing guide
As suggested by @ArmandPhilippot, each mention of CollectionEntry<collection> has been replaced with CollectionEntry<TCollectionName> as specified by its documentation.

Anyways. That's all. Feel free to provide any inputs!

Special thanks to @ArmandPhilippot for his detailed responses. It cleared a lot of confusion.

Related issues & labels (optional)

Closes Improvements to Pagination Documentation: Type Information, Argument Descriptions, and Redundant Sections #9442
Suggested label: improve-documentation

netlify · 2024-09-25T15:05:26Z

✅ Deploy Preview for astro-docs-2 ready!

Built without sensitive environment variables

Name	Link
🔨 Latest commit	`b694fb6`
🔍 Latest deploy log	https://app.netlify.com/sites/astro-docs-2/deploys/66f6dee71d2e110008f130a3
😎 Deploy Preview	https://deploy-preview-9488--astro-docs-2.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

astrobot-houston · 2024-09-25T15:08:26Z

Lunaria Status Overview

🌕 This pull request will trigger status changes.

Learn more

By default, every PR changing files present in the Lunaria configuration's files property will be considered and trigger status changes accordingly.

You can change this by adding one of the keywords present in the ignoreKeywords property in your Lunaria configuration file in the PR's title (ignoring all files) or by including a tracker directive in the merged commit's description.

Tracked Files

Locale	File	Note
en	guides/routing.mdx	Source changed, localizations will be marked as outdated.
en	reference/api-reference.mdx	Source changed, localizations will be marked as outdated.

Warnings reference

Icon	Description
🔄️	The source for this localization has been updated since the creation of this pull request, make sure all changes in the source have been applied.

src/content/docs/en/reference/api-reference.mdx

As per https://github.com/withastro/astro/blob/8d4eb95086ae79339764d02215f84f4f21b96edc/packages/astro/src/%40types/astro.ts#L2804 `page` is a generic type. As a tradeoff, `TData` name was decided to balance the documentation for TS & non-TS folks alike.

The `paginate` function's type definition was too complex to be easily explained in the documentation. Hence, this commit simplifies things by just describing the expected arguments. This makes the documentation easier to follow while still providing the necessary information.

Some parts of the docs were using `CollectionEntry<collection>` while others were using `TCollectionName`. This commit cleans things up by sticking to `TCollectionName`, making everything consistent and easier to follow.

sarah11918 · 2024-09-26T15:20:45Z

src/content/docs/en/guides/routing.mdx

@@ -459,41 +459,6 @@ const { page } = Astro.props;
 {page.url.last ? <a href={page.url.last}>Last</a> : null}
 ```

-
-#### Complete API reference


While I agree with not having this on the routing page, do you think we lose anything by not putting this e.g. right before all the individual reference items? I mean, yeah it's nice that they are all listed out individually, but many of these do fall into the "self-explanatory" category, so only needing to glance at this to know what all's available seems helpful? Otherwise, it's scrolling through 3 pages on my monitor to see everything.

otherwise, I might be tempted to leave something like this here, even if not claiming to be fully typed, but more to show available properties?

You raise a good point. We lose the convenience of glancing at all of the available properties, & having to navigate through the wall of text in the API Reference section... I agree that most of the properties are self explanatory. I can imagine that the some users may find it inconvenient to scroll all that just to see the available properties.

Considering the repercussions, keeping that block there seems warranted even though it adds to the maintenance costs.... Although, it still irks me. 😓 Surely, we can do better....

In hindsight, I realize that the actual purpose of that code block there was to serve as an At a glance section to showcase all of the supported properties by Page.

I was going to suggest that we simplify the type to a block which only mentions all of the available properties, but I realize that it would be much more work than simply copy-pasting the updated type from the codebase.

Should I revert back to its original state then?

I made a commit with an example of what I think could be a helpful structure to work from. What do you think?

though, we did lose this text which might go in the block itself for data

array containing the page’s slice of data that you passed to the paginate() function

It seems we are coming back to #8929 (comment) 😅 So, it's probably better to leave the block here. And, I agree with Sarah, we could replace result which is not descriptive enough with the sentence quoted!

I think that adding the description for data property in the block would work perfectly fine.

ArmandPhilippot · 2024-09-26T20:46:45Z

src/content/docs/en/reference/api-reference.mdx

@@ -1116,6 +1116,10 @@ The `getStaticPaths()` function should return an array of objects to determine w

 It can also be used in static file endpoints for [dynamic routing](/en/guides/endpoints/#params-and-dynamic-routing).

+:::tip
+When using TypeScript, use the [`InferGetStatic*`](/en/guides/typescript/#infer-getstaticpaths-types) type utilities to ensure type-safe access of your `params` and `props`.


I'm a little late, I was waiting until the PR was no longer a draft to comment.

InferGetStatic does not exist and I don't think the asterisk is understandable to everyone. It seems to me that GetStaticPaths is enough to have both Astro.params and Astrop.props inferred properly. The two other helpers (InferGetStaticParamsType and InferGetStaticPropsType) can be useful in some cases but I don't think they are required here.
So I would suggest:

Suggested change

When using TypeScript, use the [`InferGetStatic*`](/en/guides/typescript/#infer-getstaticpaths-types) type utilities to ensure type-safe access of your `params` and `props`.

When using TypeScript, use the [`GetStaticPaths`](/en/guides/typescript/#infer-getstaticpaths-types) type utility to ensure type-safe access of your `params` and `props`.

Thanks for the suggestion! How about the following phrasing:

When using TypeScript, follow the [Infer `getStaticPaths` types](/en/guides/typescript/#infer-getstaticpaths-types) guide to ensure type-safe access of your `params` and `props`.

Why not, all types are useful. If I insisted on GetStaticPaths it is perhaps because I find that this section of the Typescript guide seems to insist on the other two types (InferGetStaticParamsType and InferGetStaticPropsType) when only one (GetStaticPaths) is enough for most cases. But that might just be my perception...
So I'm fine with your suggestion, if you think it's more helpful!

After looking into it, it appears that GetStaticPaths is sufficient for type safety. However, this raises the question of why there are additional type utilities included. Additionally, that section does not highlight the line where we add satisfies GetStaticPaths; it only mentions the InferGetStatic* types. It seems like there’s a new area for improvement here? Though I believe it falls outside the scope of this PR.

Considering this, I will go with your suggestion. It seems satisfactory.

Um yes I think there is a missing line among those highlighted. Maybe it was forgotten because the opening parenthesis is also important in this case. As for the usefulness of the other types... I don't know. I've never used them, but no doubt some people need them. Better excess types than missing types! 😄
But you're right, it doesn't fit into this PR.

sarah11918 · 2024-09-27T10:01:24Z

Just updated branch to re-trigger the Netlify deploy. We had some random wonkiness yesterday.

ArmandPhilippot

Some nitpicks about the wording. But I'm not the best person to judge so I'm not blocking the PR for that, LGTM!

ArmandPhilippot · 2024-09-27T17:07:08Z

src/content/docs/en/guides/routing.mdx

@@ -494,6 +469,25 @@ interface Page<T = any> {
 }
 ```

+The following example template displays current information for the page along with links to navigate between pages:


I'm not sure template is needed here... I feel like it makes the sentence heavier... (I don't know if it makes sense in English) but I'm not the best person to discuss the wording.

Suggested change

The following example template displays current information for the page along with links to navigate between pages:

The following example displays current information for the page along with links to navigate between pages:

ArmandPhilippot · 2024-09-27T17:07:16Z

src/content/docs/en/reference/api-reference.mdx

@@ -1217,28 +1221,35 @@ export async function getStaticPaths({ paginate }) {
 const { page } = Astro.props;
 ```

+`paginate()` has the following arguments:
+- `data` - Array of data to paginate
+- `options` - Optional object with following properties:


Suggested change

- `options` - Optional object with following properties:

- `options` - Optional object with the following properties:

ManorSailor force-pushed the patch-api-reference-pagination branch from 5f5fb89 to 389bd46 Compare September 25, 2024 15:10

ManorSailor changed the title ~~Patch api reference pagination~~ Improve pagination documentation Sep 25, 2024

sarah11918 reviewed Sep 26, 2024

View reviewed changes

src/content/docs/en/reference/api-reference.mdx Outdated Show resolved Hide resolved

ManorSailor and others added 7 commits September 26, 2024 20:08

fix(api-reference): Misleading description of page.data

dfea63f

feat(api-reference): Add type hints for getStaticPaths return value

ae706c4

fix absolute link

10d8d83

fix(routing): Remove redundant API reference from pagination section

4e96995

fix: standardize TCollectionName for CollectionEntry type

53fb457

Some parts of the docs were using `CollectionEntry<collection>` while others were using `TCollectionName`. This commit cleans things up by sticking to `TCollectionName`, making everything consistent and easier to follow.

ManorSailor force-pushed the patch-api-reference-pagination branch from aadb5dc to 53fb457 Compare September 26, 2024 14:39

ManorSailor marked this pull request as ready for review September 26, 2024 14:45

sarah11918 reviewed Sep 26, 2024

View reviewed changes

suggestion for routing page

12f3c31

ArmandPhilippot reviewed Sep 26, 2024

View reviewed changes

Merge branch 'main' into patch-api-reference-pagination

654b551

ManorSailor added 2 commits September 27, 2024 19:27

fix(routing): Update description of page's data property

7167fc3

fix(api-reference): Rename InferGetStatic* to GetStaticPaths

b694fb6

ManorSailor requested review from sarah11918 and ArmandPhilippot September 27, 2024 16:43

ArmandPhilippot approved these changes Sep 27, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve pagination documentation #9488

Improve pagination documentation #9488

ManorSailor commented Sep 25, 2024 •

edited

Loading

netlify bot commented Sep 25, 2024 •

edited

Loading

astrobot-houston commented Sep 25, 2024 •

edited

Loading

sarah11918 Sep 26, 2024 •

edited

Loading

ManorSailor Sep 26, 2024

sarah11918 Sep 26, 2024

sarah11918 Sep 26, 2024

ArmandPhilippot Sep 26, 2024

ManorSailor Sep 27, 2024

ArmandPhilippot Sep 26, 2024

ManorSailor Sep 27, 2024

ArmandPhilippot Sep 27, 2024

ManorSailor Sep 27, 2024 •

edited

Loading

ArmandPhilippot Sep 27, 2024

sarah11918 commented Sep 27, 2024

ArmandPhilippot left a comment

ArmandPhilippot Sep 27, 2024

ArmandPhilippot Sep 27, 2024 •

edited

Loading

	When using TypeScript, use the [`InferGetStatic*`](/en/guides/typescript/#infer-getstaticpaths-types) type utilities to ensure type-safe access of your `params` and `props`.
	When using TypeScript, use the [`GetStaticPaths`](/en/guides/typescript/#infer-getstaticpaths-types) type utility to ensure type-safe access of your `params` and `props`.

	The following example template displays current information for the page along with links to navigate between pages:
	The following example displays current information for the page along with links to navigate between pages:

	- `options` - Optional object with following properties:
	- `options` - Optional object with the following properties:

Improve pagination documentation #9488

Are you sure you want to change the base?

Improve pagination documentation #9488

Conversation

ManorSailor commented Sep 25, 2024 • edited Loading

Description (required)

Related issues & labels (optional)

netlify bot commented Sep 25, 2024 • edited Loading

✅ Deploy Preview for astro-docs-2 ready!

astrobot-houston commented Sep 25, 2024 • edited Loading

Lunaria Status Overview

Tracked Files

sarah11918 Sep 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ManorSailor Sep 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sarah11918 commented Sep 27, 2024

ArmandPhilippot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ArmandPhilippot Sep 27, 2024 • edited Loading

Choose a reason for hiding this comment

ManorSailor commented Sep 25, 2024 •

edited

Loading

netlify bot commented Sep 25, 2024 •

edited

Loading

astrobot-houston commented Sep 25, 2024 •

edited

Loading

sarah11918 Sep 26, 2024 •

edited

Loading

ManorSailor Sep 27, 2024 •

edited

Loading

ArmandPhilippot Sep 27, 2024 •

edited

Loading