From: Thomas Lamprecht <t.lamprecht@proxmox.com>
To: Proxmox Backup Server development discussion
<pbs-devel@lists.proxmox.com>,
Philipp Hufnagl <p.hufnagl@proxmox.com>
Subject: Re: [pbs-devel] [PATCH proxmox-backup v6] ui: fix #4260: add dynamic notes in backup group comment
Date: Fri, 13 Oct 2023 13:54:04 +0200 [thread overview]
Message-ID: <9796d1c3-c50a-4fdd-bbba-09e7c828e580@proxmox.com> (raw)
In-Reply-To: <20231012135830.218326-1-p.hufnagl@proxmox.com>
Am 12/10/2023 um 15:58 schrieb Philipp Hufnagl:
> When there is no comment for a backup group, the comment of the last
> snapshot in this group will be shown slightly grayed out as long as
> the group is collapsed.
>
Looks OK, but as the commit message is wrong (see below), some style nits,
that I'd have just fixed up on applying otherwise, inline.
>
> Changes since v1:
> * code quality improvements
> * remove unused variable
As Dominik already told you once [0], please put the change log outside
of the commit message, after the "---" and before the diff-stat.
[0]: https://lists.proxmox.com/pipermail/pbs-devel/2023-October/006780.html
>
> Signed-off-by: Philipp Hufnagl <p.hufnagl@proxmox.com>
> ---
-> changelog goes here
> www/css/ext6-pbs.css | 4 ++++
> www/datastore/Content.js | 16 +++++++++++++---
> 2 files changed, 17 insertions(+), 3 deletions(-)
>
> @@ -903,13 +904,22 @@ Ext.define('PBS.DataStoreContent', {
> if (!data || data.leaf || data.root) {
> return '';
> }
> - if (v === undefined || v === null) {
> - v = '';
> +
> + let additionalClasses = "";
> + // when there is no group comment and the section is collapsed,
> + // display the most recent snapshot comment
If we really want to add a comment here, I'd keep it short – as when skimming
code and running into longer comments, I expect something odd happening that
needs explanation, not what I can read from the code anyway.
But if you really think it adds some value here, then fine by me, but maybe
go for something shorter like:
// fallback to the most recent snapshot comment for collapsed groups
(it's fine to go to the 100 character column width)
> + if (v === undefined || v === null|| v === '') {
can now simply be `if (!v) {`
> + if (data.expanded === false) {
Please no, an if expression is already a boolean one, no need for `xyz === false`
statements, just use a standard ! negation, like:
if (!data.expanded) {
...
or just swap the branches, avoiding the need for any negation, but having to
read things like `expr === false` or `expr === true` (or maybe `!expr !== true`)
just feels wrong.
> + v = data['last-comment'] ?? '';
> + additionalClasses = "pmx-faded";
> + } else {
> + v = '';
> + }
> }
prev parent reply other threads:[~2023-10-13 11:54 UTC|newest]
Thread overview: 2+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-10-12 13:58 Philipp Hufnagl
2023-10-13 11:54 ` Thomas Lamprecht [this message]
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=9796d1c3-c50a-4fdd-bbba-09e7c828e580@proxmox.com \
--to=t.lamprecht@proxmox.com \
--cc=p.hufnagl@proxmox.com \
--cc=pbs-devel@lists.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