Re: [pve-devel] [PATCH widget-toolkit 1/5] ObjectGrid: optionally show loading on reload

[Thomas Lamprecht <t.lamprecht@proxmox.com>]

looks functional, two higher level comments inline.

On 01/07/2022 16:16, Aaron Lauterer wrote:
> Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
> ---
>  src/grid/ObjectGrid.js | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/src/grid/ObjectGrid.js b/src/grid/ObjectGrid.js
> index b355d6d..3c01851 100644
> --- a/src/grid/ObjectGrid.js
> +++ b/src/grid/ObjectGrid.js
> @@ -48,6 +48,8 @@ Ext.define('Proxmox.grid.ObjectGrid', {
>      // see top-level doc-comment above for details/example
>      gridRows: [],
>  
> +    showReloading: false,

reloading is a bit of a misnormer and the existing `reload` fn name doesn't
help, as we actually only do loads, we don't care if its the first one or
a successive (re-)load one. Besides that, "showing reload" is a bit to generic,
as that could also mean some loading indicator other than masking the whole
component.

Maybe `maskOnLoad` could be a more fitting config name for this (open for
better proposals with above in mind).

> +
>      disabled: false,
>      hideHeaders: true,
>  
> @@ -221,7 +223,16 @@ Ext.define('Proxmox.grid.ObjectGrid', {
>  
>      reload: function() {
>  	let me = this;
> -	me.rstore.load();
> +	let param;
> +	if (me.showReloading) {
> +	    me.setLoading();
> +	    param = {
> +		callback: function() {
> +		    me.setLoading(false);
> +		},
> +	    };

I'd avoid the param variable and just to an if/else with passing the config
object directly. Using an arrow fn makes it also a bit shorter, such a simple
object can even be placed in a single line (but no hard feelings on that
one), e.g.:

me.rstore.load({ callback: () => me.setLoading(false) });

> +	}
> +	me.rstore.load(param);
>      },
>  
>      getObjectValue: function(key, defaultValue) {