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

[dd-ddm] : Fix #1492 regression "Drop Based Coding" #1573

Merged
merged 4 commits into from
Jan 29, 2014

Conversation

hacklschorsch
Copy link
Contributor

Fixes #1492 (#1492)

At ~ 3.7.3 a regression was introduced by 1a55a78 that broke
"Drop Based Coding" by using Array.each() for otherDrops which
is an Object Literal.

At ~ 3.7.3 a regression was introduced by 1a55a78 that broke
"Drop Based Coding" by using Array.each() for otherDrops which
is an Object Literal.
@okuryu
Copy link
Member

okuryu commented Jan 23, 2014

Thanks for your patch. I prefer using Y.Object.each() instead of Y.each() for the performance at this case.

Also, I think it need to change ddm-drop.js#L250-254 to Y.Object.each() equivalently.

                Y.Array.each(drops, function(v) {
                    if (v !== biggest) {
                        out.push(v);
                    }
                }, this);

@hacklschorsch
Copy link
Contributor Author

Yes, thanks!

I hope the thing will never ever be an array. To be quite frank I haven't understood why it is an object anyway. Whatev, please make it work.

I think it would be nice if Y.Array.each and Y.Object.each would at least throw an exception in debug builds of YUI when being fed objects of the wrong type.

@okuryu
Copy link
Member

okuryu commented Jan 25, 2014

I hope the thing will never ever be an array. To be quite frank I haven't understood why it is an object anyway. Whatev, please make it work.

otherDrops property always works as an object as far as I can see.

I think it would be nice if Y.Array.each and Y.Object.each would at least throw an exception in debug builds of YUI when being fed objects of the wrong type.

Y.Array.each() actually calls ES5 native Array.forEach() if it available. if you passed an object to Y.Array.each(), they will be ignored. Also, Y.Object.each() works with both object and array. Example is here: http://jsbin.com/UNadUse/1/edit?html,js,console

@hacklschorsch
Copy link
Contributor Author

Thanks @okuryu, I wasn't aware of that. I added commits to the PR which include your suggestions. HTH & greetings from Munich!

@okuryu
Copy link
Member

okuryu commented Jan 25, 2014

@hacklschorsch Thanks! Are you signed our CLA already?

@hacklschorsch
Copy link
Contributor Author

Just now e-Signed it. Thanks!

@okuryu
Copy link
Member

okuryu commented Jan 26, 2014

Looks good to me. Current status is waiting any feedbacks by other Committers and Reviewers. I'll merge this after 72 hours or when get explicit approval by a Reviewer.

@hacklschorsch
Copy link
Contributor Author

Mercí <3

@ghost ghost assigned okuryu Jan 28, 2014
@okuryu okuryu merged commit b7a6d1a into yui:dev-master Jan 29, 2014
@okuryu
Copy link
Member

okuryu commented Jan 29, 2014

I've merged this into dev-master branch. I believe it'll be available in the next release.

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