Code review comment for lp:~gmb/launchpad/ajax-dupefinder-has-risen-from-the-grave

Revision history for this message
Graham Binns (gmb) wrote :

> > > + <td colspan="2">
> > > + <p>
> > > + Please describe the bug in a few words, for example,
> > "weather
> > > + applet crashes on logout":
> > > + </p>
> >
> > I don't think the <p></p> is needed here (i.e. <td>s can have contain
> > inline content)... unless it affects layout.
> >

It does. The paragraphing is deliberate.

> > > + </td>
> > > + </tr>
> > > + <tal:title_widget tal:define="widget nocall:view/widgets/title">
> > > + <tal:comment replace="nothing">
> > > + The desire to have more control over the styling of this
> > widget
> > > + prevents us from using the widget_row macro here.
> > > + </tal:comment>
> > > + <tr>
> > > + <td tal:define="field_name widget/context/__name__;
> > > + error python:view.getFieldError(field_name);
> > > + error_class python:error and 'error' or
> > None;"
> >
> > I think you can say this now:
> >
> > ('error' if error else None)
> >
> > (Woohoo!)

Indeed. Fixed.

> > > + <p>
> > > + <a id="filebug-form-url"
> > > + tal:attributes="href view/inline_filebug_form_url"
> > > + style="display: none;"></a>
> > > + <a id="duplicate-search-url"
> > > + tal:attributes="href view/duplicate_search_url"
> > > + style="display: none;"></a>
> > > + </p>
> >
> > Maybe it's simpler to put the display:none on the <p>? I think empty
> > paragraphs still influence layout.

Fixed.

« Back to merge proposal