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 .slice() method for ShallowWrapper and ReactWrapper #661

Merged
merged 1 commit into from
Nov 12, 2016

Conversation

nhardy
Copy link
Contributor

@nhardy nhardy commented Nov 6, 2016

Adds a new .slice() method for ShallowWrapper and ReactWrapper with functionality like Array#slice.

Closes #647.

Copy link
Member

@ljharb ljharb left a 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`).
Copy link
Member

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.

Copy link
Member

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`).
Copy link
Member

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)

Copy link
Member

@ljharb ljharb left a 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.

@ljharb
Copy link
Member

ljharb commented Nov 6, 2016

@nhardy this does need a rebase on latest master, though, if you don't mind :-)

@nfcampos
Copy link
Collaborator

This looks great! Thanks @nhardy !

@ljharb ljharb force-pushed the master branch 2 times, most recently from 52cfb83 to 583c04c Compare November 12, 2016 17:01
@aweary
Copy link
Collaborator

aweary commented Nov 12, 2016

LGTM! @ljharb looks like CI is hanging, can you restart the job?

@ljharb
Copy link
Member

ljharb commented Nov 12, 2016

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.

@nhardy
Copy link
Contributor Author

nhardy commented Nov 13, 2016

Meant to get to rebasing the PR today, but thanks for taking care of that :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants