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

Adding on-click and on-blur actions #92

Merged
merged 7 commits into from
Feb 25, 2016
Merged

Adding on-click and on-blur actions #92

merged 7 commits into from
Feb 25, 2016

Conversation

Robdel12
Copy link
Collaborator

@Robdel12 Robdel12 commented Feb 7, 2016

This is the first pass to get the ball rolling.

  • TODO: add docs to readme

@Robdel12 Robdel12 added this to the v2.1.0 milestone Feb 7, 2016
@@ -77,7 +77,24 @@ export default Ember.Component.extend({
this._updateValueSingle();
}

this.sendAction('action', this.get('value'), this);
this.sendAction('action', this.get('value'), this); // should we deprecate `action` in favor of `on-change`?
this.sendAction('on-change', this.get('value'), this);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Since we're adding other event actions I feel like we should deprecate the action action in favor of a more descriptive name like on-change.

Copy link
Collaborator

Choose a reason for hiding this comment

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

action is still the name of the attribute that holds the default action in the native ember input component, and so I think we should hold close to that.

* component's action with the jQuery event, x-select value, and the component.
*/
click(event) {
this.sendAction('on-click', event, this.get('value'), this);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't the value of the componnent be the first argument? I believe this is the behavior in the components that ship with ember. We should verify what happens when you do both:

{{input on-click=(action "doStuff")}}

and

<div on-click=(action "do stuff")>Hmmm....</div>

@cowboyd
Copy link
Collaborator

cowboyd commented Feb 8, 2016

If we're going to do blur, then we should also do focusout which is generally more useful.

The skipped test is an odd one. In real life I can click the select,
remove focus, and the focus out action fires. In tests the action
doesn't get fired.

I can tell the `focusOut` event gets called on the component in
tests. But the `sendAction('on-focus-out')` doesn't seem to
fire. Remember this all works in real life. Just tests it's odd.
@cowboyd
Copy link
Collaborator

cowboyd commented Feb 9, 2016

I think also, that we need to get some clarity once and for all about what the Ember conventions are for event attributes because I'm confused.

According to the guides, it appears that the attributes would be onfocusout and onblur.

We need to ascertain

  1. What the event attribute is
  2. What are the arguments that are passed to the handler

and then make x-select copy those conventions as closely as possible.

@Robdel12
Copy link
Collaborator Author

@cowboyd
Copy link
Collaborator

cowboyd commented Feb 16, 2016

@Robdel12 In that example, it uses focus-out and not on-focus-out

@Robdel12
Copy link
Collaborator Author

@cowboyd it looks like you can't send an action called click or blur? It seems to break.

cowboyd added a commit that referenced this pull request Feb 25, 2016
Adding `onclick` and `onblur`, `onfocusout` actions
@cowboyd cowboyd merged commit 32bbabe into master Feb 25, 2016
@cowboyd cowboyd deleted the add-ddau-actions branch February 25, 2016 20:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants