Implement global lazy loadable support for tabs

[lekoala]

For context, see issue
#1308

[lekoala]

@michalkleiner sorry my bad, this is what happens when you simply edit stuff through github ;-)

[GuySartorelli]

Looks good in general - the approach is nice and general, and it's resolving some problems with lazy-loadable gridfields that probably have been unnoticed (or just ignored) for a while.

The new .lazy-loadable class and the native event should be added to the relevant documentation and there are a few other fairly small changes here to make.

[GuySartorelli]

Please also run yarn build and commit the dist files to make the CI happy.

[lekoala]

@GuySartorelli hey, sorry, this is probably the worst pull request ever. I did work from the uncompiled source because I have issues running yarn build locally. I'm reviewing things with a fresh mind to make sure it's more like a proper PR :-)

[lekoala]

@GuySartorelli so I gave this another shot. There is a confusing part for me. There are currently two different entwine bindings, one for .ss-tabset, and one for both .ss-tabset and .cms-tabset. But as far as I can tell, this serves no purpose, since lazy loading should apply to both and the redraw tabs feature is guarded by a class check. I've just refactored the whole thing to merge them together... it should be working well this way
I've also applied your suggestions and kept both functions.

I didn't manage to run yarn build, i get this error
error silverstripe-admin@4.0.0: The engine "node" is incompatible with this module. Expected version "^10.x". Got "16.15.0"
that makes no sense to me since 16 is clearly higher than 10. And I tried with an older version, but then I got some errors about using a deprecated version.

[michalkleiner]

Expected version "^10.x". Got "16.15.0". That makes no sense to me since 16 is clearly higher than 10.

^10.x is weird, but essentially means any minor on 10.x.
16 doesn't meet that and I would suspect 16.x not working if other deps weren't also updated/made compatible.
It's simply not safe to just use that many major versions newer Node.
12.x might still work.

[lekoala]

@michalkleiner ah makes sense
it might still be worth an upgrade. trying to run with version 10 using nvm i get this
error @eslint/eslintrc@1.3.0: The engine "node" is incompatible with this module. Expected version "^12.22.0 || ^14.17.0 || >=16.0.0". Got "10.24.1"

supporting newer major versions might be a hassle, but staying on older versions might be just as bad...

i mean, if there is a full guide somewhere on how to use the build tools, I'm all for it, but installing node 10 and running yarn build doesn't work (at least on windows with the way i've installed things)

[kinglozzer]

I’m pretty sure our client-side build process only works with Node 10. FWIW I use nvm to handle switching between Node versions for different projects, we already have an .nvmrc file in this repo so running nvm use in the directory should be enough looks like you already have nvm 😅

@lekoala after switching to node 10, did you rm -rf node_modules && yarn?

[lekoala]

@kinglozzer thanks for your help on this

nvm use gives me "invalid version" as a result (maybe windows specific?). still, i get pick my version and use a valid branch

removing node_modules was a good idea, but it doesn't solve the issue

i get this

yarn build
yarn run v1.22.18
$ yarn && yarn lint && yarn test && NODE_ENV=production webpack -p --bail --progress
[1/5] Validating package.json...
[2/5] Resolving packages...
[3/5] Fetching packages...
[4/5] Linking dependencies...
warning " > bootstrap@4.6.1" has unmet peer dependency "jquery@1.9.1 - 3".
warning " > react-router-config@4.4.0-beta.8" has incorrect peer dependency "react-router@4.4.0-beta.1".
warning " > @storybook/react@3.4.12" has unmet peer dependency "babel-core@^6.26.0 || ^7.0.0-0".
warning " > babel-jest@23.6.0" has unmet peer dependency "babel-core@^6.0.0 || ^7.0.0-0".
warning " > storybook-addon-jsx@5.4.0" has unmet peer dependency "babel-core@^6.26.0".
[5/5] Building fresh packages...
$ eslint client/src && sass-lint -v
The system cannot find the path specified.
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
error Command failed with exit code 1.

so it seems it's either eslint or sass-lint that doesn't get installed. yarn add sass-lint works fine, but yarn add eslint doesn't work and gives: error @eslint/eslintrc@1.3.0: The engine "node" is incompatible with this module. Expected version "^12.22.0 || ^14.17.0 || >=16.0.0". Got "10.24.1" error Found incompatible module.
not even sure why it's not there in the first place when doing a regular yarn install (which doesn't trigger any error). is eslint installed globally or something like that ?

I've tried with node v12, it doesn't work due to engine restriction. I really think upgrading to at least node 12 would make sense as eslint doesn't seem to support at all node v10.

[GuySartorelli]

Node 10 works fine for me, eslint and all. I don't undertand why nvm heckles you about the version... did you nvm install 10 before nvm use?

if there is a full guide somewhere on how to use the build tools, I'm all for it

This is something we're aware it pretty lacking at the moment. But improving documentation is a lengthy process and nobody has made time to do it yet.

I really think upgrading to at least node 12 would make sense

I agree in general - but this isn't something we're going to look at doing in the immediate short term.

[GuySartorelli]

@lekoala I've built the js and pushed it to your PR branch

[GuySartorelli]

Finally got around to taking a look at this post-refactor. It looks like some of the functionality (onremove, redrawTabs, rewriteHashLinks, etc) may have been intentionally omitted from .cms-tabset.

silverstripe-admin/themes/cms-forms/templates/SilverStripe/Forms/CMSTabSet.ss

Line 1 in eb192f4

<%-- Exclude ".ss-tabset" class to avoid inheriting behaviour --%>

Can you please undo the refactoring? The only change to .cms-tabset should be that it now includes lazy loading.

[lekoala]

@GuySartorelli ok so I made this dead simple, i simply add my new function to cms-tabset and ss-tabset (so that it works everywhere and doesn't break anything).
alternative option, since we don't change anything, it can be up to anyone to simply include such script if they really need that kind of feature (i came up with the idea when working for a gridfield replacement in my upcoming module, and I provide this option in the meantime
https://github.com/lekoala/silverstripe-tabulator/blob/master/client/admin.js)

it really depends i guess if you think it's worthwhile to support this as a core feature or not

[GuySartorelli]

Sorry it's taken me so long to look at this again.

I still think it makes sense for lazyLoadGridFields to be moved into the $('.ss-tabset, .cms-tabset') entwine set - I had intended this when I said "The only change to .cms-tabset should be that it now includes lazy loading" but I should have been more explicit about it.
I'm certain that was intended, since .cms-tabset is checked inside that function and the docs say that's the class that should explicitly work with lazy loading.

If you can make that change along with the other small changes I've requested, I'll build the bundle for you again, test locally, and we can let CI do its thing - if that passes then I'll be happy with it.

We need docs for this as well as I've mentioned before, but that will be in the framework repository anyway so I'm okay so it has to be a separate PR, so I'm okay with merging this PR without docs for now - it would be super cool if you can create a new PR in framework to add docs, but I'm happy to do it if you don't.

[lekoala]

@GuySartorelli when I started working on the PR, I thought it would be useful as a global feature (because it would be logical to offer this feature for other types of fields if a generic way) and i didn't imagine it would be difficult to run the build pipe ;-) now actually i realize that maybe its also overcomplicating things.
If someone need that kind of feature, they can include an additional LeftAndMain script doing something along these lines
https://github.com/lekoala/silverstripe-tabulator/blob/master/client/admin.js
It's working pretty well, and maybe it's not needed to build this as a core feature that is moving towards react anyway (not sure how my approach play along with that).

:-) but if you find the PR interesting, maybe it's easier that you proceed and modify things yourself, you already did much of it anyway and it will be easier to run the build pipe and make sure styling is consistent with your requirements. What do you think?

[GuySartorelli]

when I started working on the PR, I thought it would be useful as a global feature

I agree, it sounds like something that would be useful to have in core for sure.

i didn't imagine it would be difficult to run the build pipe ;-)

Like I said, happy to take care of that part for you.

now actually i realize that maybe its also overcomplicating things.
If someone need that kind of feature, they can include an additional LeftAndMain script

I don't think it's overcomplicated at all. Technically just about anything can be added to a project with some extra code. ;P I think this change makes sense in core though.

that is moving towards react anyway

There hasn't been any real attempt to convert entwine functionality to react for quite a while, and I don't see it being in the works any time soon either.

if you find the PR interesting, maybe it's easier that you proceed and modify things yourself

I think it's pretty close to done, there's just the suggested changes I've mentioned above. If you'd rather not do them that's fine, though - I understand this all takes time so no pressure either way. If you want to close the PR, I might pick it up at some point, otherwise if you want to make those changes then as I've said if it passes testing I'll be happy to merge it.

[lekoala]

@GuySartorelli mhh don't mind me, i'm sometimes a bit disappointed by how much time it takes to follow up a PR that would probably take no more than 10 minutes for a solo dev :-)
I've merged the changes you requested and moved the function to the entwine set, it all looks good to me

I'd be happy to make a PR to the docs once it's merged

[GuySartorelli]

Looks good, works well locally. Thank you for this, it's a great idea.
Feel free to @ me when you create the docs PR and I'll take a look.