Rewrite slide display and transition logic. #691
Conversation
1. Change Deck.js so that it provides a render prop to the child. 1. Modify Controls.js so that they accept dispatch prop and render buttons. 1. Modify test.mdx so that it renders Controls and a currentSlide value.
|
Pushed up most recent commits, had to go home due to migraine :(. Will hopefully look to documenting and fixing some issues with the current set-up but hopefully gives some insight into what's happening! |
|
This looks great! Let me know what you think about my comments 😃 |
* Finished slide logic * Started adding in documentation
* removed Controls as no longer needed.
- Only shows one slide element at a time - Update test.mdx to better show this off
Implement react spring transitions
Fixed reverse transition
* added custom transitionEffect prop to Slide component. * added default transitionEffect
Fixed JS example. Moved back to using MDX example.
* Implemented springs for SlideElementWrappers * SlideElementWrapper can accept custom springs now!
Added proptypes
1. Fixed proptypes 2. improved documentation
1. Refactors some code. 2. Added in eslint for hooks cause I'm bad 3. Added in keyboard props to deck
Fixed some animation issues with precision. Added configuration so when going backwards you could skip animations. Stashed on this branch an experiment to skip animations if a user double-presses forwards, needs further study.
src/hooks/useSlide.js
Outdated
| */ | ||
|
|
||
| // Keeps previous slide number for comparison later. | ||
| let prevSlideNum; |
There was a problem hiding this comment.
I think using usePrevious might be more idiomatic. https://usehooks.com/usePrevious/
We might also run into problems with this global variable if we have multiple invocations of useSlide.
| set({ | ||
| opacity: 1 | ||
| }); | ||
| set({ from: transitionEffect.from, to: transitionEffect.to }); |
There was a problem hiding this comment.
Could we simplify this even further and drop the from case and just do set({ transitionEffect.to })? Seems to produce the same result I think.
src/components/Slide.js
Outdated
|
|
||
| const numberOfSlideElements = Array.isArray(children) | ||
| ? children.filter(x => isComponentType(x, 'SlideElementWrapper')).length | ||
| ? children.filter(x => isComponentType(x, 'SlideElementWrapper')).length + 1 |
There was a problem hiding this comment.
What's the + 1 for?
There was a problem hiding this comment.
Ummmm I'm gonna remove it, thanks for picking it up!
src/components/Deck.js
Outdated
| defaultSlideEffect), | ||
| config: { precision: 0 }, | ||
| unique: true, | ||
| immediate: rest.animationsWhenGoingBack ? false : state.immediate |
There was a problem hiding this comment.
This is a nit picky comment but could we pass animationsWhenGoingBack as an argument to useDeck? It might be nice to put all the "immediate" logic in one place. Not a big deal though, feel free to ignore.
There was a problem hiding this comment.
Yeaaah this is a great suggestion!
kiraarghy
changed the title
This branch rewrites the core state management logic and animation logic.
The design philosophy behind this has been moving away from the top-down redux model to a more bottoms-up local-state model (though using reducers as I think they're easier to make sense of with state changes.)
This also implements a move to using React-Spring, which should allow users to implement their own artisan animations between slides, and between slide elements.
As it is this represents an MVP before moving onto the display and layouts API for slides.