[pbs-devel] [PATCH proxmox-backup] fix: ui: spinner position

[pbs-devel] [PATCH proxmox-backup] fix: ui: spinner position

[Gabriel Goller]

When displaying the status of a job (sync, gc, etc..) the 'OK'
and 'Error' text uses a padding of 10px to the left. The
loading spinner is centered in the cell though. This doesn't
look that good.

Signed-off-by: Gabriel Goller <g.goller@proxmox.com>
---
 www/css/ext6-pbs.css | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/www/css/ext6-pbs.css b/www/css/ext6-pbs.css
index 5fd65d25..4a4a41be 100644
--- a/www/css/ext6-pbs.css
+++ b/www/css/ext6-pbs.css
@@ -81,7 +81,7 @@
 
 /* loading in task list */
 .x-grid-row-loading {
-    background: no-repeat center center;
+    background: no-repeat left 10px center;
     background-image:url(../extjs/theme-crisp/resources/images/loadmask/loading.gif);
 }
 
-- 
2.39.2

Re: [pbs-devel] [PATCH proxmox-backup] fix: ui: spinner position

[Gabriel Goller]

bump.

On 9/13/23 16:20, Gabriel Goller wrote:
> When displaying the status of a job (sync, gc, etc..) the 'OK'
> and 'Error' text uses a padding of 10px to the left. The
> loading spinner is centered in the cell though. This doesn't
> look that good.
>
> Signed-off-by: Gabriel Goller <g.goller@proxmox.com>
> ---
>   www/css/ext6-pbs.css | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/www/css/ext6-pbs.css b/www/css/ext6-pbs.css
> index 5fd65d25..4a4a41be 100644
> --- a/www/css/ext6-pbs.css
> +++ b/www/css/ext6-pbs.css
> @@ -81,7 +81,7 @@
>   
>   /* loading in task list */
>   .x-grid-row-loading {
> -    background: no-repeat center center;
> +    background: no-repeat left 10px center;
>       background-image:url(../extjs/theme-crisp/resources/images/loadmask/loading.gif);
>   }
>   

Re: [pbs-devel] [PATCH proxmox-backup] fix: ui: spinner position

[Dominik Csapak]

On 9/13/23 16:20, Gabriel Goller wrote:
> When displaying the status of a job (sync, gc, etc..) the 'OK'
> and 'Error' text uses a padding of 10px to the left. The
> loading spinner is centered in the cell though. This doesn't
> look that good.

this is debatable, and having the 'loading' state stand out, makes sense imo.

*if* we want to change this, we either would have to adapt pve/pmg as well,
or refactor the class into the widget-toolkit (idk if there are some differences
across products, though it shouldn't be), because we also use that for the task
and job status list (e.g. replication)

> 
> Signed-off-by: Gabriel Goller <g.goller@proxmox.com>
> ---
>   www/css/ext6-pbs.css | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/www/css/ext6-pbs.css b/www/css/ext6-pbs.css
> index 5fd65d25..4a4a41be 100644
> --- a/www/css/ext6-pbs.css
> +++ b/www/css/ext6-pbs.css
> @@ -81,7 +81,7 @@
>   
>   /* loading in task list */
>   .x-grid-row-loading {
> -    background: no-repeat center center;
> +    background: no-repeat left 10px center;
>       background-image:url(../extjs/theme-crisp/resources/images/loadmask/loading.gif);
>   }
>   

Re: [pbs-devel] [PATCH proxmox-backup] fix: ui: spinner position

[Gabriel Goller]

On 10/12/23 11:28, Dominik Csapak wrote:

> On 9/13/23 16:20, Gabriel Goller wrote:
>> When displaying the status of a job (sync, gc, etc..) the 'OK'
>> and 'Error' text uses a padding of 10px to the left. The
>> loading spinner is centered in the cell though. This doesn't
>> look that good.
>
> this is debatable, and having the 'loading' state stand out, makes 
> sense imo.
>
No hard feelings from me either to be honest. It just doesn't look that 
good (imo)
when having a short text and long rows (as in the new gc view).

> *if* we want to change this, we either would have to adapt pve/pmg as 
> well,
> or refactor the class into the widget-toolkit (idk if there are some 
> differences
> across products, though it shouldn't be), because we also use that for 
> the task
> and job status list (e.g. replication)

I looked at them briefly and they have a few things in common, but imo
it's not worth the hassle combining them. We wouldn't **need** this in
pve/pmg but I could still submit a patch... let me know.

>
> [..]

Re: [pbs-devel] [PATCH proxmox-backup] fix: ui: spinner position

[Dominik Csapak]

On 10/12/23 13:49, Gabriel Goller wrote:
> On 10/12/23 11:28, Dominik Csapak wrote:
>> *if* we want to change this, we either would have to adapt pve/pmg as well,
>> or refactor the class into the widget-toolkit (idk if there are some differences
>> across products, though it shouldn't be), because we also use that for the task
>> and job status list (e.g. replication)
> 
> I looked at them briefly and they have a few things in common, but imo
> it's not worth the hassle combining them. We wouldn't **need** this in
> pve/pmg but I could still submit a patch... let me know.
> 
>>

Depends on your definition of **need**. If we don't do this,
it would look differently e.g. for the task list in pve/pmg vs pbs
and we want to have the UI consistent across products
(except if there is a good reason to diverge) so for me it is
important to stay consistent here.

@Thomas any opinion on this?

Re: [pbs-devel] [PATCH proxmox-backup] fix: ui: spinner position

[Gabriel Goller]

On 10/12/23 14:34, Dominik Csapak wrote:

> [..]
> Depends on your definition of **need**. If we don't do this,
> it would look differently e.g. for the task list in pve/pmg vs pbs
> and we want to have the UI consistent across products
> (except if there is a good reason to diverge) so for me it is
> important to stay consistent here.
>
IIRC we don't have any long rows (meaning tables with single
columns or such) in pve/pmg (at least I have never seen them).
So it won't be a big (or even visible change in pve/pmg). But I
understand your argument of UI (and code) consistency in
between products.

Re: [pbs-devel] [PATCH proxmox-backup] fix: ui: spinner position

[Thomas Lamprecht]

Am 12/10/2023 um 14:34 schrieb Dominik Csapak:
> On 10/12/23 13:49, Gabriel Goller wrote:
>> On 10/12/23 11:28, Dominik Csapak wrote:
>>> *if* we want to change this, we either would have to adapt pve/pmg as well,
>>> or refactor the class into the widget-toolkit (idk if there are some differences
>>> across products, though it shouldn't be), because we also use that for the task
>>> and job status list (e.g. replication)
>>
>> I looked at them briefly and they have a few things in common, but imo
>> it's not worth the hassle combining them. We wouldn't **need** this in
>> pve/pmg but I could still submit a patch... let me know.
>>
>>>
> 
> Depends on your definition of **need**. If we don't do this,
> it would look differently e.g. for the task list in pve/pmg vs pbs
> and we want to have the UI consistent across products
> (except if there is a good reason to diverge) so for me it is
> important to stay consistent here.
> 
> @Thomas any opinion on this?

I find Gabriel has a point, it looks odd as is, and aligning it right
makes it better fit in with other states like "OK" or "Error: ..".

Having something like "Running" as text here would be nice, but  we
cannot just use a `.x-grid-row-loading:after { content: 'Running' }`
as that misses translations.

One option would be to avoid ExtJS CSS here and use font-awesome like
"fa fa-fw fa-circle-o-notch fa-spin", the only disadvantage from that
is that we lose the site-wide synced animation that the currently used
GIF provides, can be worked around but IIRC there's nothing native
available for doing so.

For now the adapted CSS would be an OK stop-gap, IIRC we only use such
grid loading classes for such scheduled-job grids. Adding it to PVE
(isn't there even anything besides replication using that?) would be
good too though. IMO no need for widget toolkit, I'd keep this contained
for now.

Re: [pbs-devel] [PATCH proxmox-backup] fix: ui: spinner position

[Dominik Csapak]

On 10/13/23 14:38, Thomas Lamprecht wrote:
> Am 12/10/2023 um 14:34 schrieb Dominik Csapak:
>> On 10/12/23 13:49, Gabriel Goller wrote:
>>> On 10/12/23 11:28, Dominik Csapak wrote:
>>>> *if* we want to change this, we either would have to adapt pve/pmg as well,
>>>> or refactor the class into the widget-toolkit (idk if there are some differences
>>>> across products, though it shouldn't be), because we also use that for the task
>>>> and job status list (e.g. replication)
>>>
>>> I looked at them briefly and they have a few things in common, but imo
>>> it's not worth the hassle combining them. We wouldn't **need** this in
>>> pve/pmg but I could still submit a patch... let me know.
>>>
>>>>
>>
>> Depends on your definition of **need**. If we don't do this,
>> it would look differently e.g. for the task list in pve/pmg vs pbs
>> and we want to have the UI consistent across products
>> (except if there is a good reason to diverge) so for me it is
>> important to stay consistent here.
>>
>> @Thomas any opinion on this?
> 
> I find Gabriel has a point, it looks odd as is, and aligning it right
> makes it better fit in with other states like "OK" or "Error: ..".)

fine with me then

> 
> Having something like "Running" as text here would be nice, but  we
> cannot just use a `.x-grid-row-loading:after { content: 'Running' }`
> as that misses translations.
> 
> One option would be to avoid ExtJS CSS here and use font-awesome like
> "fa fa-fw fa-circle-o-notch fa-spin", the only disadvantage from that
> is that we lose the site-wide synced animation that the currently used
> GIF provides, can be worked around but IIRC there's nothing native
> available for doing so.

we could also create our own class that uses the gif as a background image, or
simply embed the gif directly? then we'd keep the image and could still
add a translated text

> 
> For now the adapted CSS would be an OK stop-gap, IIRC we only use such
> grid loading classes for such scheduled-job grids. Adding it to PVE
> (isn't there even anything besides replication using that?) would be
> good too though. IMO no need for widget toolkit, I'd keep this contained
> for now.

we use that for running tasks and replication, and the class
'x-grid-row-loading' is the same for pve/pmg/pbs (so i thought it might
warrant to have it in widget toolkit, so that we can remove it from the
individual projects' css files)

Re: [pbs-devel] [PATCH proxmox-backup] fix: ui: spinner position

[Gabriel Goller]

On 10/13/23 15:04, Dominik Csapak wrote:
> [..]
>
>
>> Having something like "Running" as text here would be nice, but  we
>> cannot just use a `.x-grid-row-loading:after { content: 'Running' }`
>> as that misses translations.
>>
>> One option would be to avoid ExtJS CSS here and use font-awesome like
>> "fa fa-fw fa-circle-o-notch fa-spin", the only disadvantage from that
>> is that we lose the site-wide synced animation that the currently used
>> GIF provides, can be worked around but IIRC there's nothing native
>> available for doing so.
>
> we could also create our own class that uses the gif as a background 
> image, or
> simply embed the gif directly? then we'd keep the image and could still
> add a translated text
To be honest, I wouldn't add a text. We would have to separate between 
'Running'
and 'Loading', because that's not the same thing (e.g., in pbs we use 
the spinner
on the snapshot size—where it would be 'Loading'—and on the task 
log—where it would
be 'Running'). Then we would also have to adjust some tables, because 
the 'Running'
or 'Loading' text won't fit (As far as I know, they use a fixed width).
>> For now the adapted CSS would be an OK stop-gap, IIRC we only use such
>> grid loading classes for such scheduled-job grids. Adding it to PVE
>> (isn't there even anything besides replication using that?) would be
>> good too though. IMO no need for widget toolkit, I'd keep this contained
>> for now.
>
> we use that for running tasks and replication, and the class
> 'x-grid-row-loading' is the same for pve/pmg/pbs (so i thought it might
> warrant to have it in widget toolkit, so that we can remove it from the
> individual projects' css files)
>
Right, we can do that, should be a fairly simple change as well.

Re: [pbs-devel] [PATCH proxmox-backup] fix: ui: spinner position

[Thomas Lamprecht]

Am 16/10/2023 um 11:51 schrieb Gabriel Goller:
> On 10/13/23 15:04, Dominik Csapak wrote:
>> we could also create our own class that uses the gif as a background image, or
>> simply embed the gif directly? then we'd keep the image and could still
>> add a translated text
> To be honest, I wouldn't add a text. We would have to separate between 'Running'
> and 'Loading', because that's not the same thing (e.g., in pbs we use the spinner
> on the snapshot size—where it would be 'Loading'

No, there it would be running to, as that indicates that there's no
size available due to a currently running job; or do you mean
something else?

> —and on the task log—where it would
> be 'Running'). Then we would also have to adjust some tables, because the 'Running'
> or 'Loading' text won't fit (As far as I know, they use a fixed width).

To clarify, I'd explicitly only left-align those spinners for when
used as job running spinners, not for loading – and IIRC we do not
have any per-cell or per-row loading indicators anyway, as we always
load all data of a grid at once (or at least fake doing so), but never
row or even cell-wise.

So moving this new alignment behavior into its own class, that derives
from the ExtJS one, seems more reasonable to avoid odd rendering issues
for the cases where the left-alignment isn't desired.

If we then add a text to that is an independent decision, but it surely
wouldn't hurt accessibility.

Re: [pbs-devel] [PATCH proxmox-backup] fix: ui: spinner position

[Gabriel Goller]

[-- Attachment #1: Type: text/plain, Size: 1318 bytes --]

On 10/16/23 12:04, Thomas Lamprecht wrote:

> [..]
> To clarify, I'd explicitly only left-align those spinners for when
> used as job running spinners, not for loading – and IIRC we do not
> have any per-cell or per-row loading indicators anyway, as we always
> load all data of a grid at once (or at least fake doing so), but never
> row or even cell-wise.
Oh, ok, got it.
> So moving this new alignment behavior into its own class, that derives
> from the ExtJS one, seems more reasonable to avoid odd rendering issues
> for the cases where the left-alignment isn't desired.
>
> If we then add a text to that is an independent decision, but it surely
> wouldn't hurt accessibility.
Ok, so I think I'll move the current `x-grid-row-loading` class to the
`proxmox-widget-toolkit/src/css/ext-6-pmx.css` file and create a new
class `x-grid-row-loading-left` (with the 10px margin left). I'll also
have to edit the `proxmox-dark/*.scss` files so that the logo is shown
correctly in the darkmode. To add the text I would simply return a `<span>`
tag with a margin left of 20px (more or less, to make place for the icon)
and have the translated text as a content.

Returning the icon directly as a `<img>` tag isn't that simple, I'd had to
attach the whole styling + the conditional dark mode stuff.

LMK what you think!

[-- Attachment #2: Type: text/html, Size: 2051 bytes --]

Re: [pbs-devel] [PATCH proxmox-backup] fix: ui: spinner position

[Gabriel Goller]

[-- Attachment #1: Type: text/plain, Size: 1673 bytes --]

On 10/16/23 13:57, Gabriel Goller wrote:

> [..]
> Ok, so I think I'll move the current `x-grid-row-loading` class to the
> `proxmox-widget-toolkit/src/css/ext-6-pmx.css` file and create a new
> class `x-grid-row-loading-left` (with the 10px margin left). I'll also
> have to edit the `proxmox-dark/*.scss` files so that the logo is shown
> correctly in the darkmode. To add the text I would simply return a 
> `<span>`
> tag with a margin left of 20px (more or less, to make place for the icon)
> and have the translated text as a content.
>
> Returning the icon directly as a `<img>` tag isn't that simple, I'd had to
> attach the whole styling + the conditional dark mode stuff.
>
> LMK what you think!
>
Nevermind, turns out the `background-color: invert(...)` stuff also 
inverts the
gif in the `<img>` tag!
So I could do something like this (e.g., for the rendering of the job 
status, but it
works the same way everywhere we use the loading-spinner):

```
if (!record.data['last-run-endtime']) {
     metadata.tdCls = 'x-grid-row-loading-left';
     return `<img 
src="../extjs/theme-crisp/resources/images/loadmask/loading.gif"/>${gettext('Running')}`;
}
```
Then in the `proxmox-widget-toolkit/src/css/ext-6-pmx.css` we add the 
`x-grid-row-loading-left`
class:
```
.x-grid-row-loading-left {
     text-align: left;
     display: flex;
     gap: 10px;
}
```
Then obviously add the `x-grid-row-loading-left` class to the 
`proxmox-dark/*.scss`
overrides for the dark mode.

Like this we only have to change the job-running spinners to return the 
`<img>` and
`<span>` elements (with the translation), and the other spinners don't 
have to be changed!

[-- Attachment #2: Type: text/html, Size: 2464 bytes --]

Re: [pbs-devel] [PATCH proxmox-backup] fix: ui: spinner position

[Gabriel Goller]

[..]
Submitted a new series with the proposed change.