Lazy evaluation of `attachTo.element`

zefj commented 4 years ago

Hi, I came up with an improvement for the library, which would greatly simplify the ability to tour dynamic content.

The issue

Right now, elements used for attachTo.element either have to be present in the DOM upon step instantiation, or a string selector has to be used, which makes it impossible to do stuff like this:

tour.addStep({
    text: '...',
    attachTo: {
        element: getElementToAttachTo,
        on: 'auto',
    },
});

const getElementToAttachTo = () => {
    // may return elements based on some heuristic, eg. a random day in a calendar, particular element based on previous user actions, etc.
    ...
};

Having the possiblity to lazily evaluate attachTo.element would mean shepherd could be used to tour dynamic content. One of the use-cases is reacting to user actions during the tour: having lazy evaluation, the DOM could be modified as the tour is running, and those changes could be toured as well. Right now, it isn't really possible.

The proposal

I propose changing the StepOptionsAttachTo interface to this:

interface StepOptionsAttachTo {
-  element?: HTMLElement | string;
+  element?: HTMLElement | string | (() => HTMLElement | string | null | undefined);
  on?: PopperPlacement;
}

Which basically comes down to attachTo.element accepting a function as well. We would have to modify the Step class a little bit to evaluate the target element only once, preferably at the very beginning of Step._show, and keep the reference in the Step instance, but other than that I don't see any issues.

I have a working prototype (it does not yet do the single-evaluation mentioned above, would have to add that), would you be interested in such a feature?

RobbieTheWagner commented 4 years ago

@zefj this is already possible using beforeShowPromise, but your suggestion may be a simpler API.

zefj commented 4 years ago

@rwwagner90 true, but that means mutating the step options in beforeShowPromise, which makes the tour code that much harder to test and reason about, and also relying on implicit this. I could live with the latter, but options immutability alone is a very good reason to introduce the new API. That, and API simplicity.

I've already done some work here, but like I said, I still need to make sure we only evaluate the callback once (as in, parseAttachTo has to be called at most once). The only potential trouble I see is isCentered being a public method, which may trigger premature evaluation if called manually, but that isn't an issue we can't solve with good documentation.

I'll make changes and cook up a merge request tomorrow, we'll decide whether we want to go on with it or not.