-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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 .slice() method for ShallowWrapper and ReactWrapper #661
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please include tests for passing negative start and end values.
#### Arguments | ||
|
||
1. `begin` (`Number` [optional]): Zero-based index from which to slice (defaults to `0`). | ||
2. `end` (`Number` [optional]): Zero-based index at which to end slicing (defaults to `length`). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this isn't entirely how slice works - slice supports negative indexes as well. https://tc39.github.io/ecma262/#sec-array.prototype.slice has language that might be helpful.
Also, "index" implies "zero-based" - it's zero-based in virtually every language.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(also, please use 1.
for every item in any numbered list, so that reordering doesn't require renumbering)
#### Arguments | ||
|
||
1. `begin` (`Number` [optional]): Zero-based index from which to slice (defaults to `0`). | ||
2. `end` (`Number` [optional]): Zero-based index at which to end slicing (defaults to `length`). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same changes here.
(also, please use 1.
for every item in any numbered list, so that reordering doesn't require renumbering)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, LGTM! I'll leave it open for a few days for other collabs to comment first.
@nhardy this does need a rebase on latest master, though, if you don't mind :-) |
This looks great! Thanks @nhardy ! |
52cfb83
to
583c04c
Compare
LGTM! @ljharb looks like CI is hanging, can you restart the job? |
i rebased it for the OP; travis-ci is having trouble; i've got the merge queued to go on my command line once it passes. |
Meant to get to rebasing the PR today, but thanks for taking care of that :) |
Adds a new
.slice()
method forShallowWrapper
andReactWrapper
with functionality likeArray#slice
.Closes #647.