The event onClickMask should receive the current steps

[yanickrochon]

Considering that declaring a TourProvider with specific steps may be modified externally through a hook, for example, the onClickMask handler receive both currentStep and setCurrentStep, but cannot garantee that the steps within that context is the same as when the even is fired.

For example, given my current situation, the tour can only be closed with the mask at the very last step (and will optionally be allowed in other specific cases later on), so I declare my provider like such :

const AppTourProvider = ({ children }) => {
  const defaultSteps = useMemo(() => ([]), []);

  // note: the example specify a 'target` argument, but it is undefined, so I'm using 'document' to fix it
  const handleAfterOpen = useCallback(() => disableBodyScroll(document), []);
  const handleBeforeClose = useCallback(() => enableBodyScroll(document), []);

  const handleClickMask = useCallback(({ setIsOpen, currentStep }) => {
    // defaultSteps is always empty, so this is a useless check
    if (currentStep === defaultSteps.length - 1) {
      setIsOpen(false);      
    }
  }, [defaultSteps]);

  return (
    <TourProvider
      disableInteraction
      disableFocusLock

      steps={ defaultSteps }
      afterOpen={ handleAfterOpen }
      beforeClose={ handleBeforeClose }

      onClickMask={ handleClickMask }
    >
      { children }
    </TourProvider>    
  );
};

In other word, the initial steps is an empty array, and is replaced by various components using the same TourProvider. Unfortunately, once a component calls setSteps somewhere else in the code, the provider is not notified, and I'm not interested to keep a state of this value anyway. Thus, when onClickMask is called, there is an inconsistency between currentStep and defaultSteps.

Therefore, the following line

reactour/packages/tour/src/Tour.tsx

Line 72 in c023981

onClickMask({ setCurrentStep, setIsOpen, currentStep })

Should be changed to

     onClickMask({ setCurrentStep, setIsOpen, currentStep, steps }) 

Point to note, the initial steps should either be optional, or there should be a new prop, called onStepsChanged (for example), added to the available props, which would update the provider's steps once it is changed somewhere else.

[yanickrochon]

In fact, the initial steps are marked as "required" in the documentation, but it's value is an empty array by default, and not mandatory from the prop types declaration. So, my provider can be declared without steps and have onClickMask be called without any knowledge of the steps whatsoever.

[elrumordelaluz]

Hi @yanickrochon, thanks for open the Issue.

Could you please share a sandbox in order to try and understand better the issue.

Basically I'm not sure I am getting the part

Thus, when onClickMask is called, there is an inconsistency between currentStep and defaultSteps.

since steps is an Array of the content and currentStep is only the index.

Thanks in advance,

[yanickrochon]

Hi @elrumordelaluz , I thought it was clear, but here's a minimalist sandbox.

In short, I will have multiple tours in the app, all using the same TourProvider, all initialized from various triggers, each assigning their own steps. However, the TourProvider should only allow closing from the mask once it reaches the end of the tour.

Currently, there is no easy way to know what steps are being used from the TourProvider without implementing an external state manager for that component. The solution is simple, though, just pass steps that's already being passed to the TourProvider down to the onClickMask function so it is available in the handler (as suggested in the original post).

Also note that the prop steps already has a default value of [] within the code

reactour/packages/tour/src/Tour.tsx

Line 15 in c023981

steps = [],

so having it being a required prop is silly, it isn't required since it does not break the component when it isn't provided, and also since it can be set from the hook or the HOC. In other words, steps is not required, but should be set at init time or otherwise before calling setIsOpen(true);

[elrumordelaluz]

Absolutely clear @yanickrochon, thanks for the sandbox!

[elrumordelaluz]

Everything should be available on @reactour/tour@2.8.2

[yanickrochon]

Remember to update the docs, but everything's good! Thanks!

[elrumordelaluz]

Should be updated here and here. Let me know if you think I am missing something, thanks!

[yanickrochon]

@elrumordelaluz steps is still marked as required, other than that, it's OK.

(I just found out about Reactour, this week, but I do plan on forking this repo and actually contribute. I've had a 15 hours days all week, so it's pretty crazy right now. I should have time to make PRs next time.)

[elrumordelaluz]

lol, I added the ?but missed the

required: true

Each PR is pretty welcome, thanks!