Code review comment for lp:~geser/launchpadlib/toc

=== modified file 'src/launchpadlib/wadl-to-refhtml.xsl'
--- src/launchpadlib/wadl-to-refhtml.xsl 2009-08-14 16:48:11 +0000
+++ src/launchpadlib/wadl-to-refhtml.xsl 2009-09-08 21:43:30 +0000
...
> @@ -413,12 +416,53 @@
> <h1><xsl:value-of select="$title" /></h1>
> <xsl:apply-templates select="wadl:doc"/>
>
> + <xsl:call-template name="table-of-contents" />
> <xsl:call-template name="top-level-collections" />
> <xsl:call-template name="entry-types" />
> </body>
> </html>
> </xsl:template>
>
> + <!-- Table of contents -->
> + <xsl:template name="table-of-contents">
> + <div id="toc">
> + <h2>Table of Contents</h2>
> + <h3>Top-level collections</h3>

> + <xsl:for-each
> + select="key('id', 'service-root-json')/wadl:param/wadl:link">
> + <xsl:sort select="../@name" />
> + <xsl:variable name="collection_id"
> + select="substring-after(@resource_type, '#')" />
> + <xsl:if test="string-length($collection_id) > 0">

This is not well-formed. The '>' should be '&gt;'. I surprissed this works.

> + <a href="#{$collection_id}">
> + <xsl:call-template name="get-title-or-id">
> + <xsl:with-param name="element" select="key('id', $collection_id)" />
> + </xsl:call-template>
> + </a><br/>
> + </xsl:if>
> + </xsl:for-each>

This is generating inline elements that are commingled with with block
elements. The browser engine knows this cannot happen and will correct this by
wrapping these in a block element of its choosing (<p>). I would have expect
this list to be a list <ul> or <ol>. At the very least, these <a> and
<br /> elements should be in a div.

> + <h3>Entry types</h3>

> + <xsl:for-each select="wadl:resource_type[
> + @id != 'service-root'
> + and @id != 'HostedFile'
> + and not(contains(@id, 'page-resource'))
> + ]">
> + <xsl:sort select="@id" />
> + <xsl:variable name="id" select="./@id"/>
> + <xsl:variable name="top_level_collections"
> + select="key('id', 'service-root-json')//@resource_type[
> + substring-after(., '#') = $id]" />
> + <xsl:if test="not($top_level_collections[contains(., $id)])">
> + <a href="#{$id}">
> + <xsl:call-template name="get-title-or-id">
> + <xsl:with-param name="element" select="." />
> + </xsl:call-template>
> + </a><br/>
> + </xsl:if>
> + </xsl:for-each>

Again I would expect an <ol> or <ul> wrapping these links in the block flow.

> + </div>
> + </xsl:template>
...