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.

Sign up for GitHub

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

Jump to bottom

ObservableArray#findIndex #936

Merged
merged 5 commits into from
Apr 14, 2017
Merged

ObservableArray#findIndex #936

merged 5 commits into from
Apr 14, 2017

Conversation

leaysgur
Copy link
Contributor

@leaysgur leaysgur commented Apr 6, 2017 •
edited
Loading

refs #934

Implement ObservableArray#findIndex to accepts 3rd arguments like ObservableArray#find.

  • Added unit tests
  • Added typescript typings
  • Verified that there is no significant performance drop (npm run perf)

I'll send another PR for updating docs later. -> #938

@leaysgur leaysgur changed the title ObservableArray#findIndex [WIP] ObservableArray#findIndex Apr 7, 2017
@leaysgur
Copy link
Contributor Author

leaysgur commented Apr 7, 2017 •
edited
Loading

@mweststrate

It seems CI fails with your latest commit 1cc4f51
Even on branch master, npm run full-test seems to fail.

Would you be able to check it?(or my mistakes are there?)

@mweststrate
Copy link
Member

@leader22 sorry, my mistake! Fixed the testsuite, so pulling in latest changes should fix it

@coveralls
Copy link

Coverage Status

Coverage increased (+0.007%) to 95.769% when pulling bcdae41 on leader22:array-find_index into 84925b3 on mobxjs:master.

@leaysgur leaysgur changed the title [WIP] ObservableArray#findIndex ObservableArray#findIndex Apr 7, 2017
@leaysgur leaysgur mentioned this pull request Apr 7, 2017
Merged
@leaysgur
Copy link
Contributor Author

leaysgur commented Apr 7, 2017

I've just finished my work.
Would you please check this PR and #938 for docs?

@capaj
Copy link
Member

capaj commented Apr 14, 2017

LGTM. Good job @leader22 !

@mweststrate
Copy link
Member

Agreed! Can be merged and released as well

@capaj capaj merged commit 08e5a4f into mobxjs:master Apr 14, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@leaysgur @mweststrate @coveralls @capaj