iLoveElysia/pjax
1
0 Fork 0
Code Issues Pull Requests Actions Packages Projects Releases 9 Wiki Activity

Fix form submission #129

Merged
robinnorth merged 4 commits from fix/forms into master 2018-03-02 15:25:09 -05:00
Conversation 13 Commits 4 Files Changed 8
+214 -38
robinnorth commented 2018-02-21 13:29:24 -05:00 (Migrated from github.com)
Copy link
Copy source

Fixes check for radio and checkbox inputs (#124) and GET form submission (#126)

Fixes check for radio and checkbox inputs (#124) and GET form submission (#126)
BehindTheMath (Migrated from github.com) reviewed 2018-02-27 12:30:04 -05:00
lib/send-request.js
BehindTheMath (Migrated from github.com) commented 2018-02-27 12:30:04 -05:00
Copy link
Copy source

@robinnorth Is there any reason not to build the querystring for both GET and POST in 1 step, and then depending on the method, append it to location or the body?

@robinnorth Is there any reason not to build the querystring for both GET and POST in 1 step, and then depending on the method, append it to `location` or the body?
BehindTheMath (Migrated from github.com) reviewed 2018-02-27 12:33:46 -05:00
lib/proto/attach-form.js
BehindTheMath (Migrated from github.com) commented 2018-02-27 12:33:46 -05:00
Copy link
Copy source

@robinnorth Would this be considered a breaking change?

@robinnorth Would this be considered a breaking change?
robinnorth (Migrated from github.com) reviewed 2018-02-28 11:01:17 -05:00
lib/send-request.js
robinnorth (Migrated from github.com) commented 2018-02-28 11:01:17 -05:00
Copy link
Copy source

There was indeed a reason to build them separately to begin with, as I was updating the query string parameters for GET submissions, however I've had to abandon that and discard any previously-set ones in order to properly reflect the behaviour of unchecked checkbox inputs on form submission -- they should be omitted entirely from the query string, even if they had been checked previously.

So, I will refactor this as suggested.

There was indeed a reason to build them separately to begin with, as I was updating the query string parameters for GET submissions, however I've had to abandon that and discard any previously-set ones in order to properly reflect the behaviour of unchecked checkbox inputs on form submission -- they should be omitted entirely from the query string, even if they had been checked previously. So, I will refactor this as suggested.
robinnorth (Migrated from github.com) reviewed 2018-02-28 11:09:17 -05:00
lib/proto/attach-form.js
robinnorth (Migrated from github.com) commented 2018-02-28 11:09:17 -05:00
Copy link
Copy source

Technically maybe, but as we don't document the requestOptions option or its keys, so the implication could be that you shouldn't rely on it to have a certain shape. I'm not sure that it's actually very useful to have requestOptions be a part of this.options anyway, as it's only intended to be set internally by the library. As #127 and #130 show, it just causes issues with manual invocation when it's not set. Perhaps it should be refactored to this.requestOptions?

Technically maybe, but as we don't document the `requestOptions` option or its keys, so the implication could be that you shouldn't rely on it to have a certain shape. I'm not sure that it's actually very useful to have `requestOptions` be a part of `this.options` anyway, as it's only intended to be set internally by the library. As #127 and #130 show, it just causes issues with manual invocation when it's not set. Perhaps it should be refactored to `this.requestOptions`?
BehindTheMath (Migrated from github.com) reviewed 2018-02-28 11:18:59 -05:00
lib/proto/attach-form.js
BehindTheMath (Migrated from github.com) commented 2018-02-28 11:18:58 -05:00
Copy link
Copy source

Do we need a global requestOptions at all? Why don't we just pass it directly to loadUrl()?

Do we need a global `requestOptions` at all? Why don't we just pass it directly to `loadUrl()`?
robinnorth (Migrated from github.com) reviewed 2018-02-28 11:25:28 -05:00
lib/proto/attach-form.js
robinnorth (Migrated from github.com) commented 2018-02-28 11:25:28 -05:00
Copy link
Copy source

No, I don't think we do need it to be global -- it can just be a third parameter to loadUrl(), after options, and default to an empty object if not set.

No, I don't think we do need it to be global -- it can just be a third parameter to `loadUrl()`, after `options`, and default to an empty object if not set.
BehindTheMath (Migrated from github.com) reviewed 2018-02-28 11:31:37 -05:00
lib/proto/attach-form.js
BehindTheMath (Migrated from github.com) commented 2018-02-28 11:31:37 -05:00
Copy link
Copy source

That would be simpler.

I don't think we need to pass options either. When it's needed, it can be retrieved from this.options.

That would be simpler. I don't think we need to pass `options` either. When it's needed, it can be retrieved from `this.options`.
robinnorth (Migrated from github.com) reviewed 2018-02-28 14:46:14 -05:00
lib/proto/attach-form.js
robinnorth (Migrated from github.com) commented 2018-02-28 14:46:14 -05:00
Copy link
Copy source

Currently the history popstate handler relies on being able to pass a modified options object to loadUrl, so I don't think we can easily drop that parameter.

Currently the history popstate handler relies on being able to pass a modified options object to `loadUrl`, so I don't think we can easily drop that parameter.
BehindTheMath (Migrated from github.com) reviewed 2018-02-28 16:31:03 -05:00
lib/proto/attach-form.js
BehindTheMath (Migrated from github.com) commented 2018-02-28 16:31:03 -05:00
Copy link
Copy source

Good point. But if it's not passed in, we can default to this.options.

Good point. But if it's not passed in, we can default to `this.options`.
robinnorth commented 2018-03-01 05:29:36 -05:00 (Migrated from github.com)
Copy link
Copy source

@BehindTheMath I have pushed a refactor commit for the query string building, but have left requestOptions as-is for now, as I intend to make a separate PR to make some changes around that. Would be good to merge this before submitting that too.

@BehindTheMath I have pushed a refactor commit for the query string building, but have left `requestOptions` as-is for now, as I intend to make a separate PR to make some changes around that. Would be good to merge this before submitting that too.
BehindTheMath (Migrated from github.com) approved these changes 2018-03-02 13:46:34 -05:00
BehindTheMath commented 2018-03-02 13:49:18 -05:00 (Migrated from github.com)
Copy link
Copy source

Looks good, so you can squash and merge this.

After thinking more about requestOptions, do we need it to be in this.options so that it gets stored with the history?

Looks good, so you can squash and merge this. After thinking more about `requestOptions`, do we need it to be in `this.options` so that it gets stored with the history?
robinnorth commented 2018-03-02 15:24:58 -05:00 (Migrated from github.com)
Copy link
Copy source

Thanks @BehindTheMath.

Yes, I came to the same conclusion as you about requestOptions after stepping through the code, but am going to change it so that lib/send-request.js handles requestOptions being empty without throwing an error.

Thanks @BehindTheMath. Yes, I came to the same conclusion as you about `requestOptions` after stepping through the code, but am going to change it so that `lib/send-request.js` handles `requestOptions` being empty without throwing an error.
Sign in to join this conversation.
Reviewers
No Reviewers
robinnorth
BehindTheMath
Labels
Clear labels
bug
dependencies
Pull requests that update a dependency file
 duplicate
enhancement
invalid
question
wontfix
No labels
Milestone
No items
No Milestone
Projects
Clear projects
No projects
Assignees
Clear assignees
iLoveElysia
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: iLoveElysia/pjax#129