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

Feature/manual expand #20

Merged
merged 4 commits into from
Apr 27, 2018

Conversation

tnormington
Copy link
Contributor

I added a new prop for expandedIndex so you can control it manually outside the component. Had to upgrade React for getDerivedStateFromProps, and there is 1 peer dependency warning that I couldn't resolve.

Tim Normington added 2 commits April 25, 2018 15:50
…er dep warning). Added expandedIndex prop so you can manually control which piece is expanded.
@cedricdelpoux
Copy link
Owner

Thank you for your help. But your PR made the build fail. There is an incompatibility with enzyme and the new React. That why I used React 15. But it will be great if you succeed to run test successfully with react 16

@tnormington
Copy link
Contributor Author

Whoops! my bad. I'll look into using componentDidReceiveProps instead of getDerivedStateFromProps & re-submit the PR if I figure anything out.

@cedricdelpoux
Copy link
Owner

@tnormington
Copy link
Contributor Author

Are you open to upgrading to React v16 if I can get enzyme working? It looks like I may just need an adapter plugin: http://airbnb.io/enzyme/docs/installation/react-16.html

@cedricdelpoux
Copy link
Owner

Of course I am! Be my guest :)

@tnormington
Copy link
Contributor Author

tnormington commented Apr 26, 2018

Okay I think I got it working & tests passing with React 16. I had to add an enzyme adapter, and a requestAnimationFrame shim following this comment: jestjs/jest#4545 (comment)

I also had to exclude the shim file from the tests for full coverage.

EDIT: I will rebase this and push today to fix the merge conflict

@cedricdelpoux cedricdelpoux merged commit 15400e1 into cedricdelpoux:master Apr 27, 2018
@cedricdelpoux
Copy link
Owner

Thank you for your work. I fixed the conflict and rebased your commits

@codecov-io
Copy link

Codecov Report

Merging #20 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master    #20   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           3      4    +1     
  Lines          31     32    +1     
  Branches        1      1           
=====================================
+ Hits           31     32    +1
Impacted Files Coverage Δ
setup.js 100% <100%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9992119...3a090e1. Read the comment docs.

@cedricdelpoux
Copy link
Owner

@tnormington Just releaded v2.1.0

Thank you for your help 😄

@tnormington
Copy link
Contributor Author

tnormington commented Apr 27, 2018

Nice! Glad to help, thank for doing the rebase I was struggling 😛

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