142890 - make banner html a separate template

Status: RESOLVED FIXED

(Bugzilla :: User Interface, defect, P2)

Description

[Myk Melez [:myk] [@mykmelez]]

It's good to chop up templates into smaller bites so installations can customize
them with minimal forking and CVS conflict.  The template formerly known as the
"bannerhtml" parameter is now part of header.html.tmpl, even though it's a good
candidate for customization (installations that do nothing else are likely to
remove this explicit reference to mozilla.org).  That code should be broken out
into a separate template.

Comment 1

[Gervase Markham [:gerv]]

How about inverting the rationale?

That is to say, the current bannerhtml could be part of mozilla.org's custom
template.

Gerv

Comment 2

[Jouni Heikniemi]

That would IMO definitely be the right way to go, so b.m.o would be just once
customer among the others, and would create its own customizations when needed.
It's not a good thing for the default installation to contain an <img src>
referring to mozilla.org anyway.

Comment 3

[Andreas Franke (gone)]

But at least for me, it was helpful to have *some* banner by default, so that I
did not need to reinvent the <table> ...</table> around it etc.

Comment 4

[Matthew Tuck [:CodeMachine]]

I'd like to see this for 2.16, it's a regression after all.

As for having the default pointing to mozilla.org, I agree absolutely.  We can
easily replace it with something simple.  Getting something nicer was the point
of stalled bug #100095.

Comment 5

[Gervase Markham [:gerv]]

I think that most installations will customise a lot of header.html.tmpl and
footer.html.tmpl, and any hope of avoiding CVS conflicts is a pretty minimal
one. Splitting them up into tiny bits will just hurt performance and make the
header and footer as a whole more of a pain to work with. 

One solution would be to remove the mozilla.org-specific reference and replace
with a Bugzilla header. This will happen on the trunk, as the front page is
currently being redesigned, so I think that we should leave things as they are
there. So, the question then is: do we want to do anything on the branch? IMO,
for the reasons above, it's not worth it.

Gerv

Comment 6

[Matthew Tuck [:CodeMachine]]

Can you give some examples of customising the header and footer?  Maybe some
will, but I don't think most will.  mozilla.org won't AFAICS.  OTOH every
installation will customise their banner I imagine, at least until we get rid of
the mozilla.org banner.

Comment 7

[Andreas Franke (gone)]

Attachment: example: my cvs diff -w header.html.tmpl footer.html.tmpl

This is the current header/footer cvs diff from my installation.

Comment 8

[Matthew Tuck [:CodeMachine]]

Attachment: Proposed fix.

Comment 9

[Gervase Markham [:gerv]]

Comment on attachment 85210 [details] [diff] [review]
Proposed fix.

>+[%# Migration note: this corresponds to the old Param 'bannerhtml' %]

Nit: "this file".

>+    <table bgcolor="#000000" width="100%" border="0" cellpadding="0"
>+           cellspacing="0">
>+      <tr>
>+        <td>
>+          <font color="#FFFFFF" size="8"><center>
>+            This is a default Bugzilla installation!  Woohoo!

Please, no. This will get very boring very quickly.

>+          </center></font>
>+        </td>
>+      </tr>
>+    </table>

Why does this need a <table> rather than a <p>?

>+    <center>
>+      <small>Bugzilla version [% Param("version") %]</small>
>+    </center>

>-[%# Migration note: this section corresponds to the old Param 'bannerhtml' %]
>+[% INCLUDE global/banner.html.tmpl %]

You could leave this comment in, as:

>-[%# Migration note: banner.html.tmpl corresponds to the old Param 'bannerhtml' %]

It just makes things clearer when reading the header.

Gerv

Comment 10

[Matthew Tuck [:CodeMachine]]

> Why does this need a <table> rather than a <p>?

Andreas wanted it in comment #3.  Andreas, is there a substantial benefit here
for using a table, or are you too lazy to use custom. =)

Comment 11

[Matthew Tuck [:CodeMachine]]

Andreas?

Comment 12

[Matthew Tuck [:CodeMachine]]

Attachment: New fix.

This new patch should fix those issues.  I'm still using <table>, I can't be
bothered working out how to get the background colour effect otherwise.

I also have bumped the minor version number this time.

Comment 13

[Andreas Franke (gone)]

Matty, I have no idea whether there needs to be a table or not, and I don't care
whether the banner is in its own template or not (as long as there aren't any
performance hits due to the extra file, but they don't seem to be significant).
I just like to have *something* that can easily be customized (yes, I'm lazy),
e.g. by just changing the banner url.

Comment 14

[Jouni Heikniemi]

In order for there to be a banner url to change, there would have be a banner
gif in the distribution first. The current solution replaces the default heading
with just text, which is fine by me - but the bare text doesn't imo need a
table. The table is a necessary construction to create the
horizontally-100%-wide bar, but whether that is needed or not is a question of
layout - it goes well with the mozilla banner, but not necessarily with all the
other things. The <table> should really be replaced with a proper CSS2 layout
thingy, but that's not worth the effort at the moment.

I'd just replace the whole table with <h1>Bugzilla version [% Param("version")
%]</h1>. It isn't as easy customization as it could be, nor does it look very
cool... but since we lack a) a proper Bugzilla banner and b) time, I think it's
better to compromise here. We may have that bugzilla banner one day, and then
this can be made cool again. 

Comment 15

[Matthew Tuck [:CodeMachine]]

Attachment: Without the version number bump then.

Comment 16

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 86139 [details] [diff] [review]
Without the version number bump then.

r=myk

This is good enough; any additional tweaks are insufficiently important to hold
up the 2.16 release.

Comment 17

[Dave Miller [:justdave]]

Comment on attachment 86139 [details] [diff] [review]
Without the version number bump then.

r=justdave

IF you remove the comment in header.html.tmpl that "this file corresponds to
the old param" because it doesn't.  (or change the comment to note "the
following file" rather than "this file" so it's obvious it goes with the
following INCLUDE)

Comment 18

[Matthew Tuck [:CodeMachine]]

Checked in with comment addressed:

HEAD

Checking in template/en/default/global/banner.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/banner.html.tmpl,v 
<--  banner.html.tmpl
initial revision: 1.1
done
Checking in template/en/default/global/header.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/header.html.tmpl,v 
<--  header.html.tmpl
new revision: 1.8; previous revision: 1.7
done

2.16 BRANCH

Checking in template/en/default/global/banner.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/banner.html.tmpl,v 
<--  banner.html.tmpl
new revision: 1.1.2.1; previous revision: 1.1
done
Checking in template/en/default/global/header.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/header.html.tmpl,v 
<--  header.html.tmpl
new revision: 1.5.2.3; previous revision: 1.5.2.2
done