Closed Bug 332559 Opened 18 years ago Closed 18 years ago

calendar widget rewrite

Categories

(Core Graveyard :: XForms, defect)

Product:
Core Graveyard
Component:
XForms
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: surkov, Assigned: surkov)

References

Details

(Keywords: fixed1.8.0.4, fixed1.8.1)

Attachments

(1 file, 1 obsolete file)

patch2
23.32 KB, patch
allan
: review+
Details | Diff | Splinter Review 
Calendar should be changed in some way to achive more easy fix of bug 327584.
Status: NEW → ASSIGNED
Attached patch patch (obsolete) — — Splinter Review 
The main changes:
1) The current date is called as selected date, focused date is called as current. I changed the names because sometimes focused day can be not focused. It can confuse.
2) Current (previosuly named 'focused') date changing don't involve current day focusing.
3) When I click on back/forward button then focuse is saved on the button.
Attachment #217012 - Flags: review?(allan)
Attachment #217012 - Flags: review?(doronr)
Comment on attachment 217012 [details] [diff] [review]
patch

imho, it's better to ask for one review at a time. we might end up doing the same work twice otherwise.
Attachment #217012 - Flags: review?(allan)
Comment on attachment 217012 [details] [diff] [review]
patch

>diff -uNrap mozilla.orig/extensions/xforms/resources/content/widgets-xhtml.xml mozilla/extensions/xforms/resources/content/widgets-xhtml.xml
>--- mozilla.orig/extensions/xforms/resources/content/widgets-xhtml.xml	2006-03-20 19:00:10.000000000 +0800
>+++ mozilla/extensions/xforms/resources/content/widgets-xhtml.xml	2006-04-03 19:47:47.000000000 +0900
>@@ -48,7 +48,7 @@
>           xmlns:xbl="http://www.mozilla.org/xbl">
> 
> 
>- <!-- CALENDAR WIDGETS -->
>+<!-- CALENDAR WIDGETS -->
> 
> <!-- COMPACT CALENDAR -->
>   <binding id="calendar-compact"
>@@ -61,113 +61,125 @@
>     </content>
> 
>     <implementation>
>+    <!-- interface -->
>+      <method name="focus">
>+        <body>
>+          if (this.currentDayIndex != -1)
>+            this._dayElements[this.currentDayIndex].focus();
>+        </body>
>+      </method>

I think it is better to have the comment indented the same way as the method.  It makes it easier to read:

>+      <!-- interface -->
>+      <method name="focus">
>+        <body>
>+          if (this.currentDayIndex != -1)
>+            this._dayElements[this.currentDayIndex].focus();
>+        </body>
>+      </method>


> 
>     <handlers>
>-      <handler event="click">
>+      <handler event="keypress">
>+      <![CDATA[
>+        var target = event.originalTarget;
>+
>+        switch (event.keyCode) {
>+
>+        case event.DOM_VK_DOWN:
>+          if (target == this.backButton || target == this.fwdButton)
>+            this.focus();
>+          break;
>+
>+        case event.DOM_VK_UP:
>+          if (target.localName == "td" || target.namespaceURI == this.XHTML_NS) {
>+            if (this.currentDayIndex - 3 <= 0) {
>+              this.backButton.focus();
>+            } else if (this.currentDayIndex - 7 <= 0) {
>+              this.fwdButton.focus();
>+            }
>+          }
>+          break;

(target.localName == "td" || target.namespaceURI == this.XHTML_NS)

Why not just test for the namespaceURI - td will always be in the xhtml ns?
Attachment #217012 - Flags: review?(doronr) → review+
Attached patch patch2 — — Splinter Review 
(In reply to comment #3)
> I think it is better to have the comment indented the same way as the method. 
> It makes it easier to read:
>

I use the presented indentation for comment groups (f.x. to separate group of interface methods) and use the your asked for indentation for local comments (f.x. what do method do). F.x.

> >+    <!-- interface -->
> >+      <!-- Set focus on day of current date -->
> >+      <method name="focus">
> >+        <body>
> >+          if (this.currentDayIndex != -1)
> >+            this._dayElements[this.currentDayIndex].focus();
> >+        </body>
> >+      </method>

I added thease comments.

> >     <handlers>
> >-      <handler event="click">
> >+      <handler event="keypress">
> >+      <![CDATA[
> >+        var target = event.originalTarget;
> >+
> >+        switch (event.keyCode) {
> >+
> >+        case event.DOM_VK_DOWN:
> >+          if (target == this.backButton || target == this.fwdButton)
> >+            this.focus();
> >+          break;
> >+
> >+        case event.DOM_VK_UP:
> >+          if (target.localName == "td" || target.namespaceURI == this.XHTML_NS) {
> >+            if (this.currentDayIndex - 3 <= 0) {
> >+              this.backButton.focus();
> >+            } else if (this.currentDayIndex - 7 <= 0) {
> >+              this.fwdButton.focus();
> >+            }
> >+          }
> >+          break;
> 
> (target.localName == "td" || target.namespaceURI == this.XHTML_NS)
> 
> Why not just test for the namespaceURI - td will always be in the xhtml ns?
> 

calendar-compact supposes that it can be extended by successor binding, successor binding can replace content (like it do calendar-full) and therefore I prefer to be sure that 'td' has xhtml namespace.
Attachment #217012 - Attachment is obsolete: true
Attachment #217106 - Flags: review?(allan)
Comment on attachment 217106 [details] [diff] [review]
patch2

r=me
Attachment #217106 - Flags: review?(allan) → review+
Fixed on trunk
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Whiteboard: xf-to-branch
Blocks: 332853
Keywords: fixed1.8.0.3, fixed1.8.1
Whiteboard: xf-to-branch
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: