search

gilbarbara / react-joyride

Create guided tours in your apps
https://react-joyride.com/
MIT License
6.88k stars 529 forks source link

Avoid `this.refs` imperative method calls #156

Closed IanVS closed 7 years ago

IanVS commented 7 years ago

As I've been working more and more with this library, the necessity of calling imperative methods on the Joyride ref has been getting more painful, especially when coupling it with redux. A few of the challenges I've faced:

  1. I do not want my joyride logic to be mixed-in with my components, so I save the ref onto a global variable that I can use from my redux actions. This feels very dirty.

  2. The connect function from react-redux wraps the Joyride and by default will not return the correct ref. It's possible to make it spit out the right ref, but it is not recommended (see this comment/thread from Dan Abromov).

  3. In order to perform the side-effect of interacting with joyride, most of my actions need to be dispatched from thunk action creators (we use redux-thunk). I'd prefer to use only simple actions and drive Joyride by its props alone, rather than manually trying to keep my redux state in sync with the joyride state.

  4. Some of my joyrides need to span several different pages. I persist my redux state with redux-persist, so if I was just controlling Joyride with props and giving it a step index it would be fine. But I haven't found a good way to accomplish this with the current imperative API.

I've been able to refactor Joyride in my branch to rely solely upon props, while still technically allowing the use of imperative methods (although I think they should likely be deprecated). I'd like to discuss your thoughts on this, @gilbarbara, to see if it is something you would consider. It's a large change, so I would want to work closely with you on it. It seems that you have been headed in this direction, by adding run to the props, and other changes you've made.

What do you think?

gilbarbara commented 7 years ago

hey,

I was thinking in a way to remove the refs and use only props but never found a good way to do it especially with the standalone tooltips..

Maybe I should split that in a new module or deprecate it but time is not on my side lately.

IanVS commented 7 years ago

I haven't worked on the standalone tooltips so far, and have only focused on the joyride/tour.

It sounds like you'd be willing to at least review a PR that adds props support to the joyride? So far I've done it in a way that the refs still work, so I don't think the change would be breaking so far.

gilbarbara commented 7 years ago

yeah, definitely. 👍

IanVS commented 7 years ago

I think this issue can't quite be closed yet. The methods next() and reset(), still need to be used, as there is no other way to control the current step index using props. I am submitting a PR which adds a stepIndex prop to address this.