Skip to content

Conversation

@karevn
Copy link

@karevn karevn commented Oct 19, 2015

An implementation for this issue: #1433

@yyx990803
Copy link
Member

Nice one. Since this is v-for specific, maybe we can move the part into v-for as a method, and just call it in the watcher, e.g. forContext.applyReverseFilter(scope, value)

@karevn
Copy link
Author

karevn commented Oct 19, 2015

Agree. Btw, remember I offered to use <v-for> component instead of v-for directive? It would not only be a syntactic sugar, but would also allow to get rid of $forContext at all (it would all be about child - parent context relationships).

Copy link
Member

Choose a reason for hiding this comment

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

I think splitting this here makes the code harder to follow. It's ok that this check stay in the watcher.

Copy link
Author

Choose a reason for hiding this comment

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

It's more about responsibilities, IMO. Written this way, misc.js mixin is responsible for both forward and reverse filters, and abstracts forContext existence from the watcher.

Also, alias === expression does not look nice (because I easily bypassed this logic in my initial fiddle), so the logic will probably grow and I definitely don't want to put it to watcher (because it is not its responsibility) or for directive (to keep reverse filtering logic reusable just for a case). Any ideas?

Copy link
Member

Choose a reason for hiding this comment

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

When I saw scope._bubbleChanges it's a bit confusing, because this is a v-for-scope-only thing, but is attached to Vue's prototype, which imo is the wrong place. It only works because of the prototypal inheritance.

Copy link
Member

Choose a reason for hiding this comment

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

As for expression check - a reverse write should be applied as long as the expression starts with the alias. e.g. item.test and item[test] both should trigger reverse write. So you need a regular expression, e.g. new RegExp(alias + '\\b'). To be more efficient we might want to construct this regex right when we parse the alias in v-for.

@yyx990803
Copy link
Member

Btw, if possible please re-submit this to the dev branch. Latest 1.0 work is now in the dev branch.

@ShahanurSharif
Copy link

this.reverse=(this.sortKey==sortKey)?!this.reverse:false; is not working in latest version for example

{{ people.name}} {{ people.age}}
sortBy: function(sortKey){
        this.reverse=(this.sortKey==sortKey)?!this.reverse:false;
        this.sortKey=sortKey;

    },
}

@karevn
Copy link
Author

karevn commented Nov 13, 2015

@MicroDreamIT I think you're missing the point of this PR. It's intended to allow to re-apply changes done to the array processed with two-way filters. Sorting and reverse order sorting are irrelevant to this.

@ShahanurSharif
Copy link

@karevn I am new in vuejs and javascript world, how can i make it happen?

@karevn
Copy link
Author

karevn commented Nov 14, 2015

@MicroDreamIT please ask this question at Vue chat: https://gitter.im/vuejs/vue

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.

3 participants