From: Dominik Csapak <d.csapak@proxmox.com>
To: Thomas Lamprecht <t.lamprecht@proxmox.com>,
Proxmox VE development discussion <pve-devel@lists.proxmox.com>
Subject: Re: [pve-devel] [PATCH manager 1/2] ui: BulkActions: rework filters and include tags
Date: Wed, 8 Nov 2023 13:14:59 +0100 [thread overview]
Message-ID: <07357be1-4b7e-4441-9323-9741b70cd483@proxmox.com> (raw)
In-Reply-To: <394a78a0-2ab3-4953-b4bd-0534504daffd@proxmox.com>
On 11/6/23 17:01, Thomas Lamprecht wrote:
> for the commit subject please: s/BulkActions/bulk actions/
>
> Am 30/10/2023 um 13:58 schrieb Dominik Csapak:
>> This moves the filters out of the grid header for the BulkActions and
>> puts them into their own fieldset above the grid. With that, we can
>> easily include a tags filter (one include and one exclude list).
>>
>> The filter fieldset is collapsible and shows the active filters in
>> parenthesis. aside from that the filter should be the same as before.
>>
>
> basic tests seem to work, and I did not really check the code closely,
> so only a higher level review, and some stuff is even pre-existing (but
> sticks a bit more out now):
>
> - the CT/VM ID filter is a bit odd, especially if tuned to match all,
> not only parts of the VMID (which would not be *that* much better
> either IMO), when I want to migrate/start/stop a single VM I can just
> do so, no need for opening the bulk actions.
counter argument:
if i want to migrate/start/stop a specific list of vmid it may be faster
if i go to the bulk window, search vmid -> select, search vmid -> select, etc.
than go to vm -> click stop -> go to vm -> click stop
but no hard feelings, if you want i'll remove it
>
> - The migration one should move target and jobs into two columns, as of
> now there's just to much vertical space used.
>
> - Maybe we can also merge the "allow local disk" check box and the
> warning for "ct will use restart mode" into columns (the former could
> loose the box label, that note is not really that useful anyway)
>
yeah i'll try those two
> - In the spirit of the last two points, the shutdown action might also
> benefit from having force-stop and timeout on the same row in two
> columns
>
make sense
> - We have a mix of title case and sentence case for the fields, they
> should all use title case (I find https://titlecaseconverter.com/
> nice). E.g., should be "HA Status" in the filters, and Parallel Jobs
> for migration, and so on.
sry for that, i'll fix it
>
> - tag space is very limited, maybe default to using circles or even
> dense (I'm still wishing a bit for seeing the tag value on hover, like
> a tool tip), or re-use the tree-style shape.
> One alternative might be to add vertical scrolling here, but that is
> probably rather odd (and not sure if that would even work easily
> here).
scrolling in two dimensions for different containers is always a bit weird
imho, but yeah making the tags 'dense' 'circle' or reusing the treestyle makes sense
i agree with the tooltip, but last time i looked at it that was not so easy
because we already have a tooltip in the tree (and we reuse the rendering
for that) but i could see if i could add a tooltip to the whole cell
here that prints all tags in a nice way, what do you think?
>
> - disallowing multi-select for Type (maybe better labeled "Guest Type"?)
> might improve UX, as if there are only two choices anyway a "All",
> "VM" "CT" selection might be simpler – but no hard feelings here.
ah yeah, i'm not really sure why i did it this way, dropdown
with 3 distinct values makes much more sense here...
next prev parent reply other threads:[~2023-11-08 12:15 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-10-30 12:58 Dominik Csapak
2023-10-30 12:58 ` [pve-devel] [PATCH manager 2/2] ui: BulkAction: add clear filters button Dominik Csapak
2023-11-06 16:01 ` [pve-devel] [PATCH manager 1/2] ui: BulkActions: rework filters and include tags Thomas Lamprecht
2023-11-08 12:14 ` Dominik Csapak [this message]
2023-11-08 12:38 ` Thomas Lamprecht
2023-11-08 13:26 ` Dominik Csapak
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=07357be1-4b7e-4441-9323-9741b70cd483@proxmox.com \
--to=d.csapak@proxmox.com \
--cc=pve-devel@lists.proxmox.com \
--cc=t.lamprecht@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