-
-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Integration Docs Next Steps #3677
Conversation
This reverts commit cc55b31.
🦋 Changeset detectedLatest commit: 077baaa The changes in this PR will be included in the next version bump. This PR includes changesets to release 7 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Thanks, Dan! You could choose to just link to /en/guides/deploy on both Netlify/Vercel for now, and we update when those links are live. |
I want to propose coordinating the two to be merged at once; this way we can add a Deno and Node deployment guide that "matches" these READMEs. |
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.
lgtm
A new Sitemap release occurred in the meantime; I need to revisit and update that README. The merge conflicts are due to that. |
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.
Amazing work Dan! 🙌
I went through everything bar the sitemap changes as you said that’s likely to change again. Most of my comments are fairly small fry — you’ve done an amazing job bringing these all in line with each other.
One generic questions: several of the READMEs now have blank sections (e.g. Troubleshooting, Changelog, Examples). Is that intentional as a placeholder for the future or should they be removed?
packages/integrations/deno/readme.md
Outdated
|
||
To configure this adapter, pass an object to the `deno()` function call in `astro.config.mjs`. |
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 can see what you’re doing by being explicit here, but might this be clearer? “function call” sounds like something special to me but I’m not sure, so happy to leave it as is too.
To configure this adapter, pass an object to the `deno()` function call in `astro.config.mjs`. | |
To configure this adapter, pass an object to the `deno()` adapter in `astro.config.mjs`. |
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.
Maybe slightly more clear?
To configure this adapter, pass an object to the `deno()` function call in `astro.config.mjs`. | |
To configure this adapter, pass an object as the first argument to the `deno()` function in `astro.config.mjs`. |
Co-authored-by: Chris Swithinbank <swithinbank@gmail.com>
These look great, Dan! Chris' suggestions are great, too! (Though, the spaces---which I personally like---do not need to be added.) |
FYI @Jutanium : all the individual deploy pages (e.g. https://docs.astro.build/en/deploy/netlify/ ) are now LIVE! 🥳 Link away! |
Co-authored-by: Chris Swithinbank <swithinbank@gmail.com>
Co-authored-by: Chris Swithinbank <swithinbank@gmail.com>
Co-authored-by: Chris Swithinbank <swithinbank@gmail.com>
…into integration-docs
Co-authored-by: Chris Swithinbank <swithinbank@gmail.com>
Co-authored-by: Chris Swithinbank <swithinbank@gmail.com>
LGTM — thanks for all this work Dan! |
OK GitHub is bugging out and closing PRs when I try to comment… Sorry about that |
This applies the format in #3429 to the other integration docs. This moves existing information into newly defined sections and also includes updated information based on my learnings.
Note that framework integration READMEs are unchanged in this PR.
I've been sitting on this for too long and figured it'd be best to get this out there, and I'll be around to work with community experts on reworking those framework integration READMEs!
The Netlify and Vercel READMEs link to the new deployment guides; if we merge this PR immediately, these links will be broken. It may also be worth duplicating the Usage content between these READMEs and those guides, and making sure there's a deployment guide for every adapter.
Thanks to conversations with @bholmesdev, we decided to remove the canonicalURL option from both the README and the codebase of
sitemap
.