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).
>

But:

noodles775> allenap: actually, I just realised - you've got two h1's there - as the heading in the branding is already h1 - so you should definitely be using an h2 in the 'heading' fill-slot.
<noodles775> Here's a good example to work from: https://edge.launchpad.net/ubuntu/+search
<noodles775> (Curtis did that one :) ).

And looking at some other approved landings:

https://edge.launchpad.net/ubuntu/+ppas

it looks like you should be using the default h2 for the breadcrumb heading even though it repeats the heading, and the form's view.label defining the h1 heading in the main content.

Again, I'm sorry that I can't yet be more definitive :/.

« Back to merge proposal