public inbox for pve-devel@lists.proxmox.com
 help / color / mirror / Atom feed
From: Dominik Csapak <d.csapak@proxmox.com>
To: Proxmox VE development discussion <pve-devel@lists.proxmox.com>,
	Matthias Heiserer <m.heiserer@proxmox.com>
Subject: Re: [pve-devel] [PATCH v2 widget-toolkit 1/3] Buttons: add AltText
Date: Thu, 7 Apr 2022 12:46:25 +0200	[thread overview]
Message-ID: <7145eefb-5962-65f1-0e10-53da5d164528@proxmox.com> (raw)
In-Reply-To: <20220328130737.2961451-1-m.heiserer@proxmox.com>

patches LGTM, 2/3 could be squashed into 1/3 so that we have
the new code in the same commit as the Makefile change,
or what do you think @thomas? do you prefer those things
to be split into 2 patches?

on an additional note, we could even add more functionality to the
button, namely a simple 'setTextVariant' (or similar) so that the
caller can easily change between the normal and alt text.
Also a getter (like 'isAltText' or 'isNormalText') would be nice

for now the users of this button always access the 'altText' and 'text'
property directly which doesn't seem that nice if we already have
a special class for it

but this could be done later too, so no hard feelings

On 3/28/22 15:07, Matthias Heiserer wrote:
> The same code is used once in widget toolkit and twice in PVE already,
> so it makes sense to add it as a separate button.
> 
> Signed-off-by: Matthias Heiserer <m.heiserer@proxmox.com>
> ---
> changes from v1:
> move into separate class
> rename vars to something a little bit nicer
> move comment above function
> reorder some statements
> 
> Note: Because it's now guaranteed that the function is called at render time,
> I think we can leave out the extra logic to test which text is currently set.
> 
> 
>   src/button/AltText.js | 22 ++++++++++++++++++++++
>   1 file changed, 22 insertions(+)
>   create mode 100644 src/button/AltText.js
> 
> diff --git a/src/button/AltText.js b/src/button/AltText.js
> new file mode 100644
> index 0000000..e74d042
> --- /dev/null
> +++ b/src/button/AltText.js
> @@ -0,0 +1,22 @@
> +Ext.define('Proxmox.button.AltText', {
> +    extend: 'Proxmox.button.Button',
> +    xtype: 'proxmoxAltTextButton',
> +
> +    defaultText: "",
> +    altText: "",
> +
> +    listeners: {
> +	// HACK: calculate the max button width on first render to avoid toolbar glitches
> +	render: function(button) {
> +	    let me = this;
> +
> +	    button.setText(me.altText);
> +	    let altWidth = button.getSize().width;
> +
> +	    button.setText(me.defaultText);
> +	    let defaultWidth = button.getSize().width;
> +
> +	    button.setWidth(defaultWidth > altWidth ? defaultWidth : altWidth);
> +	},
> +    },
> +});





  parent reply	other threads:[~2022-04-07 10:46 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-03-28 13:07 Matthias Heiserer
2022-03-28 13:07 ` [pve-devel] [PATCH v2 widget-toolkit 2/3] Makefile: " Matthias Heiserer
2022-03-28 13:07 ` [pve-devel] [PATCH v2 widget-toolkit 3/3] apt repos: use new AltText button Matthias Heiserer
2022-03-28 13:07 ` [pve-devel] [PATCH v2 manager] ui: tree-wide: " Matthias Heiserer
2022-04-07 10:46 ` Dominik Csapak [this message]
2022-04-07 10:50 ` [pve-devel] [PATCH v2 widget-toolkit 1/3] Buttons: add AltText Dominik Csapak
2022-04-27 16:55 ` [pve-devel] applied: " Thomas Lamprecht

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=7145eefb-5962-65f1-0e10-53da5d164528@proxmox.com \
    --to=d.csapak@proxmox.com \
    --cc=m.heiserer@proxmox.com \
    --cc=pve-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
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal