Dialog specific files should be moved into /calendar/base/content/dialogs/

RESOLVED FIXED in 1.0b1

Status

Calendar
General
RESOLVED FIXED
10 years ago
6 years ago

People

(Reporter: Berend Cornelius, Assigned: Taraman)

Tracking

unspecified
1.0b1

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

10 years ago
As Philipp proposed in Bug 396819 comment #11 (Event Summary Dialog doesn't show important information to user) we should move all source code related to dialogs in a special folder like calendar/base/content/dialogs.
Summary: dialog specific source folder should be created → Dialog specific files should be moved into /calendar/base/content/dialogs/
(Assignee)

Updated

9 years ago
Assignee: nobody → Mozilla
(Assignee)

Comment 1

9 years ago
Some things I stumbled about:
The Publish Dialog is confusing me:
There is
calendar/base/content/calendar-publish-dialog.xul &.js

and
calendar/resources/content/publishDialog.xul & .js

The first one does not seem to be used anymore:
http://mxr.mozilla.org/comm-central/search?string=calendar-publish-dialog.xul

in calendar/resources/content/ there is also the PrintDialog.

Two questions arise:
1. What is the currently used PublishDialog and can the other files be deleted?
2. why is some code in the /resources/ directory rather than /base/ ?
(In reply to comment #1)
> 1. What is the currently used PublishDialog and can the other files be deleted?

The one in /resources/content! The other publish dialog can be removed imo. I work on an updated and cleaned up version of this dialog in bug 305526.


> 2. why is some code in the /resources/ directory rather than /base/ ?

Everything shared between Sunbird and Lightning should live in /base, other files should be moved into /lightning or /sunbird. The resources directory is still a relict from the times when there was the Mozilla Calendar extension.
(Assignee)

Comment 3

9 years ago
Created attachment 386906 [details] [diff] [review]
Patch V1.0

Moving all dialog-related files from calendar/base/content to calendar/base/content/dialogs.

The obsolete calendar-Publish-Dialog files are removed.

The Lightning folder does not have any dialogs.
The Sunbird folder has an "About"-Dialog, but there are so few files, that another subfolder won't bring a better overview.

>The resources directory is still a relict from the times when
>there was the Mozilla Calendar extension.
This could be straightened out in a seperate bug.
Attachment #386906 - Flags: review?(mschroeder)
Status: NEW → ASSIGNED
Comment on attachment 386906 [details] [diff] [review]
Patch V1.0

You should also move the css files which are specific to dialogs. The patch looks good so far. I'll just r- it to signal it is not complete, and wait for the next iteration with css files considered. :)
Attachment #386906 - Flags: review?(mschroeder) → review-
(Assignee)

Comment 5

9 years ago
Created attachment 390195 [details] [diff] [review]
Patch V2 including .css files

This is the patch including the .css-files in the Themes-directories.

calendar-event-dialog.css from the content directory is split up and incorporated into the calendar-event-dialog.css files in the themes dir.
The one in content/dialogs now only has -moz-binding entries.
Attachment #390195 - Flags: review?(mschroeder)
Comment on attachment 390195 [details] [diff] [review]
Patch V2 including .css files

r=mschroeder with following remarks:

> --- a/calendar/base/content/dialogs/calendar-event-dialog.css
> +++ b/calendar/base/content/dialogs/calendar-event-dialog.css
[...]
>  daypicker-weekday {
>      -moz-binding: url(chrome://calendar/content/calendar-daypicker.xml#daypicker-weekday);
>      -moz-user-focus: normal;
>      margin-top: 2px;
>  }
>  
>  daypicker-monthday {

Could you move the margin declaration for those to the theme?


The following problems with the CSS changes also apply to the winstripe version:

> --- a/calendar/base/themes/pinstripe/calendar-event-dialog.css
> +++ b/calendar/base/themes/pinstripe/dialogs/calendar-event-dialog.css
[...]
> -.status-icon {
> -  margin: 0 3px;
> -  display: none;
> -  list-style-image: url("chrome://calendar/skin/calendar-event-dialog-attendees.png");

You should leave the common parts of the rules for the icons in this general one. Also have a look at https://developer.mozilla.org/en/Writing_Efficient_CSS#Rely_on_inheritance! regarding optimizing CSS and inheritance. The display property shouldn't be needed because "inline" is the default value, and the ".status-icon" rule is always overridden.

[...]
> +#addressingWidget {
> +    -moz-user-focus: none;
> +}
> +
> +
> +#typecol-addressingWidget {
> +    min-width: 9em;
> +    border-right: 1px solid #CACAFF;
> +}

Please, remove the empty line.


I would recommend to introduce a general rule for class ".role-icon" with properties "margin" and "list-style-image". Also, there is no difference between normal and disabled ".role-icon". They use the same image, but code-wise there is a difference, so maybe we need another set of icons, or we can remove the code setting/removing the 'disabled' attribute? I don't know what's the right solution here.


> +.role-icon[role="CHAIR"] {
> +    margin: 0 3px;
> +    list-style-image: url(chrome://calendar/skin/calendar-event-dialog-attendees.png);
> +    -moz-image-region: rect(0px 201px 16px 180px);
> +}
> +.role-icon[role="CHAIR"][disabled="true"] {
> +    margin: 0 3px;
> +    list-style-image: url(chrome://calendar/skin/calendar-event-dialog-attendees.png);
> +    -moz-image-region: rect(0px 201px 16px 180px);
> +}
> +
> +.role-icon[role="CHAIR"] {
> +    margin: 0 3px;
> +    list-style-image: url(chrome://calendar/skin/calendar-event-dialog-attendees.png);
> +    -moz-image-region: rect(0px 201px 16px 180px);
> +}

Please, remove the duplicate rule for ".role-icon[role="CHAIR"]".


Some additional remarks:
We seem to have a -moz-binding in the themes for calendar-alarm-dialog.css which should be moved back to content. There are also some probably missed dialogs: calendar-alarm-snooze-popup.xul/js, calendar-subscription-list.xml, calendar-occurrence-prompt.xul, calErrorPrompt.xul, migration.xul/js and calendar-alarm-widget.xml. The patch is large enough, so this can go into a new bug & patch. :)
Attachment #390195 - Flags: review?(mschroeder) → review+
(Assignee)

Comment 7

8 years ago
Created attachment 395668 [details] [diff] [review]
Patch V2.1

>> --- a/calendar/base/themes/pinstripe/calendar-event-dialog.css
>> +++ b/calendar/base/themes/pinstripe/dialogs/calendar-event-dialog.css
[...]
>> -.status-icon {
>> -  margin: 0 3px;
>> -  display: none;
>> -  list-style-image: url("chrome://calendar/skin/calendar-event-dialog-attendees.png");

>You should leave the common parts of the rules for the icons in this general
>one. Also have a look at
These parts were duplicates in both .css files. combined and corrected them.

>I would recommend to introduce a general rule for class ".role-icon" with
>properties "margin" and "list-style-image".
Also fixed. 

Same applies to .left-icon & .right-icon.
I changed that too.

Since there have been quite some changes in the .css now, please have a look at it again.

For the additional comments I will file a spinoff-bug when this is checked in.
Attachment #386906 - Attachment is obsolete: true
Attachment #390195 - Attachment is obsolete: true
Attachment #395668 - Flags: review?(mschroeder)
Comment on attachment 395668 [details] [diff] [review]
Patch V2.1

r=mschroeder with a followup bug filed for the missing dialogs and following change reverted:

>--- a/calendar/base/themes/pinstripe/calendar-alarm-dialog.css
>+++ b/calendar/base/themes/pinstripe/dialogs/calendar-alarm-dialog.css
>@@ -34,7 +34,7 @@
>  *
>  * ***** END LICENSE BLOCK ***** */
> 
>-/* Bindings */
>+ /* Bindings */
> calendar-alarm-widget {
>     -moz-binding: url(chrome://calendar/content/calendar-alarm-widget.xml#calendar-alarm-widget);
> }
Attachment #395668 - Flags: review?(mschroeder) → review+
(Assignee)

Comment 9

8 years ago
Pushed to comm-central <http://hg.mozilla.org/comm-central/rev/e392c93ea182>

-> FIXED
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.0
(Assignee)

Comment 10

8 years ago
Followup bug is Bug 515337.
These bugs are likely targeted at Lightning 1.0b1, not Lightning 1.0. If this change was done in error, please adjust the target milestone to its correct value. To filter on this bugspam, you can use "lightning-10-target-move".
Target Milestone: 1.0 → 1.0b1
You need to log in before you can comment on or make changes to this bug.