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

make lazy-loadable a global feature #1308

Closed
lekoala opened this issue May 18, 2022 · 8 comments · Fixed by #1309
Closed

make lazy-loadable a global feature #1308

lekoala opened this issue May 18, 2022 · 8 comments · Fixed by #1309

Comments

@lekoala
Copy link
Contributor

lekoala commented May 18, 2022

Currently, if you have lazy content inside tabs, you need to implement custom js code to handle this
Which is a bit of a shame, because there is already some code related to this for the gridfield

lazyLoadGridFields: function(panel) {
panel.find('.grid-field--lazy-loadable').each((i, el) => {
const gridfield = $(el);
// Avoid triggering all gridfields when using nested tabs
if (gridfield.closest('.ss-tabset, .cms-tabset').is(this)) {
$(el).lazyload();
}
});
}

Would the ss dev consider making looking for any ".lazy-loadable" node and trigger a lazy-load event on it ? Actually, it could also be applied to the gridfield. The current implementation feels like a missed opportunity to open this feature for other fields.

@lekoala
Copy link
Contributor Author

lekoala commented May 18, 2022

the following snippet is a proof of concept

    function triggerLazyLoad(el) {
      el.find(".lazy-loadable").each(function (idx, ele) {
        ele.dispatchEvent(new Event("lazyloaded"), { once: true });
      });
    }

    $(".ss-tabset, .cms-tabset").entwine({
      onadd: function () {
        this.on(
          "tabsactivate",
          function (event, { newPanel }) {
            triggerLazyLoad(newPanel);
          }.bind(this)
        );
        this.on(
          "tabscreate",
          function (event, { panel }) {
            triggerLazyLoad(panel);
          }.bind(this)
        );
        this._super();
      },
    });

@GuySartorelli
Copy link
Member

Sounds like a great idea. Would you be open to raising a PR implementing this feature?

@lekoala
Copy link
Contributor Author

lekoala commented May 19, 2022

@GuySartorelli i can certainly give it a try, the basic idea is quite simple and yet has many advantages :)

@lekoala
Copy link
Contributor Author

lekoala commented May 23, 2022

@GuySartorelli so, i'm working on it and i've discovered that lazy loading is only delayed if there is a parent ss-tabset.. But the cms-tabset doesn't have that class (and that seems to be a deliberate choice).

(this.closest('.ss-tabset').length === 0) || (this.data('gridfield-lazy-load-state') === 'force') )

That means that for a GridFieldDetailForm, lazy loading of the GridField is always triggered right away. Is that the desired outcome ?

@lekoala
Copy link
Contributor Author

lekoala commented May 23, 2022

@GuySartorelli so I gave it a try
#1309

this is a fairly conservative PR, since i keep both ways of doing things.
fixing the cms-tabset selector seems like a nice bonus, since I don't believe excluding it was intended in this case as it defeats the initial purpose.

@GuySartorelli GuySartorelli linked a pull request May 24, 2022 that will close this issue
@GuySartorelli
Copy link
Member

GuySartorelli commented May 24, 2022

i've discovered that lazy loading is only delayed if there is a parent ss-tabset.. But the cms-tabset doesn't have that class (and that seems to be a deliberate choice). ... fixing the cms-tabset selector seems like a nice bonus, since I don't believe excluding it was intended in this case

According to the docs the cms-tabset class is actually the class that should explicitly work, so I'd say you've found a bona fide defect:

Lazy loading works based on the href attribute of the tab navigation. The base behaviour is applied through adding a class .cms-tabset to a container.

I'll take a look at the PR today and let you know if it needs any changes.

@lekoala
Copy link
Contributor Author

lekoala commented Jul 5, 2022

@GuySartorelli fyi, because I gave it a bit more thought and you might be interested (eg: maybe replace entwine some day? :-) who knows!), I came up with a creative solution that allows lazy loading without any need for this feature. The solution is simply to rely on the intersection observer, that will trigger when the tab is activated (which is nice, because the tab doesn't have the responsibility to init things itself). In order to make this convenient, I created a custom element that can wrap anything and basically allow lazy loading and field init automatically.
The silverstripe module is here
https://github.com/lekoala/silverstripe-modular-behaviour
The lib itself is here
https://github.com/lekoala/modular-behaviour.js
And I just gave it a try in my gridfield replacement module, and i'm quite happy with the results. This way I can create fields that work just the same in the frontend and in the admin without having to build some entwine specific code, an issue I've been trying to solve for quite some time.

@GuySartorelli
Copy link
Member

Awesome! I don't have time to look in depth into that code right now but having stuff work the same on the front and backends is definitely a worthwhile goal.
Replacing entwine would definitely be nice... unfortunately as the team learned when they tried to replace it with react it's a really big rabbit hole :p It's something we'd all like but it's not something we have any specific plans for at the moment.

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

Successfully merging a pull request may close this issue.

3 participants