[webkit-reviews] review canceled: [Bug 83011] Add JavaScript and CSS code for the calendar picker implementation : [Attachment 135486] Patch 2

[bugzilla-daemon at webkit.org]

Kent Tamura <tkent at chromium.org> has canceled Kent Tamura
<tkent at chromium.org>'s request for review:
Bug 83011: Add JavaScript and CSS code for the calendar picker implementation
https://bugs.webkit.org/show_bug.cgi?id=83011

Attachment 135486: Patch 2
https://bugs.webkit.org/attachment.cgi?id=135486&action=review

------- Additional Comments from Kent Tamura <tkent at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=135486&action=review


Thank you for reviewing the long code.

>> Source/WebCore/Resources/calendarPicker.css:97
>> +}
> 
> Is this used?

Deleted.

>> Source/WebCore/Resources/calendarPicker.js:36
>> +var Class = {
> 
> Inspector code would use something plural like ClassNames.

Renamed to ClassNames.

>> Source/WebCore/Resources/calendarPicker.js:37
>> +	Available: 'available',
> 
> Inspector code uses double quotes for all strings.

Replaced ' with ".

>> Source/WebCore/Resources/calendarPicker.js:656
>> + * @param {boolean} right false:left, true:right
> 
> this param doesn't exist anymore.

Right.	Deleted.

>> Source/WebCore/Resources/calendarPicker.js:693
>> +	if (this._lastY > 0 && this._x > 0)
> 
> this._lastY should be this._y
> The next line too.

Fixed.

>> Source/WebCore/Resources/calendarPicker.js:775
>> +	node.classList.add(Class.Selected);
> 
> I found it strange that the selected date of the DaysTable doesn't match the
node with the day-selected class.
> I also think that it would be useful to show the current value inside the
calendar.
> 
> Maybe there should be a selected date that is the value
> and a separate highlighted date for mouse hover.

I don't like to change the UI behavior at this moment because Chrome UX/UI team
agreed about the current behavior.
Let's change the behavior if we have a lot of complain.

>> Source/WebCore/Resources/calendarPicker.js:863
>> +	    } else if (dayNode.classList.contains(Class.UnavailableUp)) {
> 
> Class.UnavailableUp and Class.UnavailableDown are never added to any node.

Removed related code.

>> Source/WebCore/Resources/calendarPicker.js:930
>> +	    global.yearMonthController.moveRelatively(event.shiftKey ?
YearMonthController.PREVIOUS_MONTH : YearMonthController.NEXT_MONTH);
> 
> Wrong names.
> _moveRelatively
> YearMonthController._PreviousMonth
> YearMonthController._NextMonth

Fixed. They should be public.

>> Source/WebCore/Resources/calendarPicker.js:932
>> +	    global.yearMonthController.moveRelatively(event.shiftKey ?
YearMonthController.PREVIOUS_YEAR : YearMonthController.NEXT_YEAR);
> 
> _moveRelatively
> YearMonthController._NextYear
> YearMonthController._PreviousYear

ditto.

>> Source/WebCore/Resources/calendarPicker.js:934
>> +	    global.yearMonthController.moveRelatively(event.shiftKey ?
YearMonthController.PREVIOUS_10YEARS : YearMonthController.NEXT_10YEARS);
> 
> _moveRelatively
> YearMonthController._PreviousTenYears
> YearMonthController._NextTenYears

ditto.

>> ManualTests/forms/calendar-picker.html:53
>> +var japaneseArguments = {
> 
> Is this used?

This is not used, but it's useful for l10n test. I'd like to keep this.