-
-
Notifications
You must be signed in to change notification settings - Fork 32
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 submitPreventDefault to Formless when using forms #80
Add submitPreventDefault to Formless when using forms #80
Conversation
HH.form | ||
[ HE.onSubmit F.submitPreventDefault | ||
] |
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.
Use submitPreventDefault
in the readme example to avoid confusion
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 do this in a follow up PR if you'd like, but we really should switch all forms to be written this way
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 was thinking to do that follow up PR, if that's not a problem
example/readme/Component.purs
Outdated
In formless, if you use a default form you would notice a page refresh when submitting the form. | ||
In the other examples, this is avoided by not using a form element. In this example, we use | ||
a special function F.submitPreventDefault to avoid the page refresh. | ||
|
||
You can download the examples and replace F.submitPreventDefault with | ||
\_ -> F.submit to see the difference. |
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.
The text explaining how submitPreventDefault
changes the behavior of them form and how to modify the example is probably worth reviewing as well.
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 think this can be removed, probably, as you really ought to just use forms this way in general. I think it'd be better to add documentation to submit
that says it can be used to submit the form imperatively, but that if you want to capture the form submission event and submit your form that you should use submitPreventDefault
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.
We introduced submitPreventDefault
to avoid the breaking change. Maybe it could be changed to work the other way around in a future major release?
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.
Yea, I’d rather have ‘submit’ and ‘submit_’ using the usual Halogen naming conventions in the future!
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.
Thanks, @chiroptical! This is looking good, just have a few comments.
Co-authored-by: Thomas Honeyman <admin@thomashoneyman.com>
What does this pull request do?
Closes #79. In the examples, this is handled by not using form but many people will reach for what they are familiar with. It is quite confusing when you run into this especially as a beginner to frontend development. Therefore we suggest the addition of
F.submitPreventDefault
and make the appropriate changes to the readme to avoid this confusion altogether. Additionally, we add the example code in the readme to the examples directory.Where should the reviewer start?
I added some comments on where to start your review 😄
How should this be manually tested?
You can bundle the examples with
spago -x example/example.dhall bundle-app --to ./dist/app.js
to launch the "readme" example and see the behavior. You can also modify theonSubmit \_ -> F.submit
to see that the page refreshes.Other Notes:
It might be useful to modify the existing examples to now use form elements and the
F.submitPreventDefault
function.