Code review comment for lp:~julian-edwards/launchpad/mechanical-changes-6

> = Summary =
> Mechanical 3.0 changes for DistroArchSeriesBinaryPackage[Release] pages

These pages look *so* much better! It's great seeing LP transformed :)

>
> == Implementation details ==
> I slipped in a quick bug fix while I was at it:
> https://bugs.edge.launchpad.net/bugs/241341
> to fix a page heading. This was just a case of changing the title property on
> the context class.

Great! As mentioned in irc, you've added an extra 'for' into the title unintentionally, and it would be good to use smartquotes here.

>
> The change to the DistroArchSeriesBinaryPackage is trivial and can be seen
> here:
> https://launchpad.dev/ubuntu/warty/i386/mozilla-firefox
>
> The change to the DistroArchSeriesBinaryPackageRelease page was more involved.
> It uses several page fragments in the package relationships section that are
> in a different context/view. They are not used anywhere else so I added some
> conditional "yui-u" div classes to those so that they don't appear when not
> necessary.
>
> The breadcrumbs on both pages are not working properly, I will fix those in a
> separate branch.
>
> == Tests ==
> bin/test -vvt stories.soyuz -t distroarchseriesbinarypackage

Looks like the tests for the title need updating (or you may have done it since I merged).

The only other thing is that we need to ensure we markup the package relationships correctly... currently there are dt's that are not enclosed by dl's (like having an <li> without it being inside a <ul> or an <ol>. That in itself is as easy as doing the following for each relationship portlet:

http://pastebin.ubuntu.com/264324/

It does leave one remaining problem - the <dt> is then followed by a <ul> rather than a bunch of <dd>'s, but we can't fix that yet as another template (sourcepackage-index.pt) uses the same +render-list traversal (which in-turn uses the packagerelationship-list.pt). So I think we should wait until sourcepackage-index.pt is updated to 3-0 before updating packagerelationship-list.pt to output dd's). I'll leave it up to you whether it's worth an XXX.

Looking forward to seeing it land!

IRC log:
<bigjools> great
<noodles775> bigjools: you mention in your MP that the breadcrumbs aren't working properly - in what way?
<bigjools> noodles775: they are now, I landed a change last night :)
<noodles775> bigjools: heh, I couldn't find a problem :)
<bigjools> they stopped at 9.04
<noodles775> bigjools: any reason you didn't use smartquote in the title?
<bigjools> or whatever version
<bigjools> meh, details :)
<noodles775> (like in model/distributionsourcepackage's title?)
<noodles775> bigjools: also, see https://launchpad.dev/ubuntu/warty/i386/mozilla-firefox
<noodles775> What do you think we should do in that case? (The empty Package relationships section)
<bigjools> noodles775: it will never be empty in real life
<noodles775> Great.
<bigjools> our sample data is bong
* jtv (n=jtv@125.25.88.144.adsl.dynamic.totbb.net) has joined #launchpad-reviews
<noodles775> bigjools: also with the dasbp title, was it an explicit decision to do '"mozilla-firefox" binary package in Ubuntu Karmic for i386' rather than without the 'for' as suggested by mpt: '"mozilla-firefox" binary package in Ubuntu Karmic i386'
<bigjools> noodles775: it was not, I missed that
<bigjools> well spotted
<noodles775> bigjools: Do you think it would be worthwhile moving all those relationship portlets inline in the dasbpr index? From here it looks like it would not only make maintenance easier but would reduce the line-count?
<bigjools> noodles775: we can't do that
<bigjools> they're in a different context
<noodles775> Why's that? I thought they weren't re-used?
<noodles775> hrm.
<bigjools> nothing to do with re-use :(
<noodles775> I'm not yet understanding... wouldn't tal:define="relationships view/breaks" just become...
<noodles775> oh, right, separate view.
<bigjools> exactly
<bigjools> we could potentially do it by moving stuff to the dasbpr view
<bigjools> but that's more work than I want to take on for this change
<noodles775> yep, I think it'd be a good cleanup, but agree - another branch :)
<bigjools> ideally, they'd be macros in the page template
<noodles775> bigjools: We're currently mixing two different list styles for those relationships...
<noodles775> I notice that packagerelationship-list is also used by sourcepackage-index.pt :/
<noodles775> So ideally, it should be outputting a bunch of dd's for this current template, but that would then leave the sourcepackage-index template with mixed list types.
* bigjools checks
<noodles775> Rather than <dl><dt>term</dt><dd>first</dd><dd>second</dd></dl> it's doing <dl><dt>term</dt><ul><li>first</li><li>second</li></ul></dl>
<noodles775> I'm not sure what the best thing to do is... probably to leave your template as is, but put an XXX in the packagerelationship-list.pt to a bug for updating the sourcepackage-index template, identifying that they should be updated to dd's when that template is updated (for consistency - currently the sourcepackage-index template uses h3 followed by a ul).
<bigjools> I'm still trying to find a page that shows the error
<noodles775> bigjools: there is no error visually, it's just that the lists semantically do not make sense...
<bigjools> ok
<noodles775> I'm not even sure that it would validate (ie. having <ul> inside a <dl> etc.)
* bigjools is being thick
<noodles775> But we could fix it when the sourcepackage-index.pt is updated? What do you think?
<bigjools> still trying to get my head around where the problem is
<noodles775> A more obvious example would be <table><td><tr>...</tr></td></table> - a tr cannot be inside a td... it doesn't make sense.
<noodles775> Similarly, when a browser sees a <dl> it expects to find inside <dt>'s followed by one or more <dd>'s, not a <dt> followed by a <ul>.
* deryck (n=deryck@samba/team/deryck) has joined #launchpad-reviews
<bigjools> noodles775: I can't see where you mean
<bigjools> how is sourcepackage-index.pt affected?
<noodles775> it uses the same +render-list that all these relationship templates do, afaics?
<noodles775> bigjools: I'll do a diff as part of my reply - hopefully that'll make more sense.
<bigjools> are you sure?
<noodles775> bigjools: I think so... I find it hard to explain things with just irc :), where as a diff is much more precise :)
<bigjools> I'm being super thick
<bigjools> ah
* bigjools found it
<noodles775> Great! I'm approving it now (just noting the things we discussed).
<bigjools> noodles775: actually won't it be ok?
<bigjools> I use <dd tal:content=....
<noodles775> bigjools: in the package relationships section?
<bigjools> in the portlets
<bigjools> sorry
<bigjools> in the detail portlet
<bigjools> nowhere else uses a dl
<bigjools> it's a lone dt
<noodles775> yeah, that one is fine, it's the lists in the package relationships section, like this: http://pastebin.ubuntu.com/264322/
<noodles775> Yes, that's part of the problem.
<bigjools> really?
<bigjools> thought it would be ok
<noodles775> A dt should only ever exist inside a dl (afaik)
<bigjools> ok
<bigjools> so shall I just make it <strong> ?
<noodles775> bigjools: You could - but then you'd need to ensure that it gets a new-line break etc... I think semantically a dl is the right way to go (as we have else-where with the 3-0 templates). I'm doing a quick diff now...
<bigjools> okidoki
<noodles775> bigjools: so it's just a matter of doing this for each of the relationship portlets: http://pastebin.ubuntu.com/264324/
<bigjools> noodles775: ok, great!
<noodles775> It doesn't affect the display. And we can update the packagerelationship-list.pt to print dd's when we do the only other callsite.