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

Added lazy loading for backend form tabs #4658

Merged
merged 7 commits into from
Dec 9, 2019

Conversation

tobias-kuendig
Copy link
Member

Once a backend form has lots of fields with lots of widgets in lots of tabs, the page load time becomes a problem. We did have this problem with our Mall plugin, where the Product edit form is massive.

This problem lead me to try out a new "lazy" property for the tab definition. In it, you can define what tabs you want to load only once they are activated:

tabs:
    lazy:
        - 'Tab 1'
        - 'Tab 2'
        - 'Tab 3'
    fields:
        # ...

This reduced the backend "load" event on the Mall Product form from ~3.8s to ~2.5s on my local dev machine.

What do you guys think? If this can be included into the core I will add the necessary documentation.

lazy-tabs

@w20k
Copy link
Contributor

w20k commented Oct 3, 2019

Looks awesome @tobias-kuendig!

@bennothommo
Copy link
Contributor

@tobias-kuendig Looks incredible, good job! The one thing that springs to mind though - if a person hasn't lazy loaded a tab, will those fields still contribute to the data that is saved? For example, a field with a default value - will that default value still be used during save?

@SebastiaanKloos
Copy link
Contributor

And preset and trigger fields in tabs which are not loaded?

@tobias-kuendig
Copy link
Member Author

Thank you all for the feedback! You have raised some very good points.

By adding hidden input fields for all fields of a lazy loaded tab, we can solve the trigger, preset and default value problems:

By having a hidden field with the default or current value present, the value will sill be sent to the backend for deferred tabs that have not yet been loaded. This results in the same payload with lazy loaded tabs as it does without them.

The same is true for triggers and presets: The trigger and preset JS events will work for the hidden fields as well, so any field that would be disabled by a trigger (and therefore not be sent to the backend) will be disabled now as well.

The only problem is when a "hidden" field is filled by a preset and the user then activates a tab. In this case the previously generated preset will be lost since the part of the form is rebuilt. I circumvented this issue by triggering an input event on all fields that depend on a preset, after a lazy tab was loaded. This, to me, seems like the easiest solution to this problem.

I did some testing of this with the playground plugin. I did not bother to come up with a valid use-case. But if someone wants to tinker around with it, you can use this fork: https://github.com/OFFLINE-GmbH/test-plugin/tree/lazy-loading-tabs

To show how everything works I did temporarily change the hidden fields to default text fields so you can see what the values would be for deferred tabs. Once the tab is loaded these fields are replaced by the original form inputs.
Note that I did slow down the tab loading process on purpose to make it easier to grasp the workflow:

lazy-tabs-2

@bennothommo
Copy link
Contributor

@tobias-kuendig Have you had a chance to test lazy loaded tabs with form widgets that use AJAX handlers - eg. a repeater or media finder field? Do those still work?

@tobias-kuendig
Copy link
Member Author

tobias-kuendig commented Oct 5, 2019

Yes, everything is working as expected ‒ I did, however, find a problem with the "hidden" fields for repeaters since they use arrays of arrays as values.

I'll look into it.

@LukeTowers LukeTowers requested a review from daftspunk October 6, 2019 06:15
@serkanbektas
Copy link

Thanks for development. That seems very good

@LukeTowers LukeTowers added this to the v1.0.461 milestone Oct 9, 2019
Copy link
Member

@daftspunk daftspunk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Thoughtful code and implemented using an opt-in approach

@LukeTowers LukeTowers modified the milestones: v1.0.461, Pending Features Dec 7, 2019
@daftspunk daftspunk modified the milestones: Pending Features, v1.0.461 Dec 9, 2019
@daftspunk daftspunk merged commit 4704f85 into octobercms:develop Dec 9, 2019
daftspunk pushed a commit to octobercms/test-plugin that referenced this pull request Dec 9, 2019
daftspunk added a commit to octobercms/docs that referenced this pull request Dec 9, 2019
@tobias-kuendig tobias-kuendig deleted the lazy-loading-tabs branch December 9, 2019 10:25
$target = post('target');
$tabName = post('name');

$fields = array_get(optional($this->getTab('primary'))->fields, $tabName);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lazy loading works only with primary tabs, throws "Invalid argument supplied for foreach()" on secondary.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@iotch submit a new issue for that please with full details

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to hear from @tobias-kuendig if this is intended

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The original PR did not take secondary tabs into consideration. I have no idea how much effort it would be to implement the feature for them as well.

If they re-use most of the other tab code it might be nothing more than searching the secondary tabs in the line you commented on as well.

@mjauvin
Copy link
Contributor

mjauvin commented Dec 21, 2019

I think the following changes can fix this:

  • add data-tab-section="<?= $type ?>" to the "a" element in modules/backend/widgets/form/partials/_form_tabs.htm:
<a href="#<?= $type . 'tab-' . $index ?>" data-tab-name="<?= e($name) ?>" data-tab-section="<?= $type ?>">
  • add section: $el.data('tab-section') to FormWidget.prototype.bindLazyTabs in modules/backend/widgets/form/assets/js/october.form.js:
data: { 
    target: $el.data('target'),
    name: $el.data('tab-name'),
    section: $el.data('tab-section'),
},      
  • modify the onLazyLoadTab() method in modules/backend/widgets/Form.php to use the new section post data:
$tabSection = post('section');

$fields = array_get(optional($this->getTab($tabSection))->fields, $tabName);

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

Successfully merging this pull request may close these issues.

9 participants