-
Notifications
You must be signed in to change notification settings - Fork 2k
Send submitting elements value along with post #390
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
base: master
Are you sure you want to change the base?
Conversation
function handleSubmitClick(event) { | ||
var $submit = $(this), | ||
$form = $submit.closest('form'), | ||
hiddenClass = 'pjax-submit-button-value' |
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.
Missing comma
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'll get it added.
@@ -135,6 +136,44 @@ function handleSubmit(event, container, options) { | |||
event.preventDefault() | |||
} | |||
|
|||
var submitClickSelectors = 'form[data-pjax] input[type=submit],\ |
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.
Not sure this selector works.. Do all pjax forms necessarily have a data-pjax
attribute?
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 Readme infers as much (https://github.com/defunkt/jquery-pjax#pjaxsubmit), but there is nothing cas iron. I think it is should be 'opt-in' functionality though. Do you agree?
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 samples in the code do not require it:
https://github.com/defunkt/jquery-pjax/blob/master/jquery.pjax.js#L111
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.
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'm happy to do this. I just think that the form functionality as a whole should be opt-in.
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.
Or possibly opt-out? form:not(:pjax-ignore)
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.
If there are no perf implications I would enable it by default, because then pjax-forms would behave like usual forms, which is what devs would expect in the first place IMO
First of all, I'd like to avoid adding hidden elements to the DOM because then we have to worry about cleaning them up after pjax, and probably other edge-cases as well. How about we add the submit button name+value pair to |
Rather than add a hidden element, we could add the name+value to a data attribute on the form, during the click event. Then apply attribute to the post in the submit event. |
@github uses https://github.com/josh/rails-behaviors/blob/master/remote_submit.coffee to generically resolve the submit button tracking. I'm a little wary of the complexity of supporting this here. |
@josh My PR is based on that code. |
In the latest changes I've:
|
Yeah I'm also wary of the complexity to do this, but I think if you install pjax on the page, we should make sure that forms are submitted with the same parameters as they would be natively. Transparency is one of the promises of pjax. This is starting to look usable. I would like if there were more unit tests, though, such as if the form has multiple submit buttons, or the submit buttons don't have a I'd also like to avoid this conflicting with rails-behaviors button tracking. If it conflicts, we cannot have it in pjax. |
I've added more unit tests to deal with:
There should be no conflict with rails-behaviors because we're using data attributes instead of hidden fields. @staabm raised a point re naming conventions. This PR currently assumes the submitting form will be decorated with
I think this could be resolved in a few different ways: Bind All Documentation New Initialiser This:
Becomes:
My preference is for the Documentation option. |
Thanks. I'm traveling right now so it can be a while until I review all of |
Any idea when this might be merged in? |
Also hit this problem recently. The other option is stashing the clicked submit button for all forms using something like the jquery-ujs solution: https://github.com/rails/jquery-ujs/blob/master/src/rails.js#L453 Which means on $.pjax.enable there would need to be something like: $(document).on("click.pjax","form :submit", function(event) {
var button = $(this),
name = button.attr("name"),
value = button.val() || "Submit",
data = name ? {name: name, value: value} : null;
button.closest("form").data("pjax:submit-button", data);
}); Then fetching and respecting it inside handleSubmit, and removing it inside disable. |
also stumbled over this problem today.. |
The recent v54 update to Chrome adds this issue also in it. |
I'm still up for doing any leg work on this, if there is any interest from the team. |
This tripped us up today. We've implemented a Yes/No/Maybe widget as a form with three button elements, submitting different values depending on which button is clicked. This widget works nicely in a no-js environment, but breaks when we try to attach pjax to it. The only other option which works in a degraded no-js environment is three forms, each with a hidden field and a submit button, a la button_to() in Rails. But those extra form tags wreck our css, bleh. It looks like this issue has been outstanding for something like two years now; is there anything we can do to move it forward? |
New PR in favour of #295
Creates hidden element in form to ensure inclusion of value associated to submitting element.