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

Install remoteForm directly onto passed element. #33

Merged
merged 3 commits into from
Feb 19, 2024

Conversation

strfx
Copy link
Contributor

@strfx strfx commented Jan 22, 2024

Hi,

remoteForm can install itself onto any element that matches passed selector. In some cases, you might want to install remoteForm onto a specific form element without having to assign this form an id just to make it selectable.

This change allows passing a HTMLFormElement reference to remoteForm directly.

Let me know what you think and if that change fits into the overall design of remoteForm!

remoteForm can install itself onto any element that matches passed
selector. In some cases, you might want to install remoteForm onto
a specific form element without having to assign this form an `id` just
to make it selectable.

This change allows passing a `HTMLFormElement` reference to remoteForm
directly.
@strfx strfx requested a review from a team as a code owner January 22, 2024 13:08
@strfx strfx requested a review from manuelpuyol January 22, 2024 13:08
@@ -45,6 +45,11 @@ describe('remoteForm', function () {
document.querySelector('button[type=submit]').click()
})

it('installs remoteForm on form reference', function (done) {
remoteForm(htmlForm, async () => done())
document.querySelector('button[type=submit]').click()
Copy link
Contributor

Choose a reason for hiding this comment

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

This test is missing any assertions.

Can you please take a look at the test above and add some assertions to insure that the expected behaviour is indeed being installed on the element.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for your feedback!

I was solely relying on Jasmine's done() to verify that the installed handler runs. I've extended the test to verify that we've installed remoteForm on the correct form element 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay thank you for the update. Given that this is testing that the form is installed on the passed reference, I think this test is more complete.

@strfx strfx requested a review from theinterned February 14, 2024 11:24
Copy link
Contributor

@theinterned theinterned left a comment

Choose a reason for hiding this comment

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

This all looks good!

I can't seem to get the test workflow to run, but I ran it locally and all passes 👍

@@ -45,6 +45,11 @@ describe('remoteForm', function () {
document.querySelector('button[type=submit]').click()
})

it('installs remoteForm on form reference', function (done) {
remoteForm(htmlForm, async () => done())
document.querySelector('button[type=submit]').click()
Copy link
Contributor

Choose a reason for hiding this comment

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

Okay thank you for the update. Given that this is testing that the form is installed on the passed reference, I think this test is more complete.

@theinterned theinterned merged commit 1e66834 into github:main Feb 19, 2024
@theinterned
Copy link
Contributor

theinterned commented Feb 19, 2024

This has shipped in v0.3.0! (npm link)

@strfx
Copy link
Contributor Author

strfx commented Feb 20, 2024

awesome, thank you @theinterned! :-)

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