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

Allow props to be overriden by steps #170

Closed
IanVS opened this issue Jan 4, 2017 · 9 comments
Closed

Allow props to be overriden by steps #170

IanVS opened this issue Jan 4, 2017 · 9 comments
Labels

Comments

@IanVS
Copy link
Contributor

IanVS commented Jan 4, 2017

In some cases, its necessary to change the Joyride props only for particular steps. For example, in some of my steps, I want to show an overlay, but not in other steps. Rather than changing the <Joyride showOverlay /> prop, it would be great to specify in the steps array itself:

const steps = [{
  title: 'step1',
  target: '.myclass',
  showOverlay: false,
}]

In this case, the value specified in the step would have priority over any props provided to the Joyride. Other props which I think would be good to be able to override:

  • locale
  • holePadding
  • scrollOffset
  • scrollToStep
  • showBackButton
  • showSkipButton
  • disableOverlay

And maybe a few others. What do you think about allowing properties like that to be specified on steps to override the global props of the Joyride? I can immediately submit PRs for at least holePadding and scrollToStep.

@gilbarbara
Copy link
Owner

LGTM.
However I think it will be better to add support to all props at once..

@gilbarbara
Copy link
Owner

Looking at the code today it will need to change a lot of code for all options to work. Maybe it is better to add just the big ones for now?
And pass these options inside a props property instead of the root step? I think it will be easier to document.

Thoughts?

@IanVS
Copy link
Contributor Author

IanVS commented Jan 8, 2017

Hm, personally it doesn't bother me to have a flat structure, since props is short for properties, and a step is just an object with properties. :) I like the idea of either specifying them on the joyride or directly on the step (kind of similar to "Setting tour options" in http://linkedin.github.io/hopscotch/).

I haven't looked at what it would take to override all of them. The ones I mentioned are basically one-line changes. But, it would be nice to (eventually?) generalize the logic so that whenever a prop is needed, we first check the current step, and then the Joyride props. Having that check in one method instead of spread around would be nice and clean.

@IanVS
Copy link
Contributor Author

IanVS commented Jan 8, 2017

As for documenting the options, I think that's just a matter of organizing the README and explaining it. I can take a shot at it, and if you think it's unclear we can put them in a props if you prefer, how about that?

@gilbarbara
Copy link
Owner

Let's go flat. I need to rewrite the README anyway.
:)

@IanVS
Copy link
Contributor Author

IanVS commented Jan 8, 2017

Speaking of step properties, I have some questions for you, do you use gitter?

@gilbarbara
Copy link
Owner

Yeah, sometimes.. But it's almost 4AM here, I'll crash now. 😴
You can add me in Gtalk too, I'm always online there. gilbarbara@gmail.com

@gilbarbara
Copy link
Owner

Or plain email too..

@gilbarbara gilbarbara mentioned this issue Dec 27, 2017
44 tasks
@gilbarbara gilbarbara added this to the V2 milestone Dec 27, 2017
@gilbarbara
Copy link
Owner

All tour props can be overridden in V2
npm i react-joyride@next

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants