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

Add Data Mutation Guide #994

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

amirhhashemi
Copy link
Contributor

  • I have read the Contribution guide
  • This PR references an issue (except for typos, broken links, or other minor problems)

Description(required)

This is a follow up to #991.

This PR adds a data mutation guide

Related issues & labels

Copy link

stackblitz bot commented Jan 1, 2025

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

Copy link

netlify bot commented Jan 1, 2025

Deploy Preview for solid-docs ready!

Name Link
🔨 Latest commit cffcf21
🔍 Latest deploy log https://app.netlify.com/sites/solid-docs/deploys/677570e6afe8d50008af2f58
😎 Deploy Preview https://deploy-preview-994--solid-docs.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.

@amirhhashemi amirhhashemi mentioned this pull request Jan 1, 2025
2 tasks
Copy link

netlify bot commented Jan 1, 2025

Deploy Preview for solid-docs ready!

Name Link
🔨 Latest commit 29db057
🔍 Latest deploy log https://app.netlify.com/sites/solid-docs/deploys/678004d167ebb20008032b4a
😎 Deploy Preview https://deploy-preview-994--solid-docs.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.

@amirhhashemi amirhhashemi force-pushed the add-data-mutation-guide branch from e51f764 to 9ad48e6 Compare January 1, 2025 17:08
Comment on lines 44 to 62
const addPost = action(async (title: string) => {
await fetch("https://my-api.com/posts", {
method: "POST",
body: JSON.stringify({ title }),
});
}, "addPost");

export default function Page() {
const [title, setTitle] = createSignal("");
return (
<form action={addPost.with(title())} method="post">
<input value={title()} onChange={(e) => setTitle(e.target.value)} />
<button>Add Post</button>
</form>
);
}
```

Note that the action takes a `string` as its parameter rather than a `FormData`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const addPost = action(async (title: string) => {
await fetch("https://my-api.com/posts", {
method: "POST",
body: JSON.stringify({ title }),
});
}, "addPost");
export default function Page() {
const [title, setTitle] = createSignal("");
return (
<form action={addPost.with(title())} method="post">
<input value={title()} onChange={(e) => setTitle(e.target.value)} />
<button>Add Post</button>
</form>
);
}
```
Note that the action takes a `string` as its parameter rather than a `FormData`.
const addPost = action(async (title: string, formData: FormData) => {
await fetch("https://my-api.com/posts", {
method: "POST",
body: JSON.stringify({ title }),
});
}, "addPost");
export default function Page() {
const [title, setTitle] = createSignal("");
return (
<form action={addPost.with(title())} method="post">
<input value={title()} onChange={(e) => setTitle(e.target.value)} />
<button>Add Post</button>
</form>
);
}

Note that the action now takes a string as its first parameter and the FormData as its second.

Copy link
Contributor Author

@amirhhashemi amirhhashemi Jan 2, 2025

Choose a reason for hiding this comment

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

I tried not to go into details of APIs in this guide. I believe that this content is better suited for the API Reference.

Copy link
Contributor

@devagrawal09 devagrawal09 Jan 8, 2025

Choose a reason for hiding this comment

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

I understand, but saying

the action takes a string as its parameter rather than a FormData

just felt a little misleading, it gives off the impression that the formdata cannot be used anymore if you switch to the .with form.

Copy link
Contributor Author

@amirhhashemi amirhhashemi Jan 8, 2025

Choose a reason for hiding this comment

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

How about this:

Note that the action takes a string as its first parameter rather than a FormData.

This doesn't imply that FormData is not accessible inside the action, and doesn't go too much API details.

I don't want to mention the FormData is passed as the second argument because it's not related to the example.

Copy link
Contributor Author

@amirhhashemi amirhhashemi Jan 8, 2025

Choose a reason for hiding this comment

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

Do you have a practical example of using FormData inside an action when using with()? If there is a common pattern that I'm not aware of, I'm open to update this particular example.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this wording makes more sense to me now, I'm good with it.

Also here's an example that shows one use case with Remix where we would need both arguments

Instead of the hidden input like in this example, we would pass the tweet id using .with, and use the formdata to determine which action was taken.

src/routes/solid-start/guides/actions-and-mutations.mdx Outdated Show resolved Hide resolved
src/routes/solid-start/guides/actions-and-mutations.mdx Outdated Show resolved Hide resolved
return (
<div>
<input value={title()} onInput={(e) => setTitle(e.target.value)} />
<button onClick={() => addPostAction(title())}>Add Post</button>
Copy link
Contributor

Choose a reason for hiding this comment

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

I would recommend adding a callout at the bottom of this section that explains why form actions are preferred over useAction in terms of accessibility and progressive enhancement.

Copy link
Contributor Author

@amirhhashemi amirhhashemi Jan 2, 2025

Choose a reason for hiding this comment

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

This is better suited for a tutorial.

Like the Data Fetching guide, this guide is geared toward people who want to know how to do things, and not why and when.

@amirhhashemi amirhhashemi marked this pull request as ready for review January 8, 2025 05:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Content]: Problems with Data Loading documentation
2 participants