From: Dominik Csapak <d.csapak@proxmox.com>
To: Thomas Lamprecht <t.lamprecht@proxmox.com>, pmg-devel@lists.proxmox.com
Subject: Re: [pmg-devel] [PATCH pmg-gui v2] StatTimeSelector: don't show invalid month/day combinations
Date: Fri, 1 Jul 2022 11:57:45 +0200 [thread overview]
Message-ID: <3ac30ae3-8d45-3795-8afd-f24657f7ccd0@proxmox.com> (raw)
In-Reply-To: <7dbfea9d-6be7-ed4d-a505-6843c6f6f31e@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)
next prev parent reply other threads:[~2022-07-01 9:58 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-07-01 8:03 Dominik Csapak
2022-07-01 9:32 ` Thomas Lamprecht
2022-07-01 9:57 ` Dominik Csapak [this message]
2022-07-01 11:36 ` 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=3ac30ae3-8d45-3795-8afd-f24657f7ccd0@proxmox.com \
--to=d.csapak@proxmox.com \
--cc=pmg-devel@lists.proxmox.com \
--cc=t.lamprecht@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