276799 - Move inline styles from calPrintEngine.js to a css file

Status: VERIFIED FIXED

(Calendar :: General, defect)

Description

[Simon Paquet [:sipaq]]

Currently calPrintEngine.js uses inline styles to style the print output. This
is bad. Using a separate calPrintEngine.css file would save us some footprint
and would also make it possible for users to modify the print output via
userchrome.css

Comment 1

[Simon Paquet [:sipaq]]

While I'm working on this bug, I have a few questions. Mostafah, maybe you can
help me here, since CVS blame shows that you've done most of the work on this file.

1. What does the commented out code at line 272 do? Can it be removed or should I 
   include it in the scope of this bug?
2. IMO the file should use a valid markup. As far as I can see, there is at least 
   one markup error. The <td>-tag opened in line 320 is never closed. IMO it 
   should be closed in line 359 after the </table>-tag.
3. Is there a reason why we don't use quotation marks throughout the HTML code?

I hope this makes some sense.

Comment 2

[Matthew (lilmatt) Willis]

(In reply to comment #1)
> 3. Is there a reason why we don't use quotation marks throughout the HTML code?

Since the gHtmlStrings are all enclosed in double quotes, double quotes in the
HTML would have to be escaped somehow, which is basically a big pain, so they
were omitted since the markup works without them. 

Comment 3

[Mostafa Hosseini]

(In reply to comment #1)
> 1. What does the commented out code at line 272 do? Can it be removed or should I 
>    include it in the scope of this bug?

Just remove it.

> 2. IMO the file should use a valid markup. As far as I can see, there is at least 
>    one markup error. The <td>-tag opened in line 320 is never closed. IMO it 
>    should be closed in line 359 after the </table>-tag.

Feel free to correct any markup errors.

Comment 4

[Simon Paquet [:sipaq]]

Attachment: New stylesheet

This is the new stylesheet for calPrintEngine.js

Comment 5

[Simon Paquet [:sipaq]]

Attachment: Patch v1

Comment 6

[Matthew (lilmatt) Willis]

(In reply to comment #4)
> Created an attachment (id=170648) [edit]
> New stylesheet
> 
> This is the new stylesheet for calPrintEngine.js

(In reply to comment #5)
> Created an attachment (id=170649) [edit]
> Patch v1
> 

Nice job sipaq!
Nominating for target milestone of SB 0.3

Comment 7

[Simon Paquet [:sipaq]]

Comment on attachment 170649 [details] [diff] [review]
Patch v1

>+    gHtmlString += "<table><tr><td valign=bottom align=center>";

Ugh, that should be

+    gHtmlString += "<table><tr><td class=bottomcenter>";

Mostafah, you want a new patch (incorporating that change) or can you work with
the current patch?

Comment 8

[Simon Paquet [:sipaq]]

Comment on attachment 170649 [details] [diff] [review]
Patch v1

I couldn't test the patch, because 1) my printer is broken and 2) printing is
broken, because the code is not hooked up to the new apis yet.

Because of that I would like to have two reviews on this patch. I'm asking mvl
to review this patch in addition to Mostafah, because he's worked quite a lot
on our printing code. Thanks in advance, guys!

Comment 9

[Mostafa Hosseini]

Comment on attachment 170649 [details] [diff] [review]
Patch v1

Patch works fine for me. I tested it on the 0.2 branch. The little change can
be made when the patch is checked in.

Comment 10

[Michiel van Leeuwen (email: mvl+moz@)]

Comment on attachment 170649 [details] [diff] [review]
Patch v1

Patch looks ok to me.

>+      gHtmlString += "<td class=bordertop width=14%>";
>+      gHtmlString += "<table height=100 width=100% class=noborder>";

I think those sizes be moved to css as well. And maybe the names of the classes
can be more descriptive. But feel free to do that in a follow-up bug if you
prefer.

You should add a license block to the css file.

Comment 11

[Mostafa Hosseini]

Patch and file checked in. Change from comment #7 applied.

Comment 12

[Simon Paquet [:sipaq]]

Attachment: Add license section to new css file (checked in)

This patch adds the usual license section to the new CSS file.
Mostafah, can you please check this in, when you find the time.

Comment 13

[Simon Paquet [:sipaq]]

(In reply to comment #10)

Hi mvl, thanks for the review.

> >+      gHtmlString += "<td class=bordertop width=14%>";
> >+      gHtmlString += "<table height=100 width=100% class=noborder>";
> 
> I think those sizes be moved to css as well.

Yes, you could move them to css. I left them in, because that would only result
in more code, instead of less code, which was the original intent behind this bug.

> And maybe the names of the classes can be more descriptive. But feel free to 
> do that in a follow-up bug if you prefer.

Sure. Do you have any particular classes that bug you the most or any
suggestions on what the names should look like?

> You should add a license block to the css file.

Already done. See the new patch in this bug.

Comment 14

[Michiel van Leeuwen (email: mvl+moz@)]

(In reply to comment #13)
> > >+      gHtmlString += "<td class=bordertop width=14%>";
> 
> Sure. Do you have any particular classes that bug you the most or any
> suggestions on what the names should look like?
> 
for example 'bordertop'. It describes the content of the style: a border on top.
I think it would be better if it describes the class: 'contentrow' (or something
like that. I don't know when that class is used).
or 'top'. That sounds to me like it is the top of the page, but it just means
the style says that is should be top-aligned.

Comment 15

[Simon Paquet [:sipaq]]

Comment on attachment 171377 [details] [diff] [review]
Add license section to new css file (checked in)

Mostafah, can you please check this in.

Comment 16

[Michiel van Leeuwen (email: mvl+moz@)]

Simon, what's left to be done here?

Comment 17

[Simon Paquet [:sipaq]]

(In reply to comment #16)
> Simon, what's left to be done here?

Mainly your comments in comment 14. But I think we can spin this off into a
followup bug.

Comment 18

[Reed Loden [:reed]]

The bugspam monkeys have been set free and are feeding on Calendar :: General. Be afraid for your sanity!