gilbarbara / react-joyride

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

Custom `tooltipComponent` doesn't receive all relevant props. #405

Closed MilllerTime closed 5 years ago

MilllerTime commented 6 years ago

Expected behavior

In the Step object doc, there are a number of useful properties that can be set to alter the tooltip, such as hideBackButton and showSkipButton. I would expect these to be provided to a custom tooltipComponent so that it can replicate the standard behavior.

Actual behavior

The props I mentioned above are not provided to a custom tooltip, so it's not possible to, for example, hide the back button per step when using a custom tooltip.

A quick test reveals that as of v2.0.0-13, the following props are provided to the custom tooltip component:

title and content from the step definition are there, but not other step features like hideBackButton. I checked if backProps contained a visible property or similar, but it does not. It would be great if all step properties were provided to custom tooltips. Thanks!

React-Joyride version

2.0.0-13

MilllerTime commented 6 years ago

There is also no style information passed to the custom tooltip. I realize it's not as critical that style attributes like background color are passed though, since you can already render the custom tooltip in any way. However, it's not possible to change the width of the tooltip on a per-step basis without passing through some of the styles. Changing the width on different steps is a great feature of the default tooltip, and I'd like to have that ability with custom tooltips as well. I have a tour in particular that would benefit from this. The other style information would be a nice to have, too.

Is there any reason why style data is not being passed to custom tooltip components?

MilllerTime commented 6 years ago

I just dug in a little deeper on this. The list of render props provided to a custom tooltip appears to be deliberately different than the props on the default tooltip.

I'd submit a PR to sync these, but it wouldn't be backwards compatible (simply passing a step prop instead of content, title, etc would break existing custom tooltips). I assume there is a reason it is designed this way? Is there anything we can do about this? Thanks again.

gilbarbara commented 6 years ago

The idea behind the custom tooltip is that you decide what to show or not.. I won't change the render props that are currently being passed down but I could add some other pieces. Passing the step besides the render props would work for your needs?

MilllerTime commented 6 years ago

Passing the step besides the render props would work for your needs?

Yeah, if all step data is accessible, that would be great!

The idea behind the custom tooltip is that you decide what to show or not..

I hear you on this. However, I'd like to avoid having a <TooltipWithBackButton>, <TooltipWithoutBackButton>, <TooltipNarrow>, <TooltipWide>, etc. I could create a higher-order component to produce these variants on the fly. It's just that you already have a documented approach to customizing the default tooltip, and I'd like my custom tooltip to be a drop-in replacement without requiring other changes and a different API.

gilbarbara commented 6 years ago

I'm not sure what to say. The idea of custom tooltips is to give flexibility to users...

MilllerTime commented 6 years ago

Right, aren't we saying the same thing? 🙂 I think providing the full step object to a custom tooltip, instead of a limited subset, offers the ultimate flexibility to users.

gilbarbara commented 5 years ago

Fixed in 3135983ff1db708a4f2efe712576a6cc6ec353fc