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

Fixing Enumerable#contains deprecation #84

Closed
wants to merge 2 commits into from
Closed

Conversation

ef4
Copy link

@ef4 ef4 commented Oct 1, 2016

As documented in the deprecation guide, addons can use this polyfill to avoid the deprecation while still supporting older versions of Ember.

Closes #82

(The test suite was already failing on beta and canary.)

As documented in [the deprecation guide](http://emberjs.com/deprecations/v2.x/#toc_enumerable-contains), addons can use [this polyfill](https://github.com/rwjblue/ember-runtime-enumerable-includes-polyfill) to avoid the deprecation while still supporting older versions of Ember.
@asennikov asennikov self-assigned this Oct 5, 2016
@asennikov
Copy link
Owner

Fixed deprecation in that commit ( thx for the polyfill hint) - 7cc865a

Also, I like your switch to didRender from observers, I'd like to incorporate that, but I remember there were some issues with this approach before. I'll check again.
However, I'm not sure with open -> _open rename you've added, as these methods are public and are used within other related components.

@ef4
Copy link
Author

ef4 commented Nov 12, 2016

However, I'm not sure with open -> _open rename you've added, as these methods are public and are used within other related components.

If you need that to maintain some backward compatibility that makes sense. But, going forward I would recommend against using methods on components as API between components.

If open and close are things that can be done by an ancestor component of this one, it should be expressed as data coming down (like {{g-map-infowindow isOpen=(something ...)}}).

If open and close are things that can be done by a descendant component of this one, they should be yielded downward as actions that can come back up. {{yield (hash open=(action "open") close=(action "close"))}}

@asennikov
Copy link
Owner

I agree with you. My problem with that initially was that you can't just send isOpen down and expect popup to appear, you need to ensure that the state is in sync with google map object, which does not belong to a single component it is shared between all of them, if it makes sense.

However, I see how that could be improved and this should be refactored, but carefully.

@xomaczar
Copy link

@asennikov can you release a new version with this fix.

@asennikov
Copy link
Owner

@xomaczar yep, done

@xomaczar
Copy link

Thx

Andrey S. Khomenko

On Nov 14, 2016, at 6:17 PM, Alexander Sennikov notifications@github.com wrote:

@xomaczar yep, done


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

@steveszc
Copy link
Collaborator

steveszc commented Apr 13, 2017

Closing. The original Enumerable#includes changes have already been merged in a separate commit.
The need to move to a DDAU architecture is now documented in issue #97. @ef4 thanks for getting the ball rolling on this.

@steveszc steveszc closed this Apr 13, 2017
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.

4 participants