[pbs-devel] [PATCH proxmox-backup v2 2/5] fix #3067: pbs ui: add support for a notes field in the dashboard
Dominik Csapak
d.csapak at proxmox.com
Tue Mar 1 11:41:27 CET 2022
high level comment:
i get what you tried to achieve here with the
container for info and notes, but i'd probably
simply add it as a regular panel.
that way, the user is not confused when the column selection
does not work the way he expects (e.g. setting it to 3 columns
still always has only the info/note as the first line)
so either a regular panel (without the collapsing feature, maybe
a browser storage option to hide it? like the days) or let
it behave like on the datastore summary (info+notes = 1 panel)
some comments inline
On 2/24/22 15:18, Stefan Sterz wrote:
> adds a panel to the dashboard displaying a comment similar to the
> comments panel in a datastore summary
>
> Signed-off-by: Stefan Sterz <s.sterz at proxmox.com>
> ---
> www/Dashboard.js | 110 ++++++++++++++++++------------
> www/Makefile | 2 +-
> www/datastore/Summary.js | 6 +-
> www/{datastore => panel}/Notes.js | 19 +++++-
> 4 files changed, 85 insertions(+), 52 deletions(-)
> rename www/{datastore => panel}/Notes.js (81%)
>
> diff --git a/www/Dashboard.js b/www/Dashboard.js
> index 70c2305b..a78ad375 100644
> --- a/www/Dashboard.js
> +++ b/www/Dashboard.js
> @@ -122,7 +122,7 @@ Ext.define('PBS.Dashboard', {
> if (key !== 'summarycolumns') {
> return;
> }
> - Proxmox.Utils.updateColumns(view);
> + Proxmox.Utils.updateColumnWidth(view);
this does not work like that? you'd have to
use the 'view.query...' expression like below else the
child panels do not get updated
> });
> },
> },
> @@ -192,26 +192,14 @@ Ext.define('PBS.Dashboard', {
>
> listeners: {
> resize: function(panel) {
> - Proxmox.Utils.updateColumns(panel);
> + panel.query('>').forEach(c => Proxmox.Utils.updateColumnWidth(c));
> },
> },
>
> title: gettext('Dashboard'),
>
> - layout: {
> - type: 'column',
> - },
> -
> bodyPadding: '20 0 0 20',
>
> - minWidth: 700,
> -
> - defaults: {
> - columnWidth: 0.49,
> - xtype: 'panel',
> - margin: '0 20 20 0',
> - },
> -
> tools: [
> {
> type: 'gear',
> @@ -224,42 +212,74 @@ Ext.define('PBS.Dashboard', {
>
> items: [
> {
> - xtype: 'pbsNodeInfoPanel',
> - reference: 'nodeInfo',
> - height: 280,
> - },
> - {
> - xtype: 'pbsDatastoresStatistics',
> + xtype: 'container',
> + layout: {
> + type: 'hbox',
> + align: 'stretch',
> + },
> + margin: '0 0 20 0',
> + padding: '0 20 0 0',
> height: 280,
> - },
> - {
> - xtype: 'pbsLongestTasks',
> - bind: {
> - title: gettext('Longest Tasks') + ' (' +
> - Ext.String.format(gettext('{0} days'), '{days}') + ')',
> + defaults: {
> + flex: 1,
> },
> - reference: 'longesttasks',
> - height: 250,
> - },
> - {
> - xtype: 'pbsRunningTasks',
> - height: 250,
> - },
> - {
> - bind: {
> - title: gettext('Task Summary') + ' (' +
> - Ext.String.format(gettext('{0} days'), '{days}') + ')',
> + minWidth: 700,
> + items: [{
> + xtype: 'pbsNodeInfoPanel',
> + reference: 'nodeInfo',
> + margin: '0 20 0 0',
> },
> - xtype: 'pbsTaskSummary',
> - height: 200,
> - reference: 'tasksummary',
> + {
> + xtype: 'pbsNotes',
> + reference: 'nodeNotes',
> + node: 'localhost',
> + loadOnInit: true,
> + }],
> },
> {
> - iconCls: 'fa fa-ticket',
> - title: 'Subscription',
> - height: 200,
> - reference: 'subscription',
> - xtype: 'pbsSubscriptionInfo',
> + xtype: 'container',
> + layout: {
> + type: 'column',
> + },
> + minWidth: 700,
> + defaults: {
> + columnWidth: 0.5,
> + xtype: 'panel',
> + margin: '0 20 20 0',
> + },
> + items: [{
> + xtype: 'pbsDatastoresStatistics',
> + height: 250,
> + },
> + {
> + xtype: 'pbsLongestTasks',
> + bind: {
> + title: gettext('Longest Tasks') + ' (' +
> + Ext.String.format(gettext('{0} days'), '{days}') + ')',
> + },
> + reference: 'longesttasks',
> + height: 250,
> + },
> + {
> + xtype: 'pbsRunningTasks',
> + height: 250,
> + },
> + {
> + bind: {
> + title: gettext('Task Summary') + ' (' +
> + Ext.String.format(gettext('{0} days'), '{days}') + ')',
> + },
> + xtype: 'pbsTaskSummary',
> + height: 250,
> + reference: 'tasksummary',
> + },
> + {
> + iconCls: 'fa fa-ticket',
> + title: 'Subscription',
> + height: 200,
> + reference: 'subscription',
> + xtype: 'pbsSubscriptionInfo',
> + }],
> },
> ],
> });
> diff --git a/www/Makefile b/www/Makefile
> index 455fbeec..636d4a57 100644
> --- a/www/Makefile
> +++ b/www/Makefile
> @@ -81,6 +81,7 @@ JSSRC= \
> panel/StorageAndDisks.js \
> panel/UsageChart.js \
> panel/NodeInfo.js \
> + panel/Notes.js \
> ZFSList.js \
> DirectoryList.js \
> LoginView.js \
> @@ -88,7 +89,6 @@ JSSRC= \
> SystemConfiguration.js \
> Subscription.js \
> datastore/Summary.js \
> - datastore/Notes.js \
> datastore/PruneAndGC.js \
> datastore/Prune.js \
> datastore/Content.js \
> diff --git a/www/datastore/Summary.js b/www/datastore/Summary.js
> index c3769257..d11646f0 100644
> --- a/www/datastore/Summary.js
> +++ b/www/datastore/Summary.js
> @@ -206,7 +206,7 @@ Ext.define('PBS.DataStoreSummary', {
> },
> },
> {
> - xtype: 'pbsDataStoreNotes',
> + xtype: 'pbsNotes',
> flex: 1,
> cbind: {
> datastore: '{datastore}',
> @@ -278,14 +278,14 @@ Ext.define('PBS.DataStoreSummary', {
> success: function(response) {
> let path = Ext.htmlEncode(response.result.data.path);
> me.down('pbsDataStoreInfo').setTitle(`${me.datastore} (${path})`);
> - me.down('pbsDataStoreNotes').setNotes(response.result.data.comment);
> + me.down('pbsNotes').setNotes(response.result.data.comment);
> },
> failure: function(response) {
> // fallback if e.g. we have no permissions to the config
> let rec = Ext.getStore('pbs-datastore-list')
> .findRecord('store', me.datastore, 0, false, true, true);
> if (rec) {
> - me.down('pbsDataStoreNotes').setNotes(rec.data.comment || "");
> + me.down('pbsNotes').setNotes(rec.data.comment || "");
> }
> },
> });
> diff --git a/www/datastore/Notes.js b/www/panel/Notes.js
> similarity index 81%
> rename from www/datastore/Notes.js
> rename to www/panel/Notes.js
> index 2928b7ec..4128dd8b 100644
> --- a/www/datastore/Notes.js
> +++ b/www/panel/Notes.js
> @@ -1,6 +1,6 @@
> -Ext.define('PBS.DataStoreNotes', {
> +Ext.define('PBS.panel.Notes', {
imo, the changes below would warrant its own patch
(makes it easier to review)
> extend: 'Ext.panel.Panel',
> - xtype: 'pbsDataStoreNotes',
> + xtype: 'pbsNotes',
> mixins: ['Proxmox.Mixin.CBind'],
>
> title: gettext("Comment"),
> @@ -11,7 +11,16 @@ Ext.define('PBS.DataStoreNotes', {
>
> cbindData: function(initalConfig) {
> let me = this;
> - me.url = `/api2/extjs/config/datastore/${me.datastore}`;
> +
> + if (('node' in me && 'datastore' in me) ||
> + (!('node' in me) && !('datastore' in me))) {
> + throw 'either both a node and a datastore were given or neither. only provide one.';
i'd do that check differently for multiple reasons
"'foo' in bar" is not syntax we really use
having a single error for multiple error cases seems complicated
something like this is more readable:
---
if (!me.node && !me.datastore) {
throw "no ...";
}
if (me.node && me.datastore) {
throw "both ....";
}
--
alternatively, you could prefer the 'node' (or datastore, doesn't matter)
instead and only throw an error if none is given
> + } else if ('node' in me) {
> + me.url = `/api2/extjs/nodes/${me.node}/config`;
> + } else {
> + me.url = `/api2/extjs/config/datastore/${me.datastore}`;
> + }
> +
> return { };
> },
>
> @@ -97,6 +106,10 @@ Ext.define('PBS.DataStoreNotes', {
> let sp = Ext.state.Manager.getProvider();
> me.collapseMode = sp.get('notes-collapse', 'never');
>
> + if (me.loadOnInit === true) {
> + me.load();
> + }
> +
> if (me.collapseMode === 'auto') {
> me.setCollapsed(true);
> }
More information about the pbs-devel
mailing list