-
Notifications
You must be signed in to change notification settings - Fork 5
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
Yield reset and submit actions #197
Conversation
🦋 Changeset detectedLatest commit: bf5aca5 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
f2d34dc
to
44f0efd
Compare
44f0efd
to
c1130c6
Compare
"ember-source": "~5.1.0", | ||
"ember-source-channel-url": "^3.0.0", |
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.
Some ordering of packages after I pnpm add @types/ember__modifier -D
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.
These tests essentially follow the way some of our other tests work when testing functionality of headless-form
. We don't rewrite the headless-form
tests, we simply test for very basic functionality only. Simon has test coverage for this work in headless-form
via this PR
Looks like Cloudlfare is having troubles at the moment, but I'll rekick the job in the AM (when I imagine it'll be back to normal). |
Preview URLsEnv: preview |
Cloudflare issue resolved 🎉 |
|
||
await click('[data-test-reset]'); | ||
|
||
assert.dom('[data-name]').hasValue('Simon'); |
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.
Oh nice! Such a great reset
Should we also test that it resets
to empty form data? (not just the form is prefilled with data)
Also, it would be nice to also update the docs with both yielding submit
and reset
?
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.
Also, it would be nice to also update the docs with both yielding submit and reset?
Good call - adding some docs in the latest commit!
Should we also test that it resets to empty form data? (not just the form is prefilled with data)
I outlined this a bit at https://github.com/CrowdStrike/ember-toucan-core/pull/197/files#r1245937002 - I'm a bit hesitant to add too much repetition between our tests and headless-form, personally.
submit: () => void; | ||
|
||
/** | ||
* Yielded action that will reset form state, same as when triggering the native `reset` event on the form. |
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.
Should note that if a FormData is present, it will reset to that, not just blank form inputs.
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.
Good call - let me add some docs. I took these straight from ember-headless-form
. I didn't want to get too into the weeds on what all this does, as that's a concern of ember-headless-form
, we are only exposing the action from there.
This looks great! Really excited to see these items |
|
||
The component yields back `submit` and `reset` actions for more complex cases of submitting or resetting your form data. | ||
|
||
**NOTE:** Calling `submit` directly is **not** required for most cases. The implementation only requires a button tag with the `type="submit"` attribute set. |
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 for adding this, and also pointing out that's it's not needed most of the time.
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.
That's what a call a nice changelog entry! 😍
🚀 Description
This PR exposes
reset
andsubmit
to consumers viaember-headless-form
.Closes #196
cc: @ryhinchey
🔬 How to Test
📸 Images/Videos of Functionality
N/A