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

[FEATURE ember-views-text-support-on-change] #9734

Closed

Conversation

@seanpdoyle
Copy link
Contributor

commented Nov 26, 2014

Fixes #5492.

When an <input type="text"> or <textarea> value changes, expose and emit a
change event.

@seanpdoyle

This comment has been minimized.

Copy link
Contributor Author

commented Nov 26, 2014

I would love to add test coverage for this, but I'm not sure where to look for precedent on testing View mixins that depend on DOM elements.

I tried following

test("it should support actions specified as strings", function() {
expect(2);
var obj = EmberObject.createWithMixins(TargetActionSupport, {
target: EmberObject.create({
anEvent: function() {
ok(true, "anEvent method was called");
}
}),
action: 'anEvent'
});
ok(true === obj.triggerAction(), "a valid target and action were specified");
});
but when I invoked view.change(), it blew up, demanding that a jQuery element exist for when .val() is called


Source:     
TypeError: Cannot read property 'val' of undefined
    at Mixin.create._elementValueDidChange (http://localhost:4200/ember.js:42344:36)
    at Mixin.create.change (http://localhost:4200/ember.js:42375:14)
    at Object.<anonymous> (http://localhost:4200/ember-tests.js:48573:12)
    at Object.Test.run (http://localhost:4200/qunit/qunit.js:203:18)
    at http://localhost:4200/qunit/qunit.js:361:10
    at process (http://localhost:4200/qunit/qunit.js:1453:24)
    at http://localhost:4200/qunit/qunit.js:479:5
@mmun

This comment has been minimized.

Copy link
Member

commented Nov 26, 2014

👍 Definitely been wanting this. For the input event as well. (Actually, this PR looks more like what the input event does. change only fires on blur.)

@rwjblue

This comment has been minimized.

@seanpdoyle

This comment has been minimized.

Copy link
Contributor Author

commented Nov 26, 2014

There seems to be a problem when I emit the change action. I think it has to do with the name collision on the change property of the view.

For example, focusIn triggers the focus-in action. They have the same semantics, but the casing difference is what allows them to exist separately.

Is on-change an ok action to trigger? Is there a way around the collision?

@stefanpenner

This comment has been minimized.

Copy link
Member

commented Nov 26, 2014

👍 but this is sorta new API, feature flag might be needed

@seanpdoyle seanpdoyle force-pushed the seanpdoyle:text-support-change-event branch Nov 26, 2014

@stefanpenner

This comment has been minimized.

Copy link
Member

commented Dec 3, 2014

rebase is needed.

@trek

This comment has been minimized.

Copy link
Member

commented Dec 3, 2014

Please document this as well.

@seanpdoyle seanpdoyle force-pushed the seanpdoyle:text-support-change-event branch 3 times, most recently Dec 3, 2014

@seanpdoyle seanpdoyle changed the title [BUG] Emit `change` event for TextSupport [FEATURE ember-views-text-support-on-change] Dec 3, 2014

@seanpdoyle

This comment has been minimized.

Copy link
Contributor Author

commented Dec 3, 2014

@stefanpenner I think this is ready for another look.


@method change
@param {Event} event
*/

This comment has been minimized.

Copy link
@seanpdoyle

seanpdoyle Dec 3, 2014

Author Contributor

@trek is this the type of documentation you were talking about?

I ask because I didn't add the method itself, only the guarded sendAction call. I have a hunch that this method was deliberately undocumented.

@stefanpenner

This comment has been minimized.

Copy link
Member

commented Dec 3, 2014

LGTM, lets let @trek answer the doc question

@trek

This comment has been minimized.

Copy link
Member

commented Dec 5, 2014

Docs LGTM.

@trek

This comment has been minimized.

Copy link
Member

commented Dec 5, 2014

We'll discuss at the core team meeting tomorrow. I think this fits into a larger discussion about making the builtin components event based rather than relying on data binding for communication.

@seanpdoyle seanpdoyle force-pushed the seanpdoyle:text-support-change-event branch 2 times, most recently Dec 11, 2014

@seanpdoyle

This comment has been minimized.

Copy link
Contributor Author

commented Dec 14, 2014

@mixonic did this one slip by during the meeting as well?

@mixonic

This comment has been minimized.

Copy link
Member

commented Dec 15, 2014

Maybe! I've flagged it @seanpdoyle

@seanpdoyle seanpdoyle force-pushed the seanpdoyle:text-support-change-event branch 3 times, most recently Dec 19, 2014

@seanpdoyle

View changes

packages/ember-views/lib/mixins/text_support.js Outdated

if (isEnabled('ember-views-text-support-on-change')) {
sendAction('on-input', this, event);
}

This comment has been minimized.

Copy link
@seanpdoyle

seanpdoyle Dec 23, 2014

Author Contributor

@stefanpenner would I be monkeying around with wiring the polyfill (http://benalpert.com/2013/06/18/a-near-perfect-oninput-shim-for-ie-8-and-9.html) in focusIn and focusOut? Would jQuery handle the onpropertychange => input event wiring?

Simply adding an input key seems to work fine http://emberjs.jsbin.com/hufoxacuno/1/edit

This comment has been minimized.

Copy link
@seanpdoyle

seanpdoyle Jan 2, 2015

Author Contributor

@seanpdoyle seanpdoyle force-pushed the seanpdoyle:text-support-change-event branch 4 times, most recently Jan 2, 2015

@seanpdoyle seanpdoyle force-pushed the seanpdoyle:text-support-change-event branch Jan 5, 2015

@mmun

This comment has been minimized.

Copy link
Member

commented Jan 6, 2015

@seanpdoyle can you rebase?

@mmun

View changes

packages/ember-views/lib/mixins/text_support.js Outdated
import { get } from "ember-metal/property_get";
import { set } from "ember-metal/property_set";
import { Mixin } from "ember-metal/mixin";
import TargetActionSupport from "ember-runtime/mixins/target_action_support";

var isEnabled = Ember.FEATURES.isEnabled;

This comment has been minimized.

Copy link
@mmun

mmun Jan 6, 2015

Member

You should not do this because it will prevent defeaturify from properly stripping out the code for production builds.

This comment has been minimized.

Copy link
@seanpdoyle

seanpdoyle Jan 6, 2015

Author Contributor

Fixed and rebased.

@seanpdoyle seanpdoyle force-pushed the seanpdoyle:text-support-change-event branch Jan 6, 2015

var component, dispatcher;
var isEnabled = Ember.FEATURES.isEnabled;

if (isEnabled("ember-views-text-support-on-change")) {

This comment has been minimized.

Copy link
@seanpdoyle

seanpdoyle Jan 6, 2015

Author Contributor

@mmun is this ok? The build process won't touch the test suite, right?

This comment has been minimized.

Copy link
@mmun

mmun Jan 6, 2015

Member

Yeah, this is ok. :)

}
});
component = Component.createWithMixins(TextSupport, {
"on-change": "changeFired",

This comment has been minimized.

Copy link
@seanpdoyle

seanpdoyle Jan 6, 2015

Author Contributor

@stefanpenner I just thought about this, but should this property be onChange? Aren't dasherized properties mapped to camel case elsewhere?

This comment has been minimized.

Copy link
@jayphelps

jayphelps Jan 17, 2015

Contributor

on-change is correct, there aren't any dash->camel conversions that happen with these (at least now). Here's another example:

@seanpdoyle seanpdoyle force-pushed the seanpdoyle:text-support-change-event branch Jan 20, 2015

[FEATURE ember-views-text-support-on-change]
http://emberjs.jsbin.com/hufoxacuno/1/edit

Implements and Fixes #5492.

When an `<input type="text">` or `<textarea>` value changes,
expose and emit an `on-input` event.

When an `<input type="text">` or `<textarea>` value changes and blurs,
expose and emit an `on-change` event.

@seanpdoyle seanpdoyle force-pushed the seanpdoyle:text-support-change-event branch to c545df0 Jan 26, 2015

@stefanpenner

This comment has been minimized.

Copy link
Member

commented Jul 8, 2015

@mixonic is this related to the kebab case stuff? If so, is this a starting point orr...

@mixonic

This comment has been minimized.

Copy link
Member

commented Jul 8, 2015

What I'd like to see is on-change={{action 'foo'}} where action 'foo' is just returning a function. The string -> sendAction step should not be needed. sendAction in components is not as useful as it was pre-component, as actions do not bubble.

@seanpdoyle I think you got run over by the progress train on this one :-( If you are interested in completing the implementation, I would be very happy to work on the RFC for you and be sure we get consensus and something merged this time. 2.1 is slipping away, but 2.2 seems realistic.

@seanpdoyle

This comment has been minimized.

Copy link
Contributor Author

commented Jul 8, 2015

@mixonic @stefanpenner is this still a good idea? I'm fine with abandoning and closing it if not.

@rwjblue

This comment has been minimized.

Copy link
Member

commented Aug 2, 2015

I'd rather focus on what I am calling "kebab actions" (as @mixonic mentioned above). If we add on-change or on-input to {{input}} that may conflict so, I'd like to hold off any changes here until we have that fleshed out.

@rwjblue rwjblue closed this Aug 2, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.