Closed
Bug 345998
Opened 18 years ago
Closed 18 years ago
Provide shared 'Jump to date' functionality
Categories
(Calendar :: Calendar Frontend, defect)
Calendar
Calendar Frontend
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: jminta, Assigned: jminta)
Details
(Whiteboard: ui-review+)
Attachments
(2 files, 2 obsolete files)
14.23 KB,
patch
|
mattwillis
:
first-review+
jminta
:
second-review-
|
Details | Diff | Splinter Review |
15.55 KB,
patch
|
jminta
:
first-review+
dmosedale
:
second-review+
|
Details | Diff | Splinter Review |
Sunbird currently has a 'Go To Date' dialog. Lightning currently has no option to quickly move far into the future. I'd like to get the functionality exposed in some way for 0.3, under the heading of Navigation. The basic idea is to allow a place for people to type in a date, rather than being forced to use navigation buttons. Option 1: Use Sunbird's dialog. -advantages: Easy to do. Code is well-tested. -disadvantages: dialogs are annoying not discoverable Option 2: Inline datepicker. (probably above/below the minimonth) -advantages: Streamlined/less UI. Highly visible Groups navigation functions together -disadvantages: More code. Very visible use of datepicker, which isn't the best UI. Generally for the inline picker, I'm looking at something like: |--------------- |<< May 2006 >> | S M T W H F S | 1 2 3 4 5 6 7 | 8 91011121314 |15161718192021 |22232425262728 |293031 1 2 3 4 |-------------- | [1/2/06 ] [Go] |-------------- |Tasks...
Assignee | ||
Updated•18 years ago
|
Flags: blocking0.3+
Whiteboard: [cal-ui-review needed]
Assignee | ||
Updated•18 years ago
|
Whiteboard: [cal-ui-review needed] → [cal-ui-review needed][swag:1d]
Comment 1•18 years ago
|
||
I can certainly try to hook up Sunbird's "Go to Date"-dialog in Lightning. But of course I would like to have a decision on whether this is "the right thing" before I prepare a patch. Joey?
Assignee | ||
Comment 2•18 years ago
|
||
(In reply to comment #1) > I can certainly try to hook up Sunbird's "Go to Date"-dialog in Lightning. But > of course I would like to have a decision on whether this is "the right thing" > before I prepare a patch. > > Joey? > That's UI-owners, not me.
Comment 3•18 years ago
|
||
ui-review+ for lightning
Whiteboard: [cal-ui-review needed][swag:1d] → [swag:1d] ui-review+
Assignee | ||
Comment 4•18 years ago
|
||
Patch implements a text-box for typing in dates. It's just Lightning for now, but implementing it in Sunbird is trivial.
Assignee: nobody → jminta
Status: NEW → ASSIGNED
Attachment #233330 -
Flags: second-review?(dmose)
Attachment #233330 -
Flags: first-review?(mattwillis)
Comment 5•18 years ago
|
||
Comment on attachment 233330 [details] [diff] [review] date-textbox >+Components.utils.reportError(day+','+offset); >+ if (day && !isNaN(offset)) { >+ day.setDate(day.getDate()+(7*offset)); >+ return day; >+ } >+ return null; >+ ]]></body> >+ </method> >+ </implementation> >+ </binding> Could you please fix the indentation here.
Assignee | ||
Comment 6•18 years ago
|
||
Same as before, but without the debug spew that Simon pointed out. Note that 2=space indenting is standard for this file.
Attachment #233330 -
Attachment is obsolete: true
Attachment #233370 -
Flags: second-review?(dmose)
Attachment #233370 -
Flags: first-review?(mattwillis)
Attachment #233330 -
Flags: second-review?(dmose)
Attachment #233330 -
Flags: first-review?(mattwillis)
Assignee | ||
Updated•18 years ago
|
Whiteboard: [swag:1d] ui-review+ → [swag:1d] ui-review+[patch in hand]
Comment 7•18 years ago
|
||
Comment on attachment 233370 [details] [diff] [review] without debug gunk Only nits... >Index: calendar/lightning/content/messenger-overlay-sidebar.js >=================================================================== >+function ltnGoToDate() { >+ var goToDate = document.getElementById("ltnDateTextPicker"); The other functions in this file put a \n before the {. >Index: calendar/resources/content/datetimepickers/datetimepickers.xml >=================================================================== Your indenting in this file contains some 4 space, some 3, and some 2. Since 2 appears to be the norm, fix the rest. r1=lilmatt with those fixed.
Attachment #233370 -
Flags: first-review?(mattwillis) → first-review+
Assignee | ||
Comment 8•18 years ago
|
||
Comment on attachment 233370 [details] [diff] [review] without debug gunk -Highlight all the text when the textbox is focused. + <xul:textbox anonid="date-textbox" flex="1" + onkeypress="if (event.keyCode == 13) fireGoEvent();"/> You can replace 13 with a descriptive constant. + <constructor><![CDATA[ + var goButton = document.getAnonymousElementByAttribute(this, "anonid", "date-go-button"); Declare the fields before the constructor. + <method name="parseLanguageDate"> + <parameter name="aValue"/> + <body><![CDATA[ Comment here explaining what this function does and what it returns. Also note that it doesn't parse things like 1/2/06 + } + ]]></body> + </method> + <method name="parseLanguageDate"> + <parameter name="aValue"/> + <body><![CDATA[ Whitspace in between methods/properties/etc + function getDateForDay(aWord) { Need more comments throughout this function + // Remove commas + aValue = aValue.replace(',',''); space between args here. more later...
Assignee | ||
Comment 9•18 years ago
|
||
Comment on attachment 233370 [details] [diff] [review] without debug gunk + aValue = aValue.toLowerCase(); don't assign to arguments. + if (aValue.indexOf(' ') == -1) { + // Just a single word, or a single date. + return getDateForDay(aValue); + } + + // Remove commas + aValue = aValue.replace(',',''); Parsing commas first makes us a bit more flexible here. + for (var i in this.mMonthShortNames) { + if (aValue.indexOf(this.mMonthShortNames[i]) != -1) { + var val = aValue.replace(this.mMonthShortNames[i], Number(i)+1); + val = val.replace(' ', '/'); + return this.parseDateTime(val); + } This can be a shared function for long/short, but that's not critical. + if (word == rel.word) { + offset = rel.offset; + continue; + } + } + for (var i in this.mDayNames) { + if (word == this.mDayNames[i]) { + day = getDateForDay(word); + continue; + } + } This should really be break, not continue. + if (day && !isNaN(offset)) { + day.setDate(day.getDate()+(7*offset)); + return day; + } Test against null, not just NaN here.
Attachment #233370 -
Flags: second-review?(dmose) → second-review-
Assignee | ||
Comment 10•18 years ago
|
||
Patch updated to reflect the previous review comments. Should have the indenting story figured out, and all of dmose's comments have been incorporated.
Attachment #234874 -
Flags: second-review?(dmose)
Attachment #234874 -
Flags: first-review+
Comment 11•18 years ago
|
||
Comment on attachment 234874 [details] [diff] [review] patch v2 getDateForDay still needs comments defining behavior and explaining code.
Attachment #234874 -
Flags: second-review?(dmose) → second-review-
Assignee | ||
Comment 12•18 years ago
|
||
Includes the additional comments requested.
Attachment #234874 -
Attachment is obsolete: true
Attachment #235338 -
Flags: second-review?(dmose)
Attachment #235338 -
Flags: first-review+
Comment 13•18 years ago
|
||
Comment on attachment 235338 [details] [diff] [review] loquacious r=dmose
Attachment #235338 -
Flags: second-review?(dmose) → second-review+
Comment 14•18 years ago
|
||
Patch checked in on MOZILLA_1_8_BRANCH and trunk. -> FIXED
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Whiteboard: [swag:1d] ui-review+[patch in hand] → ui-review+
Updated•18 years ago
|
Whiteboard: ui-review+ → ui-review+ [litmus testcase wanted]
Comment 15•18 years ago
|
||
Litmus testcase 2630 created
Whiteboard: ui-review+ [litmus testcase wanted] → ui-review+
You need to log in
before you can comment on or make changes to this bug.
Description
•