Re: [pmg-devel] [PATCH pmg-gui v2] StatTimeSelector: don't show invalid month/day combinations

[Thomas Lamprecht <t.lamprecht@proxmox.com>]

On 01/07/2022 10:03, Dominik Csapak wrote:
> by limiting the store of the day selector by the selected month
> 
> reported by a user in the forum:
> https://forum.proxmox.com/threads/wrong-calendar.111631/
> 

looks functional, some code nits inline that would make this quite a bit
shorter (and thus easier/quicker to read and grasp), ideally with those
addressed (but also otherwise if you really don't find them sensible):

Reviewed-by: Thomas Lamprecht <t.lamprecht@proxmox.com>

> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
> changes from v1:
> * use js 'Date' to correctly calculate the last day of the month
>   including leapyears, etc.
> 
>  js/StatTimeSelector.js | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/js/StatTimeSelector.js b/js/StatTimeSelector.js
> index f01b058..eae2d91 100644
> --- a/js/StatTimeSelector.js
> +++ b/js/StatTimeSelector.js
> @@ -73,7 +73,25 @@ Ext.define('PMG.StatTimeSelector', {
>  	    Ext.GlobalEvents.fireEvent('pmgStatTimeSelectorUpdate', data);
>  	},
>  
> +	updateDays: function() {

really not hard feelings for this one, but would be something like `updateMaxDays`
or `updateDayLimit` slightly more telling?

> +	    let yearsel = this.lookupReference('yearsel');
> +	    let monthsel = this.lookupReference('monthsel');

nit: useless intermediate variables

> +	    let daysel = this.lookupReference('daysel');
> +	    let year = yearsel.getValue();
> +	    let month = monthsel.getValue();

let year = this.lookupReference('yearsel').getValue();
let month = this.lookupReference('monthsel').getValue();

> +	    // create a date for the next month, but day 0 which wraps to
> +	    // the last day of the current month. Our month is already
> +	    // 1 greater than what Date expects, so we don't have to add 1

I'd use 100 cc for comments to reduce vertical space and reword as:

        // get last day of current month through wrapping back day 0 from next (zero indexed) month

a bit terse, but anybody that worked with JS's current date API just a bit would expect
anything sane here...

> +	    let maxDays = new Date(year, month, 0).getDate();
> +	    daysel.getStore().setFilters([{

In the spirit of above I'd drop the single use `daysel` too and instead do:

this.lookupReference('daysel').setFilters([{

> +		property: 'day',
> +		operator: '<=',
> +		value: maxDays,
> +	    }]);
> +	},
> +
>  	onSelect: function() {
> +	    this.updateDays();
>  	    this.updateVisibility();
>  	},
>