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

Inaccurate Page interface for Pagination in Routing guide #8929

Closed
ArmandPhilippot opened this issue Jul 26, 2024 · 6 comments · Fixed by #8941
Closed

Inaccurate Page interface for Pagination in Routing guide #8929

ArmandPhilippot opened this issue Jul 26, 2024 · 6 comments · Fixed by #8941
Labels
help wanted Issues looking for someone to run with them! improve documentation Enhance existing documentation (e.g. add an example, improve description)

Comments

@ArmandPhilippot
Copy link
Contributor

📚 Subject area/topic

Routing/Pagination

📋 Page(s) affected (or suggested, for new content)

https://docs.astro.build/en/guides/routing/
https://docs.astro.build/en/reference/api-reference/#paginate

📋 Description of content that is out-of-date or incorrect

The Routing guide displays a complete API reference for the page prop related to Pagination. This reference is no longer accurate since withastro/astro#11176. The documentation has been updated in API reference (see #8751) but not in guides/routing.mdx.

There is another mistake I think. The default for page.size seems to be 10 and not 25 (see paginate.ts#L19 and paginate.ts#L80). It seems this error comes from astro/@types comment. I created a Stackblitz reproduction for this. I'll make an update there as well.

I could update the Routing guide but I wonder if this section is relevant. Since we already have the interface in API Reference, it seems duplicated (and prone to errors when new features are added). I think a link to the API Reference could make more sense:

-#### Complete API reference
-
-```ts
-interface Page<T = any> {
-	/** result */
-	data: T[];
-	/** metadata */
-	/** the count of the first item on the page, starting from 0 */
-	start: number;
-	/** the count of the last item on the page, starting from 0 */
-	end: number;
-	/** total number of results */
-	total: number;
-	/** the current page number, starting from 1 */
-	currentPage: number;
-	/** number of items per page (default: 25) */
-	size: number;
-	/** number of last page */
-	lastPage: number;
-	url: {
-		/** url of the current page */
-		current: string;
-		/** url of the previous page (if there is one) */
-		prev: string | undefined;
-		/** url of the next page (if there is one) */
-		next: string | undefined;
-	};
-}
-```
+<ReadMore>Learn more about [`page` prop](https://docs.astro.build/en/reference/api-reference/#the-pagination-page-prop).</ReadMore>

And we could add the default for pageSize/page.size to the API Reference page.

What do you think?

🖥️ Reproduction in StackBlitz (if reporting incorrect content or code samples)

For the page.size issue only: https://stackblitz.com/edit/astro-paginate-pagesize?file=src%2Fpages%2Fpokemon%2F[page].astro&on=stackblitz

@ArmandPhilippot ArmandPhilippot added the improve documentation Enhance existing documentation (e.g. add an example, improve description) label Jul 26, 2024
@ArmandPhilippot ArmandPhilippot changed the title Inacurrate Page interface for Pagination in Routing guide Inaccurate Page interface for Pagination in Routing guide Jul 26, 2024
@ArmandPhilippot
Copy link
Contributor Author

ArmandPhilippot commented Jul 26, 2024

The issue on astro/@types has been confirmed (see withastro/astro#11560) and the PR have been merged (withastro/astro#11561).

@sarah11918
Copy link
Member

Great catch, @ArmandPhilippot and I totally agree that there is duplication to show the full API reference here if it's in on the reference page. However, I did notice that while we list out all the properties in the API reference, we don't show it all in one nice example like we do here.

Deleting it from the routing page and linking to the full API reference instead makes sense. If we did that, what do you think about reproducing this block on the API reference page instead, just before listing out each property? Is that too much? Is it helpful to be able to see everything at once, or is it unnecessary?

We also currently do not link back to the API reference page at all, and I think we should! So my thought is yes, delete the "Full API reference" section and instead include a <ReadMore> component sending us there for the full API reference / list of properties available. Would you like to propose something for the Routing page?

(I also noticed that on the API reference page, page.size does not specify its default, so maybe there is some other content from the in-code documentation that would be helpful to include on the API page in each entry itself?

@ArmandPhilippot
Copy link
Contributor Author

Deleting it from the routing page and linking to the full API reference instead makes sense. If we did that, what do you think about reproducing this block on the API reference page instead, just before listing out each property? Is that too much? Is it helpful to be able to see everything at once, or is it unnecessary?

I think it's a matter of taste. From a personal point of view, I would say that it is a duplicate but some might find this useful. Since the goal of a documentation is to be useful to as many people as possible, this addition makes sense to me. Those who are not interested can easily scroll down the page.

However, adding this block could harm the coherence of the page. Other types do not show the full interface before going into detail about each property. For example:

So if we add the full interface, maybe we should do it for other types.

Would you like to propose something for the Routing page?

Of course, I can take care of it!

I also noticed that on the API reference page, page.size does not specify its default, so maybe there is some other content from the in-code documentation that would be helpful to include on the API page in each entry itself?

I will look the types if find some other helpful information to add!

@sarah11918
Copy link
Member

So if we add the full interface, maybe we should do it for other types.

OK, so maybe this is reason enough not to do it! 😅

Here's my current thinking:

Routing page:

  • KEEP the big block where it is, update it to include everything it was missing
  • ADD a <ReadMore> component linking to the full API reference

API reference page: (can be handled separately, doesn't have to be the same PR)

  • ADD any helpful defaults etc. to the properties that are listed out there that you find in the types that you think are useful for people to have

How does that sound?

@sarah11918 sarah11918 added the help wanted Issues looking for someone to run with them! label Jul 29, 2024
@ArmandPhilippot
Copy link
Contributor Author

@sarah11918 This looks good to me. And yes, I will make two different PRs because it's probably better for translators if the page is only updated once 😄 and I'd like to check for missing Since component (see #6467) in api-reference.mdx while I'm here. Is there a reason for not adding it for page.url.first and page.url.last or is this just an oversight?

@sarah11918
Copy link
Member

I'm sure the missing <Since /> is an oversight! Please do add!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Issues looking for someone to run with them! improve documentation Enhance existing documentation (e.g. add an example, improve description)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants