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

feat: add autorotate #17

Merged
merged 4 commits into from
Nov 26, 2023
Merged

Conversation

webmarkyn
Copy link
Contributor

Turntable is automatically rotated if autorotate prop has enabled property set to true. Autorotation is disabled as soon as mousedown/keydown events are fired.

@nerdyman
Copy link
Owner

Thanks for opening this @webmarkyn, it's a great feature! I'll take a look at it today

@nerdyman nerdyman self-requested a review November 16, 2023 10:31
Copy link
Owner

@nerdyman nerdyman left a comment

Choose a reason for hiding this comment

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

Thanks for this feature @webmarkyn!

I've left a pretty minor tweak for the autoRotate prop in the review.

Also, I think it should continue rotating after the user stops interacting with the component too, what do you reckon?

Example scenario with autoRotate.disabled: false:

  • Stop autorotation while pointer down
  • Resume autorotation on pointer up

It would be better this way IMO since the default behaviour would always do what the autoRotate.disabled prop says rather than only doing it once. This means a developer could choose to disable autorotation if they wanted be adding a pointerdown event to the turntable and disable it themselves if thet wanted to.

E.g.

// Developer can choose to disable autorotation themselves if they want to.
const SomeComponent = () => {
  const [disabled, setDisabled] = useState(false);
  
 const onPointerDown = () => {
   setDisabled(true);
  }


  return (
    <ReactImageTurntable 
      autoRotate={{ disabled }} 
      onPointerDown={onPointerDown}
    />
  );
};

enabled: boolean;
/** The speed (in ms) at which the turntable rotates. */
speed?: number;
};
Copy link
Owner

Choose a reason for hiding this comment

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

I think autoRotate.enabled could be replaced with an optional autoRotate.disabled prop and speed could be renamed to interval like you've done in the internal lib code to more closely describe what it does.

// type 
{
  autoRotate?: {
    /** Whether to disable the autorotation of the turntable. */
    disabled?: boolean;
    /** The speed (in ms) at which the turntable rotates. */
    interval?: number;
  };
}

// default value
autoRotate = { disabled: true, interval: 200 }

@webmarkyn
Copy link
Contributor Author

@nerdyman thanks for the review. Made the changes you requested, please take a look.

@webmarkyn
Copy link
Contributor Author

@nerdyman Hello. Can you check the PR please?

@nerdyman
Copy link
Owner

nerdyman commented Nov 21, 2023 via email

@webmarkyn
Copy link
Contributor Author

@nerdyman Sorry to hear that. Take care.

@nerdyman nerdyman merged commit 7238297 into nerdyman:main Nov 26, 2023
1 check failed
@nerdyman
Copy link
Owner

Thanks for your work and patience on this @webmarkyn!

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