Skip to content

Conversation

@paulozoom
Copy link
Contributor

When $.rails.disableForm(form) is called twice in a row, the second execution makes [data-disable-with] inputs lose their original val() or html(), which the first execution stored in [data-ujs:enable-with].

Adding :enabledto the disableSelector prevents this.

Ensures elements aren’t disabled twice.
@lucasmazza
Copy link
Contributor

@paulozoom thanks for the contribution, but could you please add a test along with your patch?
Thanks!

@paulozoom
Copy link
Contributor Author

@lucasmazza Have to admit I never did testing, so it may take me a while. Can you point me into the direction so I can learn it quickly?

@lucasmazza
Copy link
Contributor

@paulozoom no luck figuring it out how to replicate that scenario in the tests, but I believe we can get this in without touching the tests for now. Could you resolve the conflicts on this branch and push it? Thanks ❤️

@JangoSteve
Copy link
Member

@paulozoom I think I understand what's happening and may be able to replicate in a test, but in order to do that, I'd have to manually execute some code twice that in reality shouldn't ever be triggered twice. Can you explain how the disableForm() function is being triggered twice with your setup? What scenario would cause this to happen?

@paulozoom
Copy link
Contributor Author

@JangoSteve: I have a e-commerce checkout form in which the shipping cost is updated live based on the checkout address. Whenever the user starts typing/changing any of the address fields, I disable the form with a specific message, say $form.disableFormElementsWith('Calculating shipping');. It doesn't check whether the form is already disabled — that would be one DOM call to check, and another to disable (if applicable), whereas immediately disabling the form with the :disabled filter is always just one. Also, since enableSelector only enables any :disabled inputs, I expected disableSelector to do likewise — I couldn't see a reason not to, hence the pull request.

@lucasmazza I'll take a look

Conflicts:
	src/rails.js
@paulozoom
Copy link
Contributor Author

@lucasmazza There you go, I think.

lucasmazza added a commit that referenced this pull request May 13, 2014
Add :enabled to disableSelector
@lucasmazza lucasmazza merged commit 8f190ca into rails:master May 13, 2014
@paulozoom paulozoom deleted the patch-1 branch May 17, 2014 17:06
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