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 onSubmit to form #589

Merged
merged 19 commits into from
Aug 6, 2020
Merged

Add onSubmit to form #589

merged 19 commits into from
Aug 6, 2020

Conversation

snowystinger
Copy link
Member

@snowystinger snowystinger commented Jun 6, 2020

Closes
Not adding onReset yet, there are too many differences between browsers and between controlled/uncontrolled elements within a form. We can consider onReset in the future if there is need.

✅ Pull Request Checklist:

  • Included link to corresponding Issue.
  • Added/updated unit tests and storybook for this change (for new code or code which already has tests).
  • Filled out test instructions.
  • Updated documentation (if it already exists for this component).
  • Looked at the Accessibility Practices for this feature - Aria Practices

📝 Test Instructions:

🧢 Your Project:

Make sure they aren't filtered out
On all of our Form examples, preventDefault so that we don't redirect
Not sure if i should make preventDefault a default setting though...
@adobe-bot
Copy link

Build successful! 🎉

@adobe-bot
Copy link

Build successful! 🎉

# Conflicts:
#	packages/@react-types/form/src/index.d.ts
@adobe-bot
Copy link

Build successful! 🎉

@adobe-bot
Copy link

Build successful! 🎉

# Conflicts:
#	packages/@react-spectrum/provider/docs/Provider.mdx
#	packages/@react-spectrum/provider/stories/Provider.stories.tsx
@adobe-bot
Copy link

Build successful! 🎉

@devongovett devongovett changed the base branch from master to main July 12, 2020 00:54
'target',
'onSubmit',
'onReset'
]);

Choose a reason for hiding this comment

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

Can you add onKeyDown to this?

@joepuzzo
Copy link

Any updates on when this will get released?

@joepuzzo
Copy link

joepuzzo commented Jul 31, 2020

Could you add onKeyDown to the list of props. I use it to control weather enter triggers a submit. https://github.com/joepuzzo/informed/blob/548a4b9298172168688046f19a058cbe1ace268e/src/FormController.js#L517

Here is example https://joepuzzo.github.io/informed/?path=/story/hooks--useform

@matthewdeutsch matthewdeutsch linked an issue Jul 31, 2020 that may be closed by this pull request
@joepuzzo
Copy link

joepuzzo commented Aug 3, 2020

@snowystinger can you please fix the conflict so this can be merged? Much appreciated.

@snowystinger
Copy link
Member Author

Hi @joepuzzo, thanks so much for your interest, this work isn't in our immediate sprints. This was really a bit of an exploration. If you'd like to contribute, we could help with that.
On a side note, instead of onKeyDown, could you preventDefault in onSubmit?

@joepuzzo
Copy link

joepuzzo commented Aug 4, 2020

@snowystinger Key down is for preventing the enter key from submitting the form. I already call preventDefault in the onSubmit. Also, this work seems to already be complete? It just needs to get merged into master and published. Any ideas when that may be as this prevents doing any form submissions. Thanks again for the awesome work.

@snowystinger
Copy link
Member Author

@joepuzzo You shouldn't need to do anything other than preventDefault in onSubmit https://www.w3schools.com/jsref/event_preventdefault.asp

As for this PR, there are a lot of issues with onReset right now. You can try them out in different browsers in the stories that I modified specifically for this PR. If you don't need onReset, then we can can accept a smaller PR that just adds onSubmit

@joepuzzo
Copy link

joepuzzo commented Aug 4, 2020

Hmm what is the issue with onReset exactly? Yes on submit would get me mostly what I need. That would at least allow form submissions to work :)

@snowystinger snowystinger changed the title Add form props [WIP] Add form props Aug 4, 2020
# Conflicts:
#	packages/@react-spectrum/provider/docs/Provider.mdx
…now, we can add it back in followup if anyone needs it
@snowystinger snowystinger changed the title [WIP] Add form props Add onSubmit to form Aug 5, 2020
Copy link
Member

@devongovett devongovett left a comment

Choose a reason for hiding this comment

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

tests are failing

packages/@react-types/form/src/index.d.ts Outdated Show resolved Hide resolved
did more research, only get and post are valid form methods, other libraries hide this by accepting the others but actually submitting a post request
enctype can truly only be three values, any custom ones will result in inconsistent behavior, if something else is needed, then js ajax must be used
@adobe-bot
Copy link

Build successful! 🎉

@adobe-bot
Copy link

Build successful! 🎉

@adobe-bot
Copy link

Build successful! 🎉

@adobe-bot
Copy link

Build successful! 🎉

@devongovett devongovett merged commit 5fd2e04 into main Aug 6, 2020
@devongovett devongovett deleted the add-form-props branch August 6, 2020 21:29
@joepuzzo
Copy link

joepuzzo commented Aug 7, 2020

Thanks guys! What is your release cycle like? Do you release beta when you merge to master?

@devongovett
Copy link
Member

Should be early next week. We're working on getting automated nightly builds set up as well so should be faster in the future.

@joepuzzo
Copy link

Any updates on beta releases?

@snowystinger
Copy link
Member Author

today or tomorrow hopefully
also, during testing we found that there was some missing behavior, it may not make it into this release unfortunately #904

@joepuzzo
Copy link

Missing behavior from a forms onSubmit? What is the behavior?

@snowystinger
Copy link
Member Author

@LFDanLu found a bug where Forms couldn't be submitted by pressing enter or spacebar on the submit button.

@joepuzzo
Copy link

That bug already exists. I think it has to do with the button. Not the form

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.

Form does not allow onSubmit and other form props to be passed.
4 participants