mint-metrics / mojito-js-delivery

🧪 Source-controlled JS split testing framework for building and launching A/B tests.
https://mojito.mx/docs/js-delivery-intro
Other
16 stars 30 forks source link

Make veil work with the new trigger system #34

Closed kingo55 closed 4 years ago

kingo55 commented 4 years ago

Currently veil does not work with the current trigger system because it was designed to support a wider variety of timings that our veil codebase was designed to handle.

But I think there might be a simple solution for Veil, we could bake it into the trigger system. We could define a class, like:

._mojitoVeil {
    display: none;
}

And inside the trigger we could use logic like:

function trigger(test)
{
    if (isTestPage) {
        document.documentElement.classList.add('_mojitoVeil');
        window.setTimeout(function(){document.documentElement.classList.remove('_mojitoVeil');},3000);
        Mojito.utils.domReady(function(){
            document.documentElement.classList.remove('_mojitoVeil');
            test.activate();
        });
    }
}

Clearly we could provide some helper functions to abstract this away. Would this be an effective option?

dapperdrop commented 4 years ago

Makes sense, @kingo55

We should remove the existing veil code inside the library as it rarely gets used, and it hasn't been documented.

I'm not sure helper functions should replace what you've suggested above as I don't foresee it to be widely used enough to justify the extra lines in the library.

Instead we should probably add it as a pattern (to help users overcome flickering) to our documentation. We'll need to consider things like a fallback recipe (previously bucketed vs new to test) and the need for manual exposure.

edit: structure/grammar

kingo55 commented 4 years ago

True... We should consider the threshold for utils we include in the lib vs what get included via shared code.

I'll add our old veil functions to the other issue. And we can document with an example pattern on the https://mojito.mx/ site.