-
-
Notifications
You must be signed in to change notification settings - Fork 84
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
Cleanup #9
Conversation
this.offsetY = this._maybeMutAttr('offset-y', 0); | ||
this.width = this._maybeMutAttr('width', 0); | ||
this.height = this._maybeMutAttr('height', 0); | ||
this.buffer = this.getAttr('buffer') | 5; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this line doesn't make sense, why bitwise-or with 5? the idiom with | 0
means int value coercion not default to 0.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am ashamed
I confirmed that it compiles to the same. output in the Babel repl.
@mmun @krisselden are we good on this PR for now? I wanna work on ember-collection for a bit today, but the dummy app is currently broken, and I've already had to do a couple things in this PR. |
@@ -24,7 +24,6 @@ | |||
"ember-cli-inject-live-reload": "^1.3.1", | |||
"ember-cli-qunit": "^1.0.0", | |||
"ember-cli-release": "0.2.3", | |||
"ember-disable-prototype-extensions": "^1.0.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the array observers are removed can we disable prototype extensions again?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i believe so yes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, because we use pushObject. I've selectively enabled Array extensions in the EmberENV though.
// this.rerender(); | ||
// } | ||
// }, | ||
willRender() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this change makes it so the items are never updated after initial render. The rAF
callback just calls rerender
but this won't trigger the updateCells
code path. Instead of calling rerender
should we just do a Ember.run.scheduleOnce(this, 'updateCells')
in the rAF
callback?
No description provided.