Provide shared 'Jump to date' functionality

RESOLVED FIXED

Status

RESOLVED FIXED
12 years ago
12 years ago

People

(Reporter: jminta, Assigned: jminta)

Tracking

Details

(Whiteboard: ui-review+)

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

12 years ago
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

12 years ago
Flags: blocking0.3+
Whiteboard: [cal-ui-review needed]
(Assignee)

Updated

12 years ago
Whiteboard: [cal-ui-review needed] → [cal-ui-review needed][swag:1d]

Comment 1

12 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

12 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

12 years ago
ui-review+ for lightning
Whiteboard: [cal-ui-review needed][swag:1d] → [swag:1d] ui-review+
(Assignee)

Comment 4

12 years ago
Created attachment 233330 [details] [diff] [review]
date-textbox

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

12 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

12 years ago
Created attachment 233370 [details] [diff] [review]
without debug gunk

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

12 years ago
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+
(Assignee)

Comment 8

12 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

12 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

12 years ago
Created attachment 234874 [details] [diff] [review]
patch v2

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-
(Assignee)

Comment 12

12 years ago
Created attachment 235338 [details] [diff] [review]
loquacious

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
Last Resolved: 12 years ago
Resolution: --- → FIXED
Whiteboard: [swag:1d] ui-review+[patch in hand] → ui-review+

Updated

12 years ago
Whiteboard: ui-review+ → ui-review+ [litmus testcase wanted]

Comment 15

12 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.