RSSchermer / ember-rl-dropdown

Dropdown components and mixin for Ember.
MIT License
15 stars 12 forks source link

sendAction on close/open so you can listen to changes outside component #17

Closed hakubo closed 8 years ago

hakubo commented 8 years ago

This way you can listen outside of the dropdown to changes.

hakubo commented 8 years ago

I can see that ember try:one ember-beta test fails on master too Is that something you intend to fix?

RSSchermer commented 8 years ago

Thanks for contributing! I like this idea :+1:

I think it might be better if we use the modern way of binding and invoking actions. Rather than using sendAction you can just invoke the action bound to the property:

let onDropdownChangeState = this.get('onDropdownChangeState');

if (onDropdownChangeState) {
  onDropdownChangeState(this.get('dropdownExpanded'));
}

And then use it like this:

{{#rl-dropdown-container onDropdownChangeState=(action "myHandler") ...}}
  ...
{{/rl-dropdown-container}}

I also think I might prefer having 2 separate actions that don't take parameters rather than one more complex action, maybe onExpand and onCollapse? What are your thoughts on this?

I can see that ember try:one ember-beta test fails on master too Is that something you intend to fix?

Yes, I will look into the beta failure after this is merged.

hakubo commented 8 years ago
let onDropdownChangeState = this.get('onDropdownChangeState');

if (onDropdownChangeState) {
  onDropdownChangeState(this.get('dropdownExpanded'));
}

That looks good, but what are the benefits of this pattern? Why is this better than using sendAction? if you have some articles - I'm happy to read about it :)

Yeah, two separate events sound good too. I implemented one as this covers my use case - I just need to know that something happened.

If you don't have a strong preference - I would leave one event with a parameter. If you think having two of them would improve ergonomy of the mixin, I can update it.

RSSchermer commented 8 years ago

That looks good, but what are the benefits of this pattern? Why is this better than using sendAction? if you have some articles - I'm happy to read about it :)

Here's what the Ember team had to say about them when they were introduced: http://emberjs.com/blog/2015/06/12/ember-1-13-0-released.html#toc_closure-actions

I do have a preference for 2 separate actions. I think that upon reflection I might prefer onOpen and onClose for internal consistency.

hakubo commented 8 years ago

what do you think about:

  onOpen: Ember.K,

  actions: {
   myAction() {
     this.get('onOpen')();
    }
  }
RSSchermer commented 8 years ago

I wasn't aware of Ember.K but using that as a default value indeed seems like a cleaner option than my null guard. :+1:

hakubo commented 8 years ago

Have a look now! :)

RSSchermer commented 8 years ago

Excellent, thanks a lot! Merging...

Will look into the failing tests on ember beta tonight or tomorrow and then I'll release this as a new minor version.

RSSchermer commented 8 years ago

Published as 0.8.0.