Summary: | Implement week picking to calendar picker | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Keishi Hattori <keishi> | ||||||||||
Component: | Forms | Assignee: | Keishi Hattori <keishi> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | dglazkov, tkent, webkit.review.bot | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Bug Depends on: | |||||||||||||
Bug Blocks: | 100938 | ||||||||||||
Attachments: |
|
Description
Keishi Hattori
2012-11-07 04:53:18 PST
Created attachment 172767 [details]
Patch
Comment on attachment 172767 [details] Patch Attachment 172767 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/14760349 New failing tests: platform/chromium/fast/forms/calendar-picker/calendar-picker-mouse-operations.html Comment on attachment 172767 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=172767&action=review > Source/WebCore/ChangeLog:45 > + (DaysTable.prototype._handleKey): Refactored to share the same structure as subclasses. > + (MonthPickerDaysTable.prototype._handleKey): Would you make another patch for the refactoring of existing _handleKey please? > Source/WebCore/Resources/pagepopups/calendarPicker.css:236 > +.week-mode .available.day-selected.monday { Inconsistent space before '.' > Source/WebCore/Resources/pagepopups/calendarPicker.css:262 > +.week-mode .week-column.unavailable.day-selected { two spaces before '.' > Source/WebCore/Resources/pagepopups/calendarPicker.js:200 > + var week = Week.parse(dateString); > + if (week) > + return week; Need to update the jsdoc annotation. > Source/WebCore/Resources/pagepopups/calendarPicker.js:333 > + var week = Week.createFromDate(new Date(arguments[0])); arguments[0] is inconsistent. it should be valueOrWeekOrYear. > Source/WebCore/Resources/pagepopups/calendarPicker.js:407 > + var MillisecondsPerWeek = 7 * 24 * 60 * 60 * 1000; > + return Math.floor((createUTCDate(year + 1, 0, 4).getTime() - Week.weekOneStartDateForYear(year)) / MillisecondsPerWeek); This calculation appears in Week.createFromDate too. Please consider introducing another function. > Source/WebCore/Resources/pagepopups/calendarPicker.js:1428 > + * @param {!boolean} keepSelectionPosition > * @return {!boolean} > */ > -DaysTable.prototype._maybeSetPreviousMonth = function() { > +DaysTable.prototype._maybeSetPreviousMonth = function(keepSelectionPosition) { Boolean argument is not good for readability. Please provide a wrapper function, or an enum. Also, I think keepSelectionPosition argument should be optional in order to avoid existing callsites. > Also, I think keepSelectionPosition argument should be optional in order to avoid existing callsites.
avoid updating existing callsites.
Created attachment 172924 [details]
Patch
Comment on attachment 172767 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=172767&action=review >> Source/WebCore/Resources/pagepopups/calendarPicker.css:236 >> +.week-mode .available.day-selected.monday { > > Inconsistent space before '.' I'm not sure what you mean. Comment on attachment 172767 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=172767&action=review >>> Source/WebCore/Resources/pagepopups/calendarPicker.css:236 >>> +.week-mode .available.day-selected.monday { >> >> Inconsistent space before '.' > > I'm not sure what you mean. Please ignore this comment. I though you meant .week-mode.available.day-selected.monday or .week-mode .available .day-selected .monday But your code is correct. (In reply to comment #3) > > Source/WebCore/ChangeLog:45 > > + (DaysTable.prototype._handleKey): Refactored to share the same structure as subclasses. > > + (MonthPickerDaysTable.prototype._handleKey): > > Would you make another patch for the refactoring of existing _handleKey please? Don't you want to separate the patch? Created attachment 172927 [details]
Patch
Created attachment 172929 [details]
Patch
(In reply to comment #8) > (In reply to comment #3) > > > Source/WebCore/ChangeLog:45 > > > + (DaysTable.prototype._handleKey): Refactored to share the same structure as subclasses. > > > + (MonthPickerDaysTable.prototype._handleKey): > > > > Would you make another patch for the refactoring of existing _handleKey please? > > Don't you want to separate the patch? OK. I removed all changes to _handleKey. Comment on attachment 172929 [details]
Patch
ok.
Please don't forget smaller patches are better. Time to review a patch is superlinear. In this case, you can split the DaysTable.prototype.navigateToMonth argument change to another patch, and it would make this focus on the week support.
Comment on attachment 172929 [details] Patch Clearing flags on attachment: 172929 Committed r133850: <http://trac.webkit.org/changeset/133850> All reviewed patches have been landed. Closing bug. |