Code review comment for lp:~allenap/launchpad/ui-convert-filebug-3.0-bug-415263-search

Revision history for this message
Michael Nelson (michael.nelson) wrote :

Hi Gavin,

It looks *so* much friendlier!

As I mentioned via irc, I had a few things that I was uncertain about - so I'll need to defer to Martin or Curtis:

1. At first I thought that you should be using the default <h2>context.title</h2> in the 'heading' slot - but then you rightly pointed out that this would fall into the case of the same heading twice - so we think that overriding with the h1 heading is correct (fits with Curtis' email "because showing two identical titles is stupid" but the page_title is *not* the same as the context.title here - hence the uncertainty).

2. Since you are (currently) over-riding the heading slot with an h1, you will not define view.label, otherwise the form machinery will add a second h1 to the page.

3. I'm aware that the way we designate privacy generally has changed (see base-layout.pt), but in your case you're indicating that the future bug to be created will be private - I don't know if this needs to follow the same style? Martin?

Sorry - that wasn't much help in terms of a review :/

<noodles775> Thanks allenap.... initial thoughts - it looks *much* friendlier!
<allenap> noodles775: The diff generated is complete rubbish :( I forget to target the branch to devel.
<noodles775> np.
<allenap> noodles775: But, yeah, it does look much better :)
<allenap> noodles775: I'm going to recreate the mp, so don't comment on it yet.
<noodles775> ok
* sinzui has quit (Remote closed the connection)
<allenap> noodles775: https://code.edge.launchpad.net/~allenap/launchpad/ui-convert-filebug-3.0-bug-415263-search/+merge/10321
<noodles775> allenap: have you made ff a private project for that screenshot? Or why is the report private by default there (I saw that it's not locally)
<allenap> noodles775: I set bugs private by default for ff.
<noodles775> allenap: OK, so it looks great, but there are two things I'm uncertain about.
<allenap> noodles775: Hit me with the bad news!
<noodles775> allenap: first, you've used <h1>Report a bug</h1> in the 'heading' slot, where as, if I've understood sinzui's email correctly, that should be displaying the default h2 context.title there,
<noodles775> allenap: and your h1 should be displayed by lpform macro automatically (if you define view.label).
<noodles775> allenap: secondly, I haven't seen the latest discussions on the privacy indicators, but I thought we were using a global privacy indicator, rather than custom ones like you've put into the 'heading' slot there?
* noodles775 checks the template.
<allenap> noodles775: The first point - displaying context.title in the heading slot - seems strange. The title is already on the page, just above.
<allenap> noodles775: The second point - view.label - I'll look into now.
* jtv has quit ("Leaving.")
<allenap> noodles775: The privacy indicator was preserved from the old version of the template. I don't know what the new style is for private things, but I'm happy to adopt it :)
<noodles775> allenap: the view.label for forms is described in the email on lp-dev.
<noodles775> allenap: and yes, with the first point, I think you're right - it fits into the case where displaying the same heading twice is silly :)
* henninge has quit ("Ex-Chat")
<noodles775> allenap: and with the privacy - I just realised that in your case you're indicating that something *will be* private, not that something currently is private (ie. it's not that the ff project is private here), so I think that's fine - but I'd like to check with beuno. I'll add the above to the mp.
<allenap> noodles775: If I set view.label, the heading will appear right by the form. The heading slot in base-layout is above a place-holder for the upcoming breadcrumbs wizardry. So, right now they'll look the same, but when breadcrumbs start working, they'll be different. Which one do you think I should go for?
<noodles775> allenap: I honestly don't know. I wasn't aware that the breadcrumb change (which goes under the heading in the 'heading' slot) would be changing the heading there too.

review: Approve (ui)

« Back to merge proposal