Skip to content

Improve Clarity of the Build forms with API Recipe#5899

Merged
sarah11918 merged 11 commits intowithastro:mainfrom
VoxelMC:improve-build-forms-api
Dec 24, 2023
Merged

Improve Clarity of the Build forms with API Recipe#5899
sarah11918 merged 11 commits intowithastro:mainfrom
VoxelMC:improve-build-forms-api

Conversation

@VoxelMC
Copy link
Copy Markdown
Contributor

@VoxelMC VoxelMC commented Dec 23, 2023 •
edited
Loading

Description (required)

  • Reordered recipe steps so that you don't jump back and forth between files.
  • Added diffs to the code examples to make the changes more obvious (the code is long and changes didn't jump out before)
  • Added a step to make sure readers don't forget to use a client:* directive to make their form submission work properly.

Affected page: Modified

Related issues & labels (optional)

@vercel
Copy link
Copy Markdown

vercel bot commented Dec 23, 2023 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
docs ✅ Ready (Inspect) Visit Preview Dec 24, 2023 7:11pm
1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
docs-i18n ⬜️ Ignored (Inspect) Visit Preview Dec 24, 2023 7:11pm

@VoxelMC VoxelMC changed the title Improve Comprehensibility of the Build forms with API Recipe Improve Clarity of the Build forms with API Recipe Dec 23, 2023
@VoxelMC
Copy link
Copy Markdown
Contributor Author

VoxelMC commented Dec 23, 2023

Oh, looks like I did some git wackiness, time to fix that...

@VoxelMC
Copy link
Copy Markdown
Contributor Author

VoxelMC commented Dec 23, 2023

Oh, looks like I did some git wackiness, time to fix that...

✅ 😅

@sarah11918 sarah11918 added the improve or update documentation Enhance / update existing documentation (e.g. add example, improve description, update for changes) label Dec 23, 2023
sarah11918
sarah11918 reviewed Dec 23, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@sarah11918 sarah11918 left a comment

Choose a reason for hiding this comment

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

This looks great! Nicely done! 🥳

See what you think about my suggestions below, and also, I couldn't suggest it here because it was too early in the page, but is the requirement here still output: 'server' or would 'hybrid' work, too? This recipe was probably written before hybrid existed, so just want to make sure! (And, if hybrid is OK, then we'd need a note that this page should include export const prerender = false)

@VoxelMC @sarah11918
create it!
2eb5541 
Co-authored-by: Sarah Rainsberger <sarah@rainsberger.ca>
VoxelMC and others added 3 commits December 23, 2023 13:12
@VoxelMC @sarah11918
list-ify it
f5a46b0 
Co-authored-by: Sarah Rainsberger <sarah@rainsberger.ca>
@VoxelMC @sarah11918
chill out a bit
e355982 
Co-authored-by: Sarah Rainsberger <sarah@rainsberger.ca>
@VoxelMC @sarah11918
remove stray sentence
12bf7c9 
Co-authored-by: Sarah Rainsberger <sarah@rainsberger.ca>
@VoxelMC
Copy link
Copy Markdown
Contributor Author

VoxelMC commented Dec 23, 2023

This looks great! Nicely done! 🥳

See what you think about my suggestions below, and also, I couldn't suggest it here because it was too early in the page, but is the requirement here still output: 'server' or would 'hybrid' work, too? This recipe was probably written before hybrid existed, so just want to make sure! (And, if hybrid is OK, then we'd need a note that this page should include export const prerender = false)

I will do a test to ensure this works!

@VoxelMC @sarah11918
commit now for preact - the rest follow later
a97c072 
Co-authored-by: Sarah Rainsberger <sarah@rainsberger.ca>
@VoxelMC
Copy link
Copy Markdown
Contributor Author

VoxelMC commented Dec 24, 2023 •
edited
Loading

This looks great! Nicely done! 🥳
See what you think about my suggestions below, and also, I couldn't suggest it here because it was too early in the page, but is the requirement here still output: 'server' or would 'hybrid' work, too? This recipe was probably written before hybrid existed, so just want to make sure! (And, if hybrid is OK, then we'd need a note that this page should include export const prerender = false)

I will do a test to ensure this works!

Reflected in my last commit, output: 'hybrid' does not affect the recipe, and export const prerender = true|false; doesn't either!

VoxelMC
VoxelMC commented Dec 24, 2023
View reviewed changes
src/content/docs/en/recipes/build-forms-api.mdx
Comment on lines +388 to +390
---
import FeedbackForm from "../components/FeedbackForm"
---
Copy link
Copy Markdown
Contributor Author

@VoxelMC VoxelMC Dec 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Suggested change
---
import FeedbackForm from "../components/FeedbackForm"
---
---
export const prerender = false; // if output: 'hybrid'
import FeedbackForm from "../components/FeedbackForm"
---

Copy link
Copy Markdown
Contributor Author

@VoxelMC VoxelMC Dec 24, 2023

Choose a reason for hiding this comment

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

@sarah11918 if preferred, I can add this to the code snippet at the end as well.

Copy link
Copy Markdown
Member

@sarah11918 sarah11918 Dec 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Ah, I guess maybe the page itself doesn't need to be on-demand rendered as long as the API endpoint is. So, in fact, in hybrid mode, shouldn't feedback.ts need export const prerender = false;?

I'm just confused that setting something to prerender false shouldn't be necessary if hybrid is going to prerender by default, unlike server mode. If prerendered everything were OK, then why would server mode necessary in the first place?

Copy link
Copy Markdown
Member

@sarah11918 sarah11918 Dec 24, 2023

Choose a reason for hiding this comment

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

SO, if having the statement doesn't actually make a difference, let's not put it in and... see what happens! 😀

@sarah11918 sarah11918 added the Merge Queue Approved and ready to be merged (wait for feature release if also labelled M-O-R)! label Dec 24, 2023
sarah11918
sarah11918 approved these changes Dec 24, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@sarah11918 sarah11918 left a comment

Choose a reason for hiding this comment

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

It's YOLO Christmas merge, @VoxelMC - my Christmas present to myself. If we need to PR a fix, we need to PR a fix!

Thank you again for being such a great, helpful, active contributor! 🥳

@sarah11918 sarah11918 merged commit de7a349 into withastro:main Dec 24, 2023
@VoxelMC
Copy link
Copy Markdown
Contributor Author

VoxelMC commented Dec 24, 2023 •
edited
Loading

It's YOLO Christmas merge, @VoxelMC - my Christmas present to myself. If we need to PR a fix, we need to PR a fix!

Thank you again for being such a great, helpful, active contributor! 🥳

Merry Christmas!!! I'll fix it after the holidays - if it needs to be :)

I'm always over the moon to contribute, so I'm glad to be of good service! Thank you for being such a hard-working maintainer, and for always giving great and kind feedback!

@100gle 100gle mentioned this pull request Dec 25, 2023
Merged
@McFlyPartages McFlyPartages mentioned this pull request Jan 4, 2024
Merged
yanthomasdev added a commit that referenced this pull request Jan 4, 2024
@McFlyPartages @yanthomasdev
i18n(fr) Update build-forms-api.mdx (#6100)
0ef67dc 
Update French translation for build-forms-api.mdx file with PR #5766 and #5899

Co-authored-by: Yan Thomas <61414485+Yan-Thomas@users.noreply.github.com>
ematipico pushed a commit that referenced this pull request Jan 26, 2024
@VoxelMC @sarah11918 @ematipico
Improve Clarity of the Build forms with API Recipe (#5899)
ecdd804 
Co-authored-by: Sarah Rainsberger <sarah@rainsberger.ca>
ematipico pushed a commit that referenced this pull request Jan 26, 2024
@McFlyPartages @yanthomasdev @ematipico
i18n(fr) Update build-forms-api.mdx (#6100)
c0c322b 
Update French translation for build-forms-api.mdx file with PR #5766 and #5899

Co-authored-by: Yan Thomas <61414485+Yan-Thomas@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sarah11918 sarah11918 sarah11918 approved these changes

Assignees

No one assigned

Labels

improve or update documentation Enhance / update existing documentation (e.g. add example, improve description, update for changes) Merge Queue Approved and ready to be merged (wait for feature release if also labelled M-O-R)!

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Verify Code in Build Forms recipe

2 participants

@VoxelMC @sarah11918