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(api-reference): add missing info, types and <Since /> #8943

Conversation

ArmandPhilippot
Copy link
Contributor

This PR is a follow-up to my discussion with @sarah11918 in #8929.

Description (required)

I updated api-reference.mdx to add missing (or to update) information like:

  • <Since /> component,
  • defaults,
  • types for individual properties.

For this task, I tried to keep track of my references in each commit (with URLs) so that you can verify the accuracy of the information (this is why this PR has so many commits, I found it was easier to divide the file in parts).

I left out some types that seemed too complex to understand by reading the documentation. Also, I modified a type but I'm not sure its definition is clear from reading the documentation. I'll add a review comment.

If I haven't added a <Since /> component it's either because I haven't found when the property/component was added or because it dates from v1 or earlier.

Additionally, I noticed some missing sections:

  • all the properties of Astro.cookies was described except merge, I added it
  • getDataEntryById is missing but getEntryBySlug is marked as deprecated in the documentation (I couldn't find a reference about this) so I don't know if we should add it (I guess it is also deprecated)

Finally, I removed:

Related issues & labels (optional)

Documentation "coding" style related

While translating the doc in French and while updating this file, I noticed that some parts of the files was inconsistent, more exactly when we have <Since /> and/or **Types:**:

  • Sometimes <Since /> OR **Types:** are wrapped in <p>...</p> tags, other times it is not the case
  • When <Since /> AND **Types:** are both provided, sometimes they are in a block wrapped with <p>...</p>, other times they are separated (often we have a <Since /> wrapped in <p>...</p> tags followed by or put after **Types:** and separated by a new line)

While updating this file I wasn't sure what to do. In Astro Docs Docs, we have an example but I wasn't sure if it was a guideline or just an example.

So if you have any guidance about the formatting, I'll be happy to update this PR to make the documentation more consistent (at least in this file)! 😄

* Add default for `page.size` and `pageSize` (see withastro/astro#11561)
* Add missing Since for `page.url.first` and `page.url.last` (see withastro/astro#11176)
* Add missing properties (`rawContent`, `compiledContent` and
`default`)
* Fix inaccurate types for Astro.cookies `get` and `set` methods
* Add missing `merge` method for Astro.cookies (the `AstroCookies` return type
could miss some documentation)
* Fix inaccurate type for AstroCookie `value`

I did not updated the name of the options types, but all of them should
be prefixed with `Astro`. I don't know if this is deliberate or an oversight
during an update.

See: `packages/astro/src/core/cookies/cookies.ts`
It was deprecated but it has been removed in v2.0.0 (see withastro/astro#5707). I
think it should not be displayed in the reference anymore since we are
now at v4.12.1
* See withastro/astro#4986 for `site`, `generator`, `url`, `clientAddress`, `props` and `redirect`
* See withastro/astro#6721 for `locals`
* See withastro/astro#9021 for `preferredLocale` and `preferredLocaleList`
* See withastro/astro#9101 for `currentLocale`
* See withastro/astro#7607 for the most recent
exported types
* See withastro/astro#6850 for `reference()`
* See `packages/astro/types/content.d.ts` for `schema` type (might
need a refactor)
* `getDataEntryById` is missing but since `getEntryBySlug` is
announced as deprecated I don't know if we should add it (so
no change here)
* Add missing Since (see withastro/astro#7578 for
`createContext` and `trySerializeLocals`
* Add missing types (see `packages/astro/src/core/build/types.ts` and
`packages/astro/src/@types/astro.ts`)
* Make Types in Internationalization section consistent with other
declared functions types
Copy link

netlify bot commented Jul 29, 2024

Deploy Preview for astro-docs-2 ready!

Name Link
🔨 Latest commit f6b167f
🔍 Latest deploy log https://app.netlify.com/sites/astro-docs-2/deploys/66ace305a366a7000864416f
😎 Deploy Preview https://deploy-preview-8943--astro-docs-2.netlify.app
📱 Preview on mobile
Toggle QR Code...

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
Copy link
Contributor

astrobot-houston commented Jul 29, 2024

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 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.

@sarah11918 sarah11918 added improve documentation Enhance existing documentation (e.g. add an example, improve description) consistency/formatting Standardizing without changing docs content e.g. indenting, lists etc. labels Jul 30, 2024
@sarah11918
Copy link
Member

Thank you @ArmandPhilippot , this looks great! I'm going to ask @delucis to review this one!

Copy link
Member

@delucis delucis left a comment

Choose a reason for hiding this comment

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

Wow, thank you @ArmandPhilippot for this amazingly thorough work! Love to see stuff like the default pagination values specified — really helpful 💜

So if you have any guidance about the formatting, I'll be happy to update this PR to make the documentation more consistent (at least in this file)! 😄

The examples in Astro Docs Docs are correct! Ideally, when there are multiple short items like Type + <Since>, these should be grouped together rather than in separate paragraphs.

You can see this style in use in our configuration reference which is generated automatically from code:

Reference header reading “redirects” followed by type, default, and added in information grouped together visually

getDataEntryById is missing but getEntryBySlug is marked as deprecated in the documentation (I couldn't find a reference about this) so I don't know if we should add it (I guess it is also deprecated)

Yes, if I’m not mistaken, they are both deprecated in favour of getEntry().

src/content/docs/en/reference/api-reference.mdx Outdated Show resolved Hide resolved
src/content/docs/en/reference/api-reference.mdx Outdated Show resolved Hide resolved
src/content/docs/en/reference/api-reference.mdx Outdated Show resolved Hide resolved
src/content/docs/en/reference/api-reference.mdx Outdated Show resolved Hide resolved
src/content/docs/en/reference/api-reference.mdx Outdated Show resolved Hide resolved
ArmandPhilippot and others added 8 commits August 1, 2024 15:17
Co-authored-by: Chris Swithinbank <swithinbank@gmail.com>
Co-authored-by: Chris Swithinbank <swithinbank@gmail.com>
* Add "Markdown only" mention to `rawContent` and `compiledContent`
* Reword the introductory line to index `MarkdownInstance` (see withastro#8943)

Co-authored-by: Chris Swithinbank <swithinbank@gmail.com>
* Replace `CookieGetOptions` with `AstroCookieGetOptions`
* Replace `CookieSetOptions` with `AstroCookieSetOptions`
* Replace `CookieDeleteOptions` with `AstroCookieDeleteOptions`
* Add links to documented types inside `Type:` information
From Astro Docs Docs, Types/Default/Since information should be in a
single block  (see https://contribute.docs.astro.build/reference/custom-components/#examples-1)
I wrapped single information (e.g. Types) with a `<p>` tag to keep the
block consistent and to help future contributors understand that the
format must be the same if they add information.
Also, I added an empty line after the opening `<p>` tag because of
a possible parsing issue. See: 35d1f4b
@delucis
Copy link
Member

delucis commented Aug 1, 2024

Re: 35d1f4b the newline made sure subsequent markup is treated as Markdown IIRC. In other words the following doesn’t work:

<p>
**Type:** `undefined`
</p>

It renders the children as HTML directly:

<p>
**Type:** `undefined`
</p>

But the new line lets the Markdown parser take over again:

<p>

**Type:** `undefined`
</p>

Renders:

<p>
<strong>Type:</strong> <code>undefined</code>
</p>

Even if the method is deprecated, shouldn't it be added?

Yes, probably. I asked the team about the status of getDataEntryById() — let’s see what they say!

@delucis
Copy link
Member

delucis commented Aug 1, 2024

Update on getDataEntryById() — yes, it should be documented as deprecated! And I guess we should probably also add @deprecated in the core repo.

* Add documentation about the deprecated getDataEntryById function
* Fix return type for getDataEntryBySlug (this is a Promise)
We cannot use `ul` inside `p` tag so we need to make an exception when
using Since and Type with overloads and separate them.
Copy link
Member

@delucis delucis left a comment

Choose a reason for hiding this comment

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

Spotted a couple more things reading through the page!

src/content/docs/en/reference/api-reference.mdx Outdated Show resolved Hide resolved
src/content/docs/en/reference/api-reference.mdx Outdated Show resolved Hide resolved
src/content/docs/en/reference/api-reference.mdx Outdated Show resolved Hide resolved
ArmandPhilippot and others added 4 commits August 2, 2024 01:04
Only the argument was added in `4.13.0` so it doesn't make sense to keep it under `next()` heading.

Co-authored-by: Chris Swithinbank <swithinbank@gmail.com>
Co-authored-by: Chris Swithinbank <swithinbank@gmail.com>
* Move `next()` as subheading of `onRequest()` because it is an
argument of that function
* Add a `context` subheading to describe the first argument
* Update `context` link to redirect to the new `context` section
* Add the `getImage` function signature
* List every properties returned by the function
@ArmandPhilippot
Copy link
Contributor Author

I think the previous points are all solved!

While I was rereading the page, I noticed that the getImage section could be improved. Currently the getImage section in API reference is close to a copy-paste of the section Generating images with getImage() in the Images guide.

So I added some changes to differentiate a little the two sections:

  • The function signature is now added in API reference (I had hesitated before because of the internal function which has a slightly different signature)
  • The returned properties was incomplete, so I completed them
  • I thought it might be useful to have the return type here instead of an object, so I updated the code block

I wonder if a link from Generating images with getImage() (Images guide) to getImage() (API reference) might be helpful. Something like:

<ReadMore>Learn more about [the `getImage()` return type](/en/reference/api-reference/#getimage).</ReadMore>

But we have almost the same information...

@delucis
Copy link
Member

delucis commented Aug 2, 2024

Nicely spotted! Yes, I’d even say now that once we merge this we could also clean up the “Generating images…” section to be more focused on the practical usage part with a link to reference for the full type details.

Similarly the image component docs feel a lot like they should mostly be in reference for each prop. Maybe image docs improvement is a whole other rabbit hole best tackled in a separate PR 😁

@sarah11918
Copy link
Member

Looking great, amazing work here! I trust Chris to make the final approval here, and will only say that I agree on scoping this PR to the api-reference page only! If you start looking on other pages, I'm sure you'll find more improvements you want to make, so let's make life easier on the translators and tackle these kinds of improvements one page at a time! 😄

Copy link
Member

@delucis delucis left a comment

Choose a reason for hiding this comment

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

Woo, I think we’re pretty much there — amazing work @ArmandPhilippot! I left a couple of copy suggestions not strictly related to your changes, but thought we might as well get them in while we’re in this file.

src/content/docs/en/reference/api-reference.mdx Outdated Show resolved Hide resolved
src/content/docs/en/reference/api-reference.mdx Outdated Show resolved Hide resolved
@sarah11918
Copy link
Member

Also pointing out that there's a draft API reference for Actions in #8980 , so feel free to hop into that PR and get that API reference entry up to speed from the beginning!

ArmandPhilippot and others added 2 commits August 2, 2024 13:51
The arguments are always available. It is up to the consumer to use them or not.

Co-authored-by: Chris Swithinbank <swithinbank@gmail.com>
* Aligns phrasing with the new `context` subsection of `onRequest`
* Makes the use of `next()` function clearer

Co-authored-by: Chris Swithinbank <swithinbank@gmail.com>
@ArmandPhilippot
Copy link
Contributor Author

Thanks @delucis, the onRequest section seems clearer with your two suggestions! (and, while I'm at it, thanks for all your suggestions ✨ )

I think we've covered this file. There are some missing types but adding them might be more harmful than helpful since they rely on Typescript generics:

  • Astro.glob() and its overloads
  • props and params in Astro global, context and getStaticPath

So unless you see any other improvements, I think we're good this time! 🎉

Copy link
Member

@delucis delucis left a comment

Choose a reason for hiding this comment

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

Agreed! Ready to merge — thanks again for the thorough and persistent work here @ArmandPhilippot 🙌

@delucis delucis added the Merge Queue Approved and ready to be merged (wait for feature release if also labelled M-O-R)! label Aug 2, 2024
@delucis delucis merged commit 82b7283 into withastro:main Aug 2, 2024
10 checks passed
@ArmandPhilippot ArmandPhilippot deleted the fix/api-reference-add-missing-info-types-since branch August 2, 2024 14:10
@sarah11918 sarah11918 added the feedback-improvement Response to widget/Discord feedback label Aug 14, 2024
This was referenced Aug 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
consistency/formatting Standardizing without changing docs content e.g. indenting, lists etc. feedback-improvement Response to widget/Discord feedback improve documentation Enhance existing documentation (e.g. add an example, improve description) Merge Queue Approved and ready to be merged (wait for feature release if also labelled M-O-R)!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants