Skip to content
This repository has been archived by the owner on Apr 12, 2024. It is now read-only.

feat(sliceFilter): create slice filter - more powerful limitTo filter #6399

Closed
wants to merge 1 commit into from

Conversation

gnomeontherun
Copy link
Contributor

Request Type: feature

How to reproduce:

Component(s): misc core

Impact: small

Complexity: small

This issue is related to:

Detailed Description:

This creates a new filter that passes a string or array to native slice() method. This is be useful for pagination or extracting a portion of a string or array. It differs from the limitTo filter because limitTo only has one bound, a starting position. Creating a new filter is more intuitive and reflective of native Javascript rather than extending limitTo.

Other Comments:

It should close issue #5355.

Includes passing tests and docs.

This creates a new filter that passes a string or array to native slice() method. This is be useful for pagination or extracting a portion of a string or array. It differs from the limitTo filter because limitTo only has one bound, a starting position. Creating a new filter is more intuitive and reflective of native Javascript rather than extending limitTo.

It should close issue angular#5355.
@mary-poppins
Copy link

Thanks for the PR! Please check the items below to help us merge this faster. See the contributing docs for more information.

  • Uses the issue template (#6399)

If you need to make changes to your pull request, you can update the commit with git commit --amend.
Then, update the pull request with git push -f.

Thanks again for your help!

@davidjnelson davidjnelson self-assigned this Feb 22, 2014
@davidjnelson davidjnelson added this to the Ice Box milestone Feb 22, 2014
@davidjnelson davidjnelson removed their assignment Feb 22, 2014
@btford
Copy link
Contributor

btford commented Feb 22, 2014

Not sure this should be in Angular core, but thanks for the PR! It might be good to publish this as a module on bower/npm in the mean time.

@gnomeontherun
Copy link
Contributor Author

The ability to have a portion of an array/string is very helpful if you want to do pagination with filters instead of needing to write a controller method. The limitTo filter has only a starting index value, which could be refactored to have a second bound (as mentioned #5355). However there was a lot of doubt in my mind that it would be a clean and logical to do that. If its not for the core, no problem, but the issue I linked to has a tag requesting a PR for this so I went for it. If there is a more appropriate approach, I'd be happy to rewrite it.

@pkozlowski-opensource
Copy link
Member

I don't know how I feel about having 2 filters in core that do more or less the same thing. IMO we should either:

  • extend the limitTo filter to take 2 args
  • depreciate the limitTo and go for splice
  • leave things as they are in the core and suggest more powerful options as community extensions

Personally I would vote for the fist option.

@gnomeontherun
Copy link
Contributor Author

I have another branch the first option already, though its a little confusing when you can accept a negative limit and positive offset. I'd be happy to close this and open another with limitTo having a second argument for offset so you can limit to x after starring from the offset x index position.

@quantizor
Copy link

@pkozlowski-opensource "extend the limitTo filter to take 2 args" - this sounds like the right course of action, IMO.

@tameraydin
Copy link
Contributor

I just sent a PR (#6625) that extends limitTo functionality, so the second argument will indicate the starting index.

Here is a plunker to test (based on doc example): http://plnkr.co/edit/eQF91pmy4Ji7ZckYfE12?p=preview

@pkozlowski-opensource pkozlowski-opensource changed the title feat(sliceFilter): create slice filter feat(sliceFilter): create slice filter - more powerful limitTo filter Dec 14, 2014
@pkozlowski-opensource pkozlowski-opensource self-assigned this Jan 25, 2015
@lgalfaso
Copy link
Contributor

with aaae3cc, this is no longer needed

@lgalfaso lgalfaso closed this Mar 12, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants