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

[Dominik Csapak <d.csapak@proxmox.com>]

On 7/1/22 11:32, Thomas Lamprecht wrote:
> 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();
>>   	},
>>   
> 

sure the changes make sense, sending as v3
although the comment does not fit in 100 columns here (takes up 103)
would that be still ok, or should i try to find an even shorter
sentence?

(also after looking at it again, i'd use 'lookup' instead of
'lookupReference' since that's only an alias and much shorter)