-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Fred's edit pass on content collections guide #2371
Conversation
✅ Deploy Preview for astro-docs-2 ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
2648856
to
38dc0bc
Compare
Just a reminder that we need to incorporate withastro/astro#5893 here, because |
Note: I added a necessary corresponding change to a link reference from I know this PR might not be in its final form, but at least now the other page that will need changes is here, and visible for editing. (We are also going to have to update this page by renaming |
Current status:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@FredKSchott I have done some tidying, and left some higher level comments here. Mostly, this is great! There are a couple of things I feel are worth mentioning, to make sure we're intentionally choosing against some things that were here before. Otherwise, this is a great revamp in general, and not too many nits! 🥳
6d90361
to
3364c00
Compare
@sarah11918 second pass complete! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fred!
Here are my review notes for your consideration. You're of course free to ingore any and all of them. But, these are the comments that come to mind that I'd at least bring to your attention. Some might seem like style preferences, but I've tried to only do that when I think I can make things tighter.
So, here ya go!
P.S. I have not dealt with failing links/conflicts.
|
||
// Get all `src/content/blog/` entries | ||
const allBlogPosts = await getCollection('blog'); | ||
## Querying Collections |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From this section, to Generating Routes, I'm not sure the heading levels/right sidebar organization is quite right.
Content-wise, Rendering Content doesn't seem like it should be a ##
as it seems more a peer with "Using Content in Astro Templates". But, I'm not sure I'd throw everything under the title of "Querying Collections"? And, they alternate between "script stuff" and "templating stuff".
These related items seem to be:
Script - Making the queries/filtering
Template- Templating the metadata (but not using render()
)
Script - Passing an entire Content as props
Template - Rendering the content itself with render()
Script/Template of modified metadata
I think these could all be ###
peers under one ##
? (Which we'd need a name for.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I entirely follow this, but I agree this feels a little incorrectly placed. Just reorganized it to be a party of "querying", lmk what you think.
@sarah11918 thanks for the amazing review! Made some great improvements based on your feedback. It sounds like you're either "LGTM" or close to it, so I'll leave this open until Monday or until I see your approval come through. I'm feeling pretty good about where this is now, but don't hesitate to give this another review if you have the time and feel the need. |
Just a quick reminder that merging this will remove the experimental note, so we might want to wait until Tuesday anyway! We have a "merge.on.release" label we can add for extra safety against forgetting. I'm sure it will be great, and I'll just find time to scan for typos etc. just to be safe. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, a small handful of corrections that must be changed (typos etc.) and a few more things for your consideration!
I am now confident that whatever you choose to make of this, it'll be great! 🥳
To get the most out of content collections in Astro, you may need to update your `tsconfig.json` to enable `strictNullChecks`. This change is only required if you **do not** already extend Astro's `strict` or `strictest` recommended TypeScript settings in your `tsconfig.json` file. | ||
|
||
```json title="tsconfig.json" ins={5} | ||
{ | ||
// Note: No change needed if you use "astro/tsconfigs/strict" or "astro/tsconfigs/strictest" | ||
"extends": "astro/tsconfigs/base", | ||
"compilerOptions": { | ||
"strictNullChecks": true | ||
} | ||
} | ||
``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To get the most out of content collections in Astro, you may need to update your `tsconfig.json` to enable `strictNullChecks`. This change is only required if you **do not** already extend Astro's `strict` or `strictest` recommended TypeScript settings in your `tsconfig.json` file. | |
```json title="tsconfig.json" ins={5} | |
{ | |
// Note: No change needed if you use "astro/tsconfigs/strict" or "astro/tsconfigs/strictest" | |
"extends": "astro/tsconfigs/base", | |
"compilerOptions": { | |
"strictNullChecks": true | |
} | |
} | |
``` | |
To get the most out of content collections in Astro, you may need to update your `tsconfig.json` to enable `strictNullChecks`. This change is only required if currently extend Astro's `base` TypeScript settings in your `tsconfig.json` file. | |
```json title="tsconfig.json" ins={5} "base" | |
{ | |
// Note: No change needed if you use "strict" or "strictest" | |
"extends": "astro/tsconfigs/base", | |
"compilerOptions": { | |
"strictNullChecks": true | |
} | |
} | |
``` |
I think we can tighten this up by avoiding the negative framing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is no longer true though. There are situations where you can be extending something other than base (or not extending anything, and defining everything yourself) that you still need to make this change.
``` | ||
|
||
Assuming `readingTime` was injected ([see our reading time example](/en/guides/markdown-content/#example-calculate-reading-time)), it will be available on the `remarkPluginFrontmatter` object. | ||
## Modifying Frontmatter with Remark |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an appropriate use of the caution! Something we don't actually want anyone to do! 😄
Co-authored-by: Sarah Rainsberger <[email protected]>
Co-authored-by: Sarah Rainsberger <[email protected]>
Co-authored-by: Sarah Rainsberger <[email protected]>
Co-authored-by: Sarah Rainsberger <[email protected]>
Co-authored-by: Sarah Rainsberger <[email protected]>
Co-authored-by: Sarah Rainsberger <[email protected]>
Co-authored-by: Sarah Rainsberger <[email protected]>
I'm going to merge this sooner actually, because I realized that the current docs have gotten out of date with the API. There will likely be one more PR to this page before release, so that will give us a chance for any final review before Tuesday's launch. |
Looks like this is what it was previous to withastro#2371. Filtering to non-drafts makes sense too, but currently either the variable name (and the copy above) or the filter should change for consistency.
* update translation #1442 * update translation #1434 * update translation #1510 * update translation #1538 * Update src/content/docs/de/basics/astro-pages.mdx * Update src/content/docs/de/basics/astro-pages.mdx * Update src/content/docs/de/basics/astro-pages.mdx * Update src/content/docs/de/basics/astro-pages.mdx * Update src/content/docs/de/basics/astro-pages.mdx * update translation #1638 * update translation #2091 * update translation #2133 * update translation #2409 * update translation #2371 * update translation #4610 * update translation #5128 * update translation #5205 * update translation #5240 * update translation #5364 * update translation #5765 * update translation #6267 * remove paragraph where I cant find when it was deleted in original version * update translation #6620 * update translation #8495 * update translation #8573 * update translation #9336 * update translation #9336 2/2 didnt save file locally, so git didnt add * fix all visual differences by comparing manually against original english version * fix broken links in `astro-pages.mdx` * Breaking changes to other files! fixing links which link to the �stro-pages.mdx file * but now... * Update src/content/docs/de/basics/astro-pages.mdx Co-authored-by: Max <[email protected]> * Update src/content/docs/de/basics/astro-pages.mdx Co-authored-by: Max <[email protected]> * Update src/content/docs/de/basics/astro-pages.mdx Co-authored-by: Max <[email protected]> * revert Update markdown-content.mdx @lunaria-track:src/content/docs/de/basics/astro-pages.mdx --------- Co-authored-by: Max <[email protected]> Co-authored-by: Yan <[email protected]>
What kind of changes does this PR include?
Description
As a part of my v2.0 readiness docs pass, I took a look at the content collections guide and had some suggestions for how to improve the guide. My edits focused on three main areas:
TODO
getEntry()
SSR issue