From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate001.proxmox.com (gate001.proxmox.com [IPv6:2a0f:8001:1:32::40]) by lore.proxmox.com (Postfix) with ESMTPS id 546AE1FF0A7 for ; Wed, 02 Sep 2026 10:26:40 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id C640121268; Wed, 02 Sep 2026 10:26:39 +0200 (CEST) Content-Type: text/plain; charset=UTF-8 Date: Wed, 02 Sep 2026 10:26:35 +0200 Message-Id: Subject: Re: [PATCH datacenter-manager 3/3] ui: make the default dashboard page configurable From: "Lukas Wagner" To: "Dominik Csapak" , Mime-Version: 1.0 Content-Transfer-Encoding: quoted-printable X-Mailer: aerc 0.21.0-0-g5549850facc2-dirty References: <20260820081746.989972-1-d.csapak@proxmox.com> <20260820081746.989972-4-d.csapak@proxmox.com> In-Reply-To: <20260820081746.989972-4-d.csapak@proxmox.com> X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1788337592217 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.549 Adjusted score from AWL reputation of From: address DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment (newer systems) RCVD_IN_DNSWL_MED -2.3 Sender listed at https://www.dnswl.org/, medium trust SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Message-ID-Hash: CHCIDOCOZWWNSSLC4V4NI5YLM3OK4OG3 X-Message-ID-Hash: CHCIDOCOZWWNSSLC4V4NI5YLM3OK4OG3 X-MailFrom: l.wagner@proxmox.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox Datacenter Manager development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: Thanks for these patches! On Thu Aug 20, 2026 at 10:17 AM CEST, Dominik Csapak wrote: > by allowing to set the user any view as their dashboard, saved in their > local browser storage. > > It adds the selection combobox on the 'views' configuration page in the > toolbar. > >>From a user's perspective, I think having such a setting presented this way is a bit confusing, since there is no hint about this being a browser-local setting. If I didn't read the patch notes, my instinct while exploring the UI on my own would be that this is a global setting that affects all sessions and all users. Adding a hint for this in the toolbar, either as a label or tooltip seems a bit inpractical to me, mostly due to space constraints for a label or discoverability for a tooltip. Some alternatives could be: - Replace the combobox in the toolbar with a button, e.g. "Configure Dashboard", pressing that button shows a dialog that allows selecting a view, while also explaining that this is a local setting. I would position this button on the right-hand side, to differentiate it from the 'entity-management' buttons add/modify/delete. - The dashboard already has a couple local settings, such as the refresh interval, etc., these are in the 'gear' menu in the toolbar. Maybe on could select a view in there? Would in general also not hurt to add a note there that all of these settings are local - Have a 'My Settings' dialog in the user menu similar to PVE - Abandon the local setting altogether and make it a per-user setting that is stored in the backend. Might require bigger changes, since I don't think we have any good way to store per-user properties right now. Users that routinely use multiple browsers and/or devices might prefer this over a local setting. What do you think? One further thing that I have noticed: If a view that is used as a dashboard is deleted, we should probably fall back to the default dashboard again. Otherwise user might complain about 'broken' dashboards without a clear indication about what is wrong. > Most of the code is just extending the ViewSelector so it can work in > other situations than the 'add view' dialog: > * add on_change property > * add dashboard_name property to show different text in different > contexts > * passing through the 'default' property from the combobos > > It also refactors the '__dashboard__' string this does not have to be > hardcoded in different places. > [snip] > diff --git a/ui/src/widget/mod.rs b/ui/src/widget/mod.rs > index 07c47e36..1eaf0ffd 100644 > --- a/ui/src/widget/mod.rs > +++ b/ui/src/widget/mod.rs > @@ -31,7 +31,7 @@ pub use remote_selector::RemoteSelector; > mod remote_endpoint_selector; > =20 > mod view_selector; > -pub use view_selector::ViewSelector; > +pub use view_selector::{DASHBOARD_VALUE, ViewSelector}; > =20 > mod view_filter_selector; > pub use view_filter_selector::ViewFilterSelector; > diff --git a/ui/src/widget/view_selector.rs b/ui/src/widget/view_selector= .rs > index b48ef4f7..8bc15b0a 100644 > --- a/ui/src/widget/view_selector.rs > +++ b/ui/src/widget/view_selector.rs > @@ -6,12 +6,29 @@ use pwt::widget::form::Combobox; > use pwt_macros::{builder, widget}; > =20 > use pdm_api_types::views::ViewConfig; > +use yew::html::{IntoEventCallback, IntoPropValue}; > + > +pub const DASHBOARD_VALUE: &str =3D "__dashboard__"; Do you think it would be possible to get this working without such a magic value? So rather, in the code, instead of having a String for the view ID, have a Option where None is the default dashboard and Some(id) is a specific view? If that is impractical, we should probably disallow views with this specific name from being created in the backend. I managed to get some odd behavior after creating a view with this specific name. Probably a bit unlikely for a user to create a view with this specific name by accident, but if we can avoid having conflicts/issues altogether with some changes in the code, that would of course be better. > =20 > #[widget(comp=3DViewSelectorComp, @input)] > #[derive(Clone, Properties, PartialEq)] > #[builder] > pub struct ViewSelector { > store: Store, > + > + /// Change callback > + #[builder_cb(IntoEventCallback, into_event_callback, String)] > + #[prop_or_default] > + pub on_change: Option>, > + > + /// The default value > + #[builder(IntoPropValue, into_prop_value)] > + #[prop_or_default] > + pub default: Option, > + > + #[builder] > + #[prop_or(tr!("Dashboard"))] > + pub dashboard_name: String, > } > =20 > impl ViewSelector { > @@ -32,19 +49,23 @@ impl Component for ViewSelectorComp { > } > =20 > fn view(&self, ctx: &Context) -> Html { > - let mut list =3D vec!["__dashboard__".into()]; > + let props =3D ctx.props(); > + let mut list =3D vec![DASHBOARD_VALUE.into()]; > let store =3D &ctx.props().store; > for item in store.read().data().iter() { > list.push(item.id.clone().into()); > } > Combobox::new() > - .items(Rc::new(list)) > + .with_std_props(&ctx.props().std_props) > .with_input_props(&ctx.props().input_props) > - .on_change(|_| {}) > + .items(Rc::new(list)) > + .on_change(props.on_change.clone()) > + .default(props.default.clone()) > .render_value({ > + let name =3D props.dashboard_name.clone(); > move |value: &AttrValue| { > - if value =3D=3D "__dashboard__" { > - html! {{tr!("Dashboard")}} > + if value =3D=3D DASHBOARD_VALUE { > + html! {{&name}} > } else { > html! {{value}} > }