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 support for the data-disable attribute. #347

Merged
merged 7 commits into from
Apr 28, 2014
Merged

Conversation

lucasmazza
Copy link
Contributor

This is a spike for what was proposed on #329.

This gives the same behaviour as the data-disable-with attribute, but instead of using a replacement String from the data-disable-with attribute the disabled state will use the origin text/value of the element. Since there is a lot of shared behaviour between data-disable and data-disabled-with I duplicated the test case so we can test the same stuff for both attributes.

element.html(element.data('disable-with')); // set to disabled state
var enabledState = element.html();
element.data('ujs:enable-with', enabledState); // store enabled state
element.html(element.data('disable-with') || enabledState); // set to disabled state

Choose a reason for hiding this comment

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

Maybe we should try to avoid replacing the html in case there's no disable-with? We know it should not change at this point right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess I can bring some of the changes from #348 and avoid these html calls after all.

This gives the same behavior as the `data-disable-with` attribute,
but instead of using a replacement String from the `data-disable-with`
attribute the disabled state will use the origin text/value of the element.
Conflicts:
	test/public/test/data-disable.js
@lucasmazza
Copy link
Contributor Author

summon @rafaelfranca

@rafaelfranca
Copy link
Member

:shipit:

Conflicts:
	src/rails.js
	test/public/test/data-disable.js
@daniel-rikowski
Copy link

I experimented with this change this morning. The feature seems to work "as advertised" and I could not find any regression. 👍

I'd definitely like to see this getting merged.

BTW: I never noticed disable(-with) works with text inputs and textareas, too :)

@daniel-rikowski
Copy link

I've been using this ever since without any problems. Will this be merged into master?

@JangoSteve
Copy link
Member

@lucasmazza I just looked through all this, and it looks good to me. Just gotta do some work to rebase it onto master. I'm working on it right now.

@JangoSteve JangoSteve merged commit d717eeb into master Apr 28, 2014
@JangoSteve
Copy link
Member

OK, merged in. There were some conflicts that had to be resolved, but seems to be working now.

@lucasmazza
Copy link
Contributor Author

thanks @JangoSteve ❤️ I was holding this back to handle all the conflicts in my branch instead of affecting some of the existing PRs.

@lucasmazza lucasmazza deleted the data-disable branch April 28, 2014 13:58
@JangoSteve
Copy link
Member

Ah makes sense. No worries, we'll get those PRs taken care of.

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.

5 participants