yifaneye / react-gallery-carousel

Carousel component 🎠🎠🎠 supporting touch, mouse, keyboard, thumbnails, fullscreen, lazy loading, SSR and customisations. 👉 Live editor: https://yifanai.com/rgcd1
https://yifanai.com/rgc
MIT License
215 stars 30 forks source link

Handlers and Listeners #12

Closed a-tonchev closed 3 years ago

a-tonchev commented 3 years ago

One other demanded feature would be to have handlers outside of the Component, and also listeners before and after action.

E.g.

Handlers: you could provide an ref to the HOC, and just call from outside this.galleryRef.setActiveSlide(3) or this.galleryRef.setFullScreen()

Listeners: beforeSwipe, afterSwipe, beforeScreen

I think that are the only things that I am missing from other galleries. Else I really like your gallery! :)

yifaneye commented 3 years ago

Hi @a-tonchev ! Thank you for raising this issue up! 😄

Thank you for your appreciation! I will add handlers, listeners and publish a new version before the end of next week. 👨🏻‍💻 Rest assured that there will be no breaking changes. I will notify you for the new version.

yifaneye commented 3 years ago

Hi, @a-tonchev ! I carefully considered your request of having handlers! I looked at some other components' interfaces and implementations. I read the React.js documentation at https://reactjs.org/docs/refs-and-the-dom.html#when-to-use-refs. The docs says: "Avoid using refs for anything that can be done declaratively. For example, instead of exposing open() and close() methods on a Dialog component, pass an isOpen prop to it." SS 2021-05-01 at 16 19 55 So, it would be better if developers pass the index, isMaximized and isAutoPlaying. So, I decide not to add handlers at this stage, unless there are other use cases. I thank you for bring this feature request up, my investigation led me to learn something new today!

Meanwhile, I think you are right that my component needs more listeners. 😄 I will add listeners as promised.

a-tonchev commented 3 years ago

Hi @yifaneye,

In the same screenshot that you post is recommended to use refs for the usecases I mention:

ref.fullScreen(), ref.nextSlide(), ref.goToSlide(4), because this is triggering of imperative animation

ref.play() is also okay, because it is media playback

They tell us to avoid using ref for anything that can be done declaratively like close() open() or the other props that you provide like useThumbnails, arrowIcons etc...

There is no other way slightly to jump to slide 4 than using ref, and as you can see any other gallery is using handlers 😉

IMG_20210501_093259

yifaneye commented 3 years ago

Hi @a-tonchev ,

Thank you for taking time to clarify your proposal. I really appreciate your effort. 😃 You mention about the good use cases. I think these use cases in the screenshots are for directly manipulating an element, not for controlling a component.

There is way to "jump to slide 4" other than using ref, that is by passing the index prop to my component.

I had a read on the difference between declarative vs imperative in React.js https://stackoverflow.com/questions/33655534/difference-between-declarative-and-imperative-in-react-js. I think my component provides an interface for developers to control it in a declarative style that is inline with React.js. 😉

Would you like to tell me your use case(s) please?

a-tonchev commented 3 years ago

Hi @yifaneye,

Actually I am creator of a new online shop System SaaS, similar like Shopify. It is called JUST-SELL.online

Until now I used some galleries, but for now I stopped on yours :)

You can check a demo, where you can see your gallery in action here: https://demo2.shopsuy.com/

Well the use-case where I would like to jump on slide is when the user pick product variants (e. g. Colors) to be able the gallery to jump directly on the corresponding slide. For example the slide 4 is with the color red, and when the user is chosing from the dropdown of the product the color red the gallery will automatically open the slide 4 :)

Using refs for this reason is not something new and forbidden - as I sayd everyone is using it :)

a-tonchev commented 3 years ago

You can make use of the useImperativeHandle API for this reason, this will attach to the refs any desired function from inside. Don't forget to use forwardRef if needed ;)

https://reactjs.org/docs/hooks-reference.html#useimperativehandle

https://www.google.com/amp/s/www.geeksforgeeks.org/react-js-useimperativehandle-additional-hook/amp/

yifaneye commented 3 years ago

Hi @a-tonchev ,

Thank you for sharing the demo! I really like your product! 😀 I think you can try set the current index with the index prop in your use case for now.

Thank you for suggesting the use of useImperativeHandle! I am going to have a try! I believe I am going to figure it out! 😉

yifaneye commented 3 years ago

Hi @a-tonchev !

I am pleased to report that I am getting there using useImperativeHandle! I am going to make some examples and refactor the code!

SS 2021-05-02 at 00 20 57

a-tonchev commented 3 years ago

@yifaneye wow thanks man, you are the best. There is no hurry, i still have a lot of work on other issues ;)