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

Add index as an optional parameter to #each blocks #10160

Merged
merged 1 commit into from
Jan 7, 2015
Merged

Add index as an optional parameter to #each blocks #10160

merged 1 commit into from
Jan 7, 2015

Conversation

tim-evans
Copy link
Contributor

This is the work for #10158.

@mmun
Copy link
Member

mmun commented Jan 6, 2015

Can you add something for changing indexes?

run(function() {
  view.controller.unshiftObject({ name: "Bob" });
});
equal(view.$().text(), "0. Bob1. Adam2. Steve");

@mmun
Copy link
Member

mmun commented Jan 6, 2015

Totally agree that this test belongs in ember-views, but it's fine to leave it in ember-htmlbars for now.

Historically the view tests have been in the templating packages because its easier to setup the view hierarchy. I do plan on moving the more unit-y tests to ember-views one day.

@tim-evans
Copy link
Contributor Author

Yes! I'll do that now

@tim-evans
Copy link
Contributor Author

Should I push with that test failing? I don't really know how to fix that

@mmun
Copy link
Member

mmun commented Jan 6, 2015

@tim-evans Ah, turns out contentIndex doesn't update currently. It will be a bit more complicated.

@mmun
Copy link
Member

mmun commented Jan 6, 2015

@tim-evans I got it working. See if you can incorporate this: https://github.com/mmun/ember.js/tree/each-index

@tim-evans
Copy link
Contributor Author

Merged!

@mmun
Copy link
Member

mmun commented Jan 7, 2015

Can you add a feature flag (It adds new API) and squash all the commits? Otherwise its good to merge.

@ebryn
Copy link
Member

ebryn commented Jan 7, 2015

👍 great work guys

@tim-evans
Copy link
Contributor Author

@mmun done

@@ -357,13 +357,35 @@ var CollectionView = ContainerView.extend({
item = content.objectAt(idx);

itemViewProps.content = item;
itemViewProps._blockArguments = [item];
if (!Ember.FEATURES.isEnabled('ember-each-with-index')) {
Copy link
Member

Choose a reason for hiding this comment

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

Due to a limitation of defeatureify we cannot use !Ember.FEATUREs.isEnabled, you have to use:

if (Ember.FEATURES.isEnabled('blah')) {

} else {
  /* your stuff here */
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So I need a blank if for this one?

Copy link
Member

Choose a reason for hiding this comment

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

Sadly yes, I am sorry. :(

@tim-evans
Copy link
Contributor Author

@rwjblue done.

} else if (this.blockParams === 1) {
view._blockArguments = [item];
}
}
Copy link
Member

Choose a reason for hiding this comment

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

...move it down here.

if (Ember.FEATURES.isEnabled('ember-htmlbars-each-with-index')) {
  if (this.blockParams > 1) {
    view._blockArguments = [item, view.getStream('_view.contentIndex')];
  } else if (this.blockParams === 1) {
    view._blockArguments = [item];
  }
} else {
  if (this.blockParams > 0) {
    view._blockArguments = [item];
  }
}

this adds index as an optional second parameter to #each blocks
fixes #10158
@tim-evans
Copy link
Contributor Author

Oh goodness. Lots of tweaks. I hope this is the last one!

mmun added a commit that referenced this pull request Jan 7, 2015
Add index as an optional parameter to #each blocks
@mmun mmun merged commit dca9c74 into emberjs:master Jan 7, 2015
@tim-evans
Copy link
Contributor Author

Thanks! 👍 (especially @mmun !)

@rwjblue
Copy link
Member

rwjblue commented Jan 7, 2015

@tim-evans - thank you!

@rwjblue
Copy link
Member

rwjblue commented Jan 11, 2015

As of #10193 this is enabled by default on canary (and slated for 1.11 barring any issues).

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