From: Thomas Lamprecht <t.lamprecht@proxmox.com>
To: Proxmox VE development discussion <pve-devel@lists.proxmox.com>,
Aaron Lauterer <a.lauterer@proxmox.com>
Subject: Re: [pve-devel] [PATCH widget-toolkit 1/5] ObjectGrid: optionally show loading on reload
Date: Tue, 5 Jul 2022 11:32:20 +0200 [thread overview]
Message-ID: <6d3d569f-57fe-ac85-4465-cad029cc188a@proxmox.com> (raw)
In-Reply-To: <20220701141642.2743824-2-a.lauterer@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) {
next prev parent reply other threads:[~2022-07-05 9:32 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-07-01 14:16 [pve-devel] [PATCH widget-toolkit/manager 0/5] Ceph OSD: add detail infos Aaron Lauterer
2022-07-01 14:16 ` [pve-devel] [PATCH widget-toolkit 1/5] ObjectGrid: optionally show loading on reload Aaron Lauterer
2022-07-05 9:32 ` Thomas Lamprecht [this message]
2022-07-01 14:16 ` [pve-devel] [PATCH manager 2/5] api ceph osd: add OSD details endpoint Aaron Lauterer
2022-07-01 14:16 ` [pve-devel] [PATCH manager 3/5] api ceph osd: add volume " Aaron Lauterer
2022-07-05 9:58 ` Thomas Lamprecht
2022-07-05 14:19 ` Aaron Lauterer
2022-07-06 6:37 ` Thomas Lamprecht
2022-07-01 14:16 ` [pve-devel] [PATCH manager 4/5] ui utils: add renderer for ceph osd addresses Aaron Lauterer
2022-07-01 14:16 ` [pve-devel] [PATCH manager 5/5] ui: osd: add details window Aaron Lauterer
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=6d3d569f-57fe-ac85-4465-cad029cc188a@proxmox.com \
--to=t.lamprecht@proxmox.com \
--cc=a.lauterer@proxmox.com \
--cc=pve-devel@lists.proxmox.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox