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

Form component #409

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

Form component #409

wants to merge 2 commits into from

Conversation

SarguelUnda
Copy link
Contributor

Summary

This PR does 2 things:

  • better method detection for action. If the formmethod attribute is present, use it instead of the form element.
  • add a Form component that could be compared to the A element i.e. does spa routing instead of native browser behaviors

Consideration

To achieve this we use the same trick as action which is to globally register the form element in a Set. We register it by ref but we could consider to use solidjs createUniqueId if we prefere to use string instead of dom node.

Example usage

Example app: https://github.com/SarguelUnda/solidrouter-pr-form

import { Form, action } from '@solidjs/router';

const myAction = action(async (data: FormData) => {
  console.log("ACTION")
});


const MyForm = () => {
  return <Form action={myAction} method="post">
    <input name="foo" />
    <input name="bar" />
    <button type="submit">POST ME</button>
    <button name="getsubmit" type="submit" formMethod='get' formAction="newr?azeizaei=1" >GET ME</button>
  </Form>
}

Copy link

changeset-bot bot commented Apr 24, 2024

⚠️ No Changeset found

Latest commit: 54093ae

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@SarguelUnda
Copy link
Contributor Author

Other points that might warrant discussion around this PR. The answer I'm giving are my own as a starter of conversation and any feedback on them is welcome

question: Why using a component and not extending the action api ?
answer: We could extend the action api to register the form as a routable get form if instead of getting an async function it get a relative url as a string for instance. I didn't go toward this design mainly to not change an existing api, because I was not sure of all the implications. It also fell less natural to me on the userside of things but this is highly debatable. I can rewrite this pr if the other design is preferable

import { Form, action } from '@solidjs/router';

const myAction = action(async (data: FormData) => {
  console.log("ACTION")
});


const MyForm = () => {
  return <Form action={myAction} method="post">
    <input name="foo" />
    <input name="bar" />
    <button type="submit">POST ME</button>
    <button name="getsubmit" type="submit" formMethod='get' formAction="newroute" >GET ME</button>
  </Form>
}

// vs 

const MyForm = () => {
  return <form action={myAction} method="post">
    <input name="foo" />
    <input name="bar" />
    <button type="submit">POST ME</button>
    <button name="getsubmit" type="submit" formMethod='get' formAction={action("newroute")} >GET ME</button>
  </form>
}

question: Why the component is inside its own file and not with the other components ?
answer: Data api as I understand is quite optional so I didn't want to write it within the global scope of the package.

@SarguelUnda
Copy link
Contributor Author

Hello,

Any advice how to bring this proposition forward ?

  • Is this component better on the App side than on the library side ?
  • Does this design conflict with futures direction of the solid router / data api ?
  • Is it useful if I also propose the alternative without a component but by extending the action api to compare solutions ?
  • Does some consideration about ssr / solid start escape me ? I only use solid in spa context.

@ryansolid
Copy link
Member

I admit I missed the point of suggestion. Hard to tell what the value is. There was a discussion about this issue in Discord but I can't seem to find it now. My understanding if I remember was that this would help do client side navigation similar to a Anchor tag by supporting GET method. But then I don't quite understand how actions play into it.

Basically is the target the client routed path or the action.. if it is the action then we can support GET encoding without a capital Form Component in the future. If it is the client routed path then actions don't fit exactly. I guess in a sense the client Router views itself as the GET for all routes it defines so only POST would make sense.

As I said I might need a much clearer explanation step by step about how this would behave.

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.

2 participants