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 data-params handling to handleMethod #307

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

phillipp
Copy link

This commit adds the functionality to fetch the data-params attribute from the link and add fields to the form that express the values from the data-param attributes.

This is useful for POST-requests via links that should post parameters instead of sending them in the URL.

@bcm
Copy link

bcm commented Feb 18, 2013

👍

I've got similar behavior in an extension I've written (except mine handles params of type object as well by creating rails-style bracket names, eg <input name="foo[bar][baz]" value="quux" type="hidden"> for a param foo with value {bar: {baz: 'quux'}}).

one suggestion: you probably want to replace any " characters in params[key] with &quot; so that the generated string is parsed by the browser correctly.

@ghost
Copy link

ghost commented Mar 1, 2013

this needs to be pulled ASAP!!!!

@JangoSteve
Copy link
Member

This can't be pulled in as-is. There's no such thing as a params attribut on a link tag, so using this would require the use of invalid HTML. Instead, it should be looking for the data-params attribute, which would be accessed via link.data('params').

Also, I don't get line 200. The params variable would be populated with the value of data-params, which would be a string, but then you're looping through it like it's an array or object.

@bcm
Copy link

bcm commented Mar 2, 2013

using attr instead of data is probably a simple mistake. and his use case probably requires data('params') to be set programmatically with an object.

if @phillipp doesn't get around to updating his pull request in a couple days then I'll submit one that addresses these issues.

phillipp added a commit to phillipp/jquery-ujs that referenced this pull request Mar 5, 2013
@phillipp
Copy link
Author

phillipp commented Mar 5, 2013

Yes, use of attr() was a mistake while copying my path over to another repo. This is fixed by the last commit. As passing an object into the data-params makes the most sense for me, the method is looping over the object.

@JangoSteve
Copy link
Member

Hey @phillipp, could you squash your two commits down into one and update this PR? We also need to get a test added to make sure this is working as intended. If you could do that, I can pull this in right away. otherwise, I'll have to write one when I get some time.

…andler with e.g. method: "post" to add post fields from the a-tags data-params attribute to the form.
@phillipp
Copy link
Author

Hi @JangoSteve, thanks for picking this up again. I suqashed the two commits, but I just have a few hours left before some travel and I can't deal with the testing right now. Sorry.

@JangoSteve
Copy link
Member

Thanks. I'll have to write a test when I get some time then. The one thing I'm concerned about, is that this only allows for data-params to be added as an actual jQuery data object on the link via JS. In fact, if someone had something like

<a href="/somewhere" data-remote=true data-method="post" data-params="my_value=hello">Click</a>

This would append 14 hidden fields to be appended to the form like this:

<input name="0" value="m" type="hidden" />
<input name="1" value="y" type="hidden" />
...

And so on. Given that data-params allows a string or object elsewhere in jquery-ujs, I think it'd be reasonable for people to expect this to be consistent.

I'm wondering if we should check to see if it's a string first, and if so, append it to the form's action.

jfly added a commit to jfly/worldcubeassociation.org that referenced this pull request Jun 14, 2018
To implement this, I had to write a custom javascript handler to handle
switching locales. Previously, this was handled for us via Rails's
unobtrusive javascript, but I could not find any way to add in
additional url parameters to the PATCH request the unobtrusive
javascript was generating for us. There's actually an open PR from 2013
to implement support for this here: rails/jquery-ujs#307.

I also rewrote the set_locale_spec to enable javascript and be more like
what I believe a feature spec should be. I actually don't understand how
the spec was even passing before, I would expect it to fail because it
was sending a GET request, and we don't have a handler for GET request
to update the locale. Maybe rspec/capybara/poltergiest has baked in
support for unobtrusive javascript so that the right thing will happen
even if you don't have javascript enabled?? That sounds crazy, but it's
the only thing I can think of. Regardless of how this was working
before, I do believe that the new specs are better.

This fixes thewca#2962.
jfly added a commit to jfly/worldcubeassociation.org that referenced this pull request Jun 14, 2018
To implement this, I had to write a custom javascript handler to handle
switching locales. Previously, this was handled for us via Rails's
unobtrusive javascript, but I could not find any way to add in
additional url parameters to the PATCH request the unobtrusive
javascript was generating for us. There's actually an open PR from 2013
to implement support for this here: rails/jquery-ujs#307.

I also rewrote the set_locale_spec to enable javascript and be more like
what I believe a feature spec should be. I actually don't understand how
the spec was even passing before, I would expect it to fail because it
was sending a GET request, and we don't have a handler for GET request
to update the locale. Maybe rspec/capybara/poltergiest has baked in
support for unobtrusive javascript so that the right thing will happen
even if you don't have javascript enabled?? That sounds crazy, but it's
the only thing I can think of. Regardless of how this was working
before, I do believe that the new specs are better.

This fixes thewca#2962.
jfly added a commit to jfly/worldcubeassociation.org that referenced this pull request Jun 14, 2018
To implement this, I had to write a custom javascript handler to handle
switching locales. Previously, this was handled for us via Rails's
unobtrusive javascript, but I could not find any way to add in
additional url parameters to the PATCH request the unobtrusive
javascript was generating for us. There's actually an open PR from 2013
to implement support for this here: rails/jquery-ujs#307.

I also rewrote the set_locale_spec to enable javascript and be more like
what I believe a feature spec should be. I actually don't understand how
the spec was even passing before, I would expect it to fail because it
was sending a GET request, and we don't have a handler for GET request
to update the locale. Maybe rspec/capybara/poltergiest has baked in
support for unobtrusive javascript so that the right thing will happen
even if you don't have javascript enabled?? That sounds crazy, but it's
the only thing I can think of. Regardless of how this was working
before, I do believe that the new specs are better.

This fixes thewca#2962.
jfly added a commit to jfly/worldcubeassociation.org that referenced this pull request Jun 15, 2018
To implement this, I had to write a custom javascript handler to handle
switching locales. Previously, this was handled for us via Rails's
unobtrusive javascript, but I could not find any way to add in
additional url parameters to the PATCH request the unobtrusive
javascript was generating for us. There's actually an open PR from 2013
to implement support for this here: rails/jquery-ujs#307.

I also rewrote the set_locale_spec to enable javascript and be more like
what I believe a feature spec should be. I actually don't understand how
the spec was even passing before, I would expect it to fail because it
was sending a GET request, and we don't have a handler for GET request
to update the locale. Maybe rspec/capybara/poltergiest has baked in
support for unobtrusive javascript so that the right thing will happen
even if you don't have javascript enabled?? That sounds crazy, but it's
the only thing I can think of. Regardless of how this was working
before, I do believe that the new specs are better.

This fixes thewca#2962.
jfly added a commit to thewca/worldcubeassociation.org that referenced this pull request Jun 15, 2018
To implement this, I had to write a custom javascript handler to handle
switching locales. Previously, this was handled for us via Rails's
unobtrusive javascript, but I could not find any way to add in
additional url parameters to the PATCH request the unobtrusive
javascript was generating for us. There's actually an open PR from 2013
to implement support for this here: rails/jquery-ujs#307.

I also rewrote the set_locale_spec to enable javascript and be more like
what I believe a feature spec should be. I actually don't understand how
the spec was even passing before, I would expect it to fail because it
was sending a GET request, and we don't have a handler for GET request
to update the locale. Maybe rspec/capybara/poltergiest has baked in
support for unobtrusive javascript so that the right thing will happen
even if you don't have javascript enabled?? That sounds crazy, but it's
the only thing I can think of. Regardless of how this was working
before, I do believe that the new specs are better.

This fixes #2962.
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.

3 participants