thanks for the review!
i agree with all of the points of the answers in your other mails
some comments here:
On 6/20/23 15:25, Fiona Ebner wrote:
Am 19.06.23 um 16:13 schrieb Dominik Csapak:
by removing the confusing buttons in the toolbar and adding them as
actions in an actioncolumn. There a only relevant actions are visible
and get a more expressive tooltip
I agree with Aaron that the actioncolumn is too far right at the moment.
yes makes sense to me too
with this, we now differentiate between 4 modes of the edit window:
* create a new mapping altogether
- shows all fields
* edit existing mapping on top level
- show only 'global' fields (comment+mdev), so no mappings
This one feels slightly surprising to me from a user perspective as I
can't edit the actual mapping here. But it is cleaner and I guess one
could argue in the opposite direction too.
the question would be "which mapping" if there is more than one,
so i left it out, this way the config of the overall entry
is a bit seperated from the mappings themselves
* add new host mapping
- shows nodeselector, mapping and mdev, but mdev is disabled
(informational only)
* edit existing host mapping
- show selected node (displayfield) mdev and mappings, but only
mappings are editable
we have to split the nodeselector into two fields, since the disabling
cbind does not pass through to the editconfig (and thus makes the form
invalid if we try that)
Signed-off-by: Dominik Csapak <d.csa...@proxmox.com>
---
this is not intended to be applied as is, rather i'd like some feedback
on the approach (@thomas, @aaron ?) so that if we want to do it this way
i can also do it for the usb mappings
the other approach mentioned off-list can still be done
(having a full grid with all mappings regardless of the node)
maybe only for usb devices (there it makes imho more sense) but then
we'd have two interfaces for the mappings instead of one
It does involve a bit of clicking when it's only possible to add one
node entry at a time, but I'm not generally opposed to the current RFC.
I can image the action column takes a bit of getting used to as a
Proxmox VE user, because we don't really have those there yet.
we have it in pbs and pmg, but yes it's a new pattern for pve
i however find it less confusing than the toolbar buttons
The full grid might become quite big/confusing and involve lots of
scrolling or how would the grouping by node be done?
grouping/treestyle in such grids is always a bit tricky
i am currently working on this approach for the usb mapping
(as a demo of the other solution) but am running into quite
a few weird extjs related thing regarding widget columns and field
event handlers :( i'll see if i can send it tomorrow before lunch
Maybe a third alternative would be to have a tab for each node and show
basic meta-info like how many devices are already selected on that node
and a warn/error indicator if that node is affected?
mhmm that could be done, it's also a pattern we don't currently have:
a dynamic amount of tabs for each node
this could also become complex/confusing in a cluster with many nodes,
especially the hint in the tab title would probably not be visible
for all if there are many
i'll think about it
Would the full grid and tabs approach even be feasible with many nodes
or require too many API calls?
depends how we implement it, in my naive grid solution from above, the
api calls would be only done when a user selects a node
i.e it would look like:
-------------------------------------------------------------
| node | device | port | |
-------------------------------------------------------------
| [nodeselector] | [usbselector] | [usbselector] | [X] |
| | | | |
| | | | |
-------------------------------------------------------------
[add]
for pci it would be probably only one drop down + 'all functions' checkbox
the dropdowns only make an api call for the listing when the
nodeselector in the line changes its value
if we make more than one tab, we could defer the api call when the
user changes to the tab
www/manager6/tree/ResourceMapTree.js | 166 ++++++++++++++++-----------
www/manager6/window/PCIMapEdit.js | 42 ++++---
2 files changed, 130 insertions(+), 78 deletions(-)
diff --git a/www/manager6/tree/ResourceMapTree.js
b/www/manager6/tree/ResourceMapTree.js
index 02717042..cd24923e 100644
--- a/www/manager6/tree/ResourceMapTree.js
+++ b/www/manager6/tree/ResourceMapTree.js
@@ -49,44 +49,89 @@ Ext.define('PVE.tree.ResourceMapTree', {
});
},
- addHost: function() {
+ add: function(_grid, _rI, _cI, _item, _e, rec) {
let me = this;
- me.edit(false);
+ if (!rec.data.type === 'entry') {
AFAICT, this always evaluates to false, because of the misplaced '!'.
oops ;)
+ return;
+ }
+
+ me.openMapEditWindow(rec.data.name);
},
(...)
@@ -254,63 +299,56 @@ Ext.define('PVE.tree.ResourceMapTree', {
tbar: [
{
- text: gettext('Add mapping'),
+ text: gettext('Add'),
IMHO, Add mapping was/is better
OK
handler: 'addMapping',
cbind: {
disabled: '{!canConfigure}',
},
},
(...)
diff --git a/www/manager6/window/PCIMapEdit.js
b/www/manager6/window/PCIMapEdit.js
index 0da2bae7..a0b42758 100644
--- a/www/manager6/window/PCIMapEdit.js
+++ b/www/manager6/window/PCIMapEdit.js
@@ -13,8 +13,12 @@ Ext.define('PVE.window.PCIMapEditWindow', {
cbindData: function(initialConfig) {
let me = this;
- me.isCreate = !me.name || !me.nodename;
+ me.isCreate = (!me.name || !me.nodename) && !me.entryOnly;
me.method = me.name ? 'PUT' : 'POST';
+ me.hideMapping = !!me.entryOnly;
+ me.hideComment = me.name && !me.entryOnly;
+ me.hideNodeSelector = me.nodename || me.entryOnly;
+ me.hideNode = !me.nodename || !me.hideNodeSelector;
return {
name: me.name,
nodename: me.nodename,
Nit: Is it even necessary to return these two as they are already
persistent properties?
probably not, i guess this was leftover from some previous version
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
