consolidate decorated-month-view-binding and decorated-multiday-view-binding

VERIFIED FIXED in 1.0b1

Status

Calendar
Calendar Views
--
minor
VERIFIED FIXED
10 years ago
8 years ago

People

(Reporter: Berend Cornelius, Assigned: Berend Cornelius)

Tracking

unspecified
1.0b1

Details

Attachments

(2 attachments)

(Assignee)

Description

10 years ago
The two bindings "calendar-decorated-month-view" and "calendar-decorated-multiday-view" share a lot of code that can be consolidated.
(Assignee)

Comment 1

10 years ago
Created attachment 350444 [details] [diff] [review]
[checked in] patch v. #1

I added a parent binding calendar-decorated-month-view-parent that contains all common methods, handlers, properties and that both bindings extend to.
Attachment #350444 - Flags: review?(philipp)
Attachment #350444 - Flags: review?(philipp) → review+
Comment on attachment 350444 [details] [diff] [review]
[checked in] patch v. #1

>+            <!-- this property may be overridden by the
>+                descendent classes if neeeded  -->
>+            <property name="weeksInView">
>+                <getter><![CDATA[
>+                ]]></getter>
>+                <setter><![CDATA[
>+                ]]></setter>
>+            </property>
>+
I'd make this property do at least /something/. weeksInView could return 0 in the getter case and |val| in the setter case.


>+                        if (getPrefSafe(weekPrefix+prefNames[i])) {
spaces around operators, please check for other style errors while you are at it.

r=philipp
(Assignee)

Comment 3

10 years ago
Worked in the comments of philipp and pushed to comm-central:
http://hg.mozilla.org/comm-central/rev/fac88ec64ee0
->fixed
Status: NEW → ASSIGNED
(Assignee)

Updated

10 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.0

Comment 4

10 years ago
I checked this issue and found a minor bug:

STR:
====

- switch to multiweek or month view
- move mouse over a day with many events (scroll bar in day box should be visible)
- scroll the mouse wheel

Result:
=======

- the calendar view scrolls to the next week

Expected result:
================

- the scrolling should happens in the day box
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 5

10 years ago
Created attachment 351164 [details] [diff] [review]
[checked in] patch v. #2

This patch should fix it. As I investigated I could not see that my patch v. #1 was the culprit of this flaw. I did not take the time which was the actual reason for this but I find that the patch attached should be a clean solution for the problem.
(Assignee)

Updated

10 years ago
Keywords: qawanted
(Assignee)

Comment 6

10 years ago
Comment on attachment 351164 [details] [diff] [review]
[checked in] patch v. #2

Andreas tested this patch and found it was Ok.
Attachment #351164 - Flags: review?(philipp)
Attachment #350444 - Attachment description: patch v. #1 → [checked in] patch v. #1
Comment on attachment 351164 [details] [diff] [review]
[checked in] patch v. #2

Looks good, r=philipp
Attachment #351164 - Flags: review?(philipp) → review+
Please fix the missing space before the {
(Assignee)

Comment 9

10 years ago
pushed patch v. #2 to comm-central:

http://hg.mozilla.org/comm-central/rev/f1771043d32b

-> fixed
Status: REOPENED → RESOLVED
Last Resolved: 10 years ago10 years ago
Resolution: --- → FIXED
Keywords: qawanted
Attachment #351164 - Attachment description: patch v. #2 → [checked in] patch v. #2

Comment 10

10 years ago
Checked in lightning and sunbird build 20081207 -> VERIFIED.
Status: RESOLVED → VERIFIED
Target Milestone: 1.0 → 1.0b1
You need to log in before you can comment on or make changes to this bug.