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

Buffer fix #25

Merged
merged 1 commit into from
Aug 20, 2015
Merged

Buffer fix #25

merged 1 commit into from
Aug 20, 2015

Conversation

shaunc
Copy link
Contributor

@shaunc shaunc commented Aug 19, 2015

Buffer should be added for both previous and next items, but only previous items were buffered.

@@ -40,7 +40,7 @@ export default Ember.Component.extend({
// this.lastCell = undefined;
// this.cellCount = undefined;
this.contentElement = undefined;
this._cells = [];
this._cells = Ember.A([]);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did you add this?

@mmun
Copy link
Contributor

mmun commented Aug 19, 2015

Is there a way we can test the buffer without digging into state variables on the collection component? Specifically I'm talking about

var buffer = view.get('buffer') | 5;
var width = view.get('width') | 0;
var itemWidth = view.get('itemWidth') || 1;

I'm happy to merge as is if its not feasible, but I suspect it should be by analyzing the DOM?

cc @raytiley

@mmun
Copy link
Contributor

mmun commented Aug 19, 2015

Also, please rebase against master. In the ember projects we only merge master for long running branches.

@shaunc
Copy link
Contributor Author

shaunc commented Aug 19, 2015

The real problem vs "digging into state" is that I'm doing a calculation that is really done by the layout, which I don't have access to. This could be done if sliceDidChange were available much more easily. The # of items in the dom is what I'm checking so I can't depend on DOM to get this info.

@mmun
Copy link
Contributor

mmun commented Aug 19, 2015

Gotcha.

@shaunc
Copy link
Contributor Author

shaunc commented Aug 19, 2015

I'll rebase ... anything else for the moment? If/when "sliceDidChange" goes through & in order to write tests for mixedGrid these may want to be rewritten...

@mmun
Copy link
Contributor

mmun commented Aug 19, 2015

No, it's fine how it is other than the Ember.A() thing. You can leave a TODO comment about updating the tests later if you want.

@shaunc
Copy link
Contributor Author

shaunc commented Aug 19, 2015

Ah -- didn't notice your Ember.A comment; sorry:

  1. I did get an error earlier in a test that went away when I put Ember.A in init() (I think it was "pushObject not present). Perhaps I still had the package to turn off injections still installed, though it was removed from npm (or bower)?
  2. In any case, I thought the guidance for Addons was that we should write without assuming that the ember stuff is injected. If so, the
  this._cells.pushObject(cell);

further down will break if you don't wrap in Ember.A.

@mmun
Copy link
Contributor

mmun commented Aug 19, 2015

Perhaps I still had the package to turn off injections still installed, though it was removed from npm (or bower)?

That's gotta be it.

Ember.A() is pretty dubious because it copies all the array extensions onto the array instance. If you want to avoid prototype extensions in your addon, the correct solution is not to copy all the extensions onto all your array instances, it is to not use prototype extensions :P

@mmun
Copy link
Contributor

mmun commented Aug 19, 2015

I think if you swap this._cells.pushObject with this._cells.push we can disable all prototype extensions again. We should do that separately though.

@shaunc
Copy link
Contributor Author

shaunc commented Aug 19, 2015

Um... so should I put in a separate pull request to not use pushObject?

@shaunc
Copy link
Contributor Author

shaunc commented Aug 19, 2015

ah -- just what I was thinking

@mmun
Copy link
Contributor

mmun commented Aug 19, 2015

:D I gotta run, I'll be back in a little while.

@shaunc
Copy link
Contributor Author

shaunc commented Aug 19, 2015

Sure ... Should I create a new PR with pushObject -> push and https://github.com/rwjblue/ember-disable-prototype-extensions installed?

@mmun
Copy link
Contributor

mmun commented Aug 19, 2015

@shaunc Yes exactly. You might have to add Ember.A() in some tests but I doubt it.

@mmun
Copy link
Contributor

mmun commented Aug 19, 2015

@shaunc
Copy link
Contributor Author

shaunc commented Aug 20, 2015

@mmun -- ok -- I have rebased

mmun added a commit that referenced this pull request Aug 20, 2015
@mmun mmun merged commit c0d5ea2 into adopted-ember-addons:master Aug 20, 2015
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.

2 participants