Code review comment for lp:~adeuring/launchpad/bug-201015-bug-branch-search

Revision history for this message
Curtis Hovey (sinzui) wrote :

Hi Abel.

I am sorry you had to touch this page and that I am the reviewer; this
page has irked me for a long time because it does not look consistent
with the rest of Launchpad. Instead of asking you to fix this page, I
will propose code and text fixes that I think are easy to make.

There are some general problems that are never being addresses when
we update this page. I really want to take the time to fix the general
problem with this branch, and you get to close some extra bugs too:

    * Hard to scan. There is not enough white-space in the layout and
      all the text is the same weight and size. Information is lost
      like a tree in a forest.
    * Labels/legends do not match their field content.
    * We have added to this page without ever asking if we need
      to rearrange to the order of the sections to match what users
      want to use.

Your screenshot illustrated these three problems. Notice that the
Tags any/all radio buttons look like there are subordinate to
Show only bugs affecting me. I also question why some checkbox text are
bold and others are normal.

What follows is my analysis and proposed fix: you can see a see a picture
and patch at:
    http://people.canonical.com/~curtis/bug-search.png
    http://people.canonical.com/~curtis/bug-search.patch

My assumption was that the primary problem with this form is that
it is crafted rather than generated. After investigating how I could
make this better, I know understand this issue is really one of
markup+css neglect. All pages that use <legend> in a collapsible field
except this one. They are used to add an expansible section to a form.
This page is intended to be long with all sections visible. We do not
have css rules for this page!

    * In style.css, replace
        legend {font-weight: bold;}

      with
        .long td {
            padding-right: 1em;
            }
        .long fieldset {
            margin-top: 1em;
            }
        .long legend {
            color: #666;
            font-weight: bold;
            font-size: 123.1%;
            }

     * In bugtask-macros-tableview.pt, update the form class in
       <metal:advanced_search_form define-macro="advanced_search_form"> from
          <form class="lesser" name="search" method="get" action="">

       to
          <form class="long" name="search" method="get" action="">

We do not want to use the .lesser class because this page is not showing
a long listing, and it is forcing all the text to be the same small font.
We are replacing the css rules for this page with rules that create space
between the sections and make each section distinct.

As for the legend text and sections:

    * Series-specific => Milestones, components, and tags
      * Remove the width="40%" rules from the <td> and the remove the
        empty <td>.
      * Move tags below milestones and components
      * Use <td span="2"> so that the layout can fit the space:
        Move the help to the right of the input,
        Move the radio button below the field that controls them.

    * Other => Bug relationships
      * Move the "Show ..." group of option to the first column,
        and make your addition to the bottom
      * Move the "Hide ..." group of options to the second columm.

As for the inconsistent bolding, the default rendering of the widgets
insert a style="font-weight: normal" for the checkbox and radiobutton
labels, so this crafted form should do the same.

Bug #150867 [Status defaults in advanced search page are misleading]
https://bugs.edge.launchpad.net/malone/+bug/150867
    * Can we add a note in the status section that explains that only open
      bugs will be searched...
      Or, pre-select the open status to make it clear what status will be
      searched and the user can change them.

Bug #262894 [Series-specific isn't series-specific]
https://bugs.edge.launchpad.net/malone/+bug/262894
    * Rename the section to "Milestones, components, and tags"

Bug #262896 [Advanced search page unnecessarily wraps milestone names]
https://bugs.edge.launchpad.net/malone/+bug/262896
    * Remove the width in the <td>

Bug #310689 [advanced search is confusing]
    * This overlaps with the previous bugs except for one point
    * Move tags to the first item in the "Milestones, components, and tags"

review: Needs Fixing (ui)

« Back to merge proposal