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

Adding functionality to set frame index directly #126

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

liuloppan
Copy link

  • Adding functionality to set the intital "current frame index" by passing it as a prop into Carousel.
  • Adding the function setFrame to Carousel and passing it as a Widget prop ('setFrameHandler') which can be used by Indicator dots to transition multiple frames at once.

* Added functionality to pass currentFrameIndex as props in the cases where you may have a certain order but not necessarily want to 
start at index 0
*  Added functionality to set a frame index directly with setFrame(index) which can be passed as the prop 'setFrameHandler' to Widgets in a similar fashion as 'nextHandler' and 'prevHandler'
setFrame now adds timeouts for 'next' and 'prev' which creates a smoother transition between frames.
@liuloppan
Copy link
Author

liuloppan commented Dec 2, 2019

Solves issues:
#125
#100

@amio
Copy link
Owner

amio commented Dec 3, 2019

Thanks @liuloppan! will look into it soon :D

const diff = Math.abs(index - this.state.currentFrameIndex);
if (index < this.state.currentFrameIndex) {
for (let i = 0; i < diff; i++) {
setTimeout(() => this.prev(), i * ms);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There would be a series of animation for switching to target frame, which works but not ideal, especially when user jumps between a lot frames. We need to switch to target frame directly. 🤔

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok I preferred to have the animation and to be able to control the animation time. For a small number of frames setting 'ms' to 0 will visually have the same effect, but for many frames this could be an issue as you say. But I will look into creating a function that allows changing it directly.

Copy link
Author

@liuloppan liuloppan Apr 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@amio I have updated the PR. Also updated index.js so that the example is using this functionality. Currently currentFrameIndex i passed as prop and is set to 3 to show how it works, but it could be removed.

Now you can chose to have the animation if you put in how many milliseconds (ms) it should animate between. Otherwise (default) is that the state.currentFrameIndex is set as well as state.movingFrames (which has a special override case). Which means we don't need to render any frames in between.

Copy link
Owner

@amio amio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@liuloppan Great! Just a few changes:

  • since loop cannot work with currentFrameIndex, we should log a warning in case of that,
  • let's keep the default demo as original, add some description of currentFrameIndex in README.md

@liuloppan
Copy link
Author

@liuloppan Great! Just a few changes:

  • since loop cannot work with currentFrameIndex, we should log a warning in case of that,
  • let's keep the default demo as original, add some description of currentFrameIndex in README.md
  • loop works with my intended use of currentFrameIndex, I renamed it to onStartFrameIndex and added description in README.md
  • I changed back to the original demo index.js

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.

2 participants