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

each-[item] iteration index has for name %item% #552

Merged
merged 1 commit into from
Feb 1, 2016

Conversation

jccazeaux
Copy link
Contributor

Implementation of suggested solution for #551
Tests are included.

@stalniy
Copy link
Contributor

stalniy commented Jan 29, 2016

I believe it'd better to add attribute option or even custom binder. Something like this

<div rv-each-todo="todos" rv-track-by="todoIndex"></div>

This binder just gets index value and aliases it with another name

@jccazeaux
Copy link
Contributor Author

Written like this, rv-track-by becomes a binder. One will believe it will be treated like any other binder. But it's not really a classic binder as it's used to configure an an alias name. We cannot use it as a binder because in the binder we would receive the value of todoIndex wich will probably be undefined

For this to work, the Binder rv-each- would have to read the html value of rv-track-by and not the parsed value (ar you see a better way?).

Finally, is it really necessary to offer customization of index alias? If so, we could handle it with a configuration, like:

rivets.configure({
 // Alias for index in rv-each binder
  iterationAlias: function(modelName) {
    return "%" + modelName + "%";
    // or return "modelName" + Index;
  }
});

Too much customization could lead to less simplicity.

@stalniy
Copy link
Contributor

stalniy commented Jan 29, 2016

I created a fiddle with possible examples how to implement this without touching rivets source:
http://jsfiddle.net/znv28cmj/6/

@blikblum
Copy link
Contributor

I suggest the rv-each-index:todo="todos" syntax

@benadamstyles
Copy link
Collaborator

@blikblim can you explain your idea more, I don't understand how it would work.

@blikblum
Copy link
Contributor

rv-each-[indexName]:[itemName]="[path]"

The index value would be assigned to indexName instead of hardcoded to index. The previous syntax still valid. Although i think is not a really good syntax

@benadamstyles
Copy link
Collaborator

The colon might be a problem anyway http://stackoverflow.com/a/16021232/3098651

@jccazeaux
Copy link
Contributor Author

I don't think we need to add more syntax on the binders for index generation. It would concern only the rv-each.
Using itemNamein the index name is enough and easy to use (and document).

@Duder-onomy Duder-onomy merged commit 82dddbf into mikeric:master Feb 1, 2016
@Duder-onomy
Copy link
Collaborator

I believe this is good idea and should be merged in.
It is a small and harmless addition to the code base. I can imagine that people will find this very useful.

Questions:
What should we do about the existing index functionality? It works pretty well : https://jsfiddle.net/0qgge2ek/ It does not support accessing the parent. See line #288 of functional.js for the tests. I think we should keep it. Does anyone have any strong feelings?

@jccazeaux Thanks for the PR. I am going to add the ability to configure the '%' syntax. This will ensure no naming conflicts, and people can name it whatever makes sense in the domain of their app.

@stalniy You bring up a very important point about the rv-each that I feel will need to be addressed eventually. We need a way to set a config on the binder level. For example, if we wanted to implement a more powerful each binder, one that would keep DOM elements in order. We need a way to tell rivets how to keep track of items in a collection. Right now, if you move an item from the middle to the end of a collection, rivets will rebind each item in the collection after the index from which you spliced. If we had a way to tell the binder how to keep track of things, by their ID for example, then it would know that you moved the item and move the dom element to correspond. My gut was the same as yours, to use a data attribute to set config values. Something like data-track-by would be useful. Adding the rv- could potentially collide with custom binders.

I have updated and merged the PR. If anyone objects, please do so soon as we will be releasing a new version soon with this, and other updates.

@benadamstyles
Copy link
Collaborator

I think we should keep the old index In the interest of keeping the new release as backwards-compatible as possible.

@jccazeaux
Copy link
Contributor Author

@Leeds-eBooks I didn't remove the old index in my PR ;-)

@benadamstyles
Copy link
Collaborator

👍

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.

5 participants