Re: [pve-devel] [PATCH widget-toolkit v7 1/3] add tag related helpers

[Aaron Lauterer <a.lauterer@proxmox.com>]

Some small nits inline

On 6/21/22 11:19, Dominik Csapak wrote:
> helpers to
> * generate a color from a string consistently
> * generate a html tag for a tag
> * related css classes
> 
> contrast is calculated according to SAPC draft:
> https://github.com/Myndex/SAPC-APCA
> 
> which is likely to become a w3c guideline in the future and seems
> to be a better algorithm for this
> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
>   src/Utils.js         | 90 ++++++++++++++++++++++++++++++++++++++++++++
>   src/css/ext6-pmx.css | 45 ++++++++++++++++++++++
>   2 files changed, 135 insertions(+)
> 
> diff --git a/src/Utils.js b/src/Utils.js
> index 6a03057..eb13838 100644
> --- a/src/Utils.js
> +++ b/src/Utils.js
> @@ -1272,6 +1272,96 @@ utilities: {
>   	    .map(val => val.charCodeAt(0)),
>   	);
>       },
> +
> +    stringToRGB: function(string) {
> +	let hash = 0;
> +	if (!string) {
> +	    return hash;
> +	}
> +	string += 'prox'; // give short strings more variance
> +	for (let i = 0; i < string.length; i++) {
> +	    hash = string.charCodeAt(i) + ((hash << 5) - hash);
> +	    hash = hash & hash; // to int
> +	}
> +
> +	let alpha = 0.7; // make the color a bit brighter
> +	let bg = 255; // assume white background
> +
> +	return [
> +	    (hash & 255)*alpha + bg*(1-alpha),
> +	    ((hash >> 8) & 255)*alpha + bg*(1-alpha),
> +	    ((hash >> 16) & 255)*alpha + bg*(1-alpha),

I don't think our style guides specify this clearly, but I find the mix of 
spaces and no spaces around the operators inconsistent. There are a few more 
places in this patch where we do have that kind of inconsistency.

> +	];
> +    },
> +
> +    rgbToCss: function(rgb) {
> +	return `rgb(${rgb[0]}, ${rgb[1]}, ${rgb[2]})`;
> +    },
> +
> +    rgbToHex: function(rgb) {
> +	let r = Math.round(rgb[0]).toString(16);
> +	let g = Math.round(rgb[1]).toString(16);
> +	let b = Math.round(rgb[2]).toString(16);
> +	return `${r}${g}${b}`;
> +    },

[...]

> +
> +    getTagElement: function(string, color_overrides) {
> +	let rgb = color_overrides?.[string] || Proxmox.Utils.stringToRGB(string);
> +	let bgcolor = Proxmox.Utils.rgbToCss(rgb);
> +	let style = `background-color: ${bgcolor};`;

Couldn't we save a line by calling Proxmox.Utils.rgbToCss directly in the 
string? E.g.
let style = `background-color: ${Proxmox.Utils.rgbToCss(rgb)};`;

> +	let cls;
> +	if (rgb.length > 3) {
> +	    let fgcolor = Proxmox.Utils.rgbToCss([rgb[3], rgb[4], rgb[5]]);
> +	    style += `color: ${fgcolor}`;

Same as above here.

> +	    cls = "proxmox-tag-dark";
> +	} else {
> +	    let txtCls = Proxmox.Utils.getTextContrastClass(rgb);
> +	    cls = `proxmox-tag-${txtCls}`;
> +	}
> +	return `<span class="${cls}" style="${style}">${string}</span>`;
> +    },
>   },