Closed Bug 345998 Opened 14 years ago Closed 14 years ago

Provide shared 'Jump to date' functionality

Categories

(Calendar :: Calendar Views, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: jminta, Assigned: jminta)

Details

(Whiteboard: ui-review+)

Attachments

(2 files, 2 obsolete files)

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...
Flags: blocking0.3+
Whiteboard: [cal-ui-review needed]
Whiteboard: [cal-ui-review needed] → [cal-ui-review needed][swag:1d]
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?
(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.
ui-review+ for lightning
Whiteboard: [cal-ui-review needed][swag:1d] → [swag:1d] ui-review+
Attached patch date-textbox (obsolete) — Splinter Review
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 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.
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)
Whiteboard: [swag:1d] ui-review+ → [swag:1d] ui-review+[patch in hand]
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+
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...
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-
Attached patch patch v2 (obsolete) — Splinter Review
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 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-
Attached patch loquaciousSplinter Review
Includes the additional comments requested.
Attachment #234874 - Attachment is obsolete: true
Attachment #235338 - Flags: second-review?(dmose)
Attachment #235338 - Flags: first-review+
Comment on attachment 235338 [details] [diff] [review]
loquacious

r=dmose
Attachment #235338 - Flags: second-review?(dmose) → second-review+
Patch checked in on MOZILLA_1_8_BRANCH and trunk.

-> FIXED
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Whiteboard: [swag:1d] ui-review+[patch in hand] → ui-review+
Whiteboard: ui-review+ → ui-review+ [litmus testcase wanted]
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.