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

Performance issue when opening the widget admin page #8749

Open
gdessimoneinvalle opened this issue Jan 23, 2024 · 6 comments
Open

Performance issue when opening the widget admin page #8749

gdessimoneinvalle opened this issue Jan 23, 2024 · 6 comments

Comments

@gdessimoneinvalle
Copy link

var widgets = _widgetsService.GetWidgets();

When many content pages contain widgets we experience perfomance issues as the widget admin page takes even some minutes to open

@BenedekFarkas
Copy link
Member

BenedekFarkas commented Feb 26, 2024

Continuing from #8750:

A generic-purpose solution to this could be to optionally move the layer filtering to the server-side. Right now, when you open the Widgets page and select any layer, you still see the widgets from each layer and you can hide/show the widgets of each layer on the client-side using the layer list on the right side.

This could be overridden for example by adding a site setting, which, when enabled would filter the displayed widgets down to the selected layer only by using an override of GetWidgets (with currentLayer.Id passed into it) at the place highlighted by @gdessimoneinvalle above.
Since this would make the client-side filtering useless, that should be hidden too. A new, "Show all" option (with value 0) needs to be added to the layer dropdown too.

It can be taken one step further by the above site setting just providing a default value and the Widgets screen having an additional checkbox for the same purpose, but its value stored in a cookie, so admins can switch between the two behaviors for themselves.

CC: @MatteoPiovanelli-Laser

@MatteoPiovanelli-Laser
Copy link
Contributor

Just for the sake of discussion, the things we could filter widgets on for that action are:

  1. Layer: this is what we are discussing, and I think this is what makes the most sense to have in a site setting.
  2. Zone: the usefulness of this in the context of this controller/action is debatable. Personally, I think the job of this controller is to display everything, but, since it's grouping things by Zone anyway, it may be worth it to set that up as a filter. Overall, I think this would be overengineering this feature.
  3. ContentType: I think it makes little sense to have a setting to filter this page for this.

So I think the site setting should definitely let admins select default layers whose widgets are loaded/shown; probably nothing else.
I think the UI for client side filtering may still be there, but rather than operate entirely client-side, it may call a server-side api for the updated information.

I like the approach of using the cookie for admins so they may override their own setting, but I'm not sure I'd actually implement it, unless there is a trivial way to handle this:
Suppose the default (site setting) is to show Layers 1 and 2, and that leads to a fast load for that page. As and admin, I may need to check something from one of the "slow" Layers: if that is saved as my cookie, the next time I may want to check something, I'm subjected to the slow load time; this will likely repeat itself several times until I happen to change the setting/cookie for myself. So interacting once with the slow layer may end up degrading the UX making it worse and making it feel like the rest of the changes we are proposing aren't even there.

@BenedekFarkas
Copy link
Member

BenedekFarkas commented Feb 27, 2024

What I meant by site setting is just a simple checkbox: When enabled, the filtering happens entirely on the server-side based on the dropdown and the right-side list on the Widgets page for client-side filtering is hidden, because it becomes dysfunctional.

To expand on that, the cookie-based setting is also just a checkbox that takes precedence over the site setting.

@MatteoPiovanelli-Laser
Copy link
Contributor

I see.
I would have, in the Site Setting, the list of all Layers, for the admin to select which ones are to have their widgets displayed/hidden by default in the list.
Filtering happens server-side.
Then in the Widgets page I'd keep the UI that's there to filter, but change what it does: rather than hide/display something that's already client-side, it would call a server-side api to update the list.

@BenedekFarkas
Copy link
Member

That sounds good in principle, although I'd advise against such an overhaul of the UI unless you can keep the current behavior going (because it's not a common problem) and you really really need it to work like this.

On the other hand, you can address the performance issue by adding just a single checkbox setting and 1 line changed in the controller.

@sebastienros
Copy link
Member

What does OC do? I believe we designed it to be efficient, and nobody has complained so far.

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

No branches or pull requests

4 participants