218568 - Clean up charting UI

Status: RESOLVED FIXED

(Bugzilla :: Reporting/Charting, defect, P1)

Description

[Gervase Markham [:gerv]]

Having played with the new charts, there are a couple of tweaks I'd like to make
to them to simplify the UI.

Gerv

Comment 1

[Gervase Markham [:gerv]]

Attachment: Patch v.1

This patch:

- Rearranges the two sections on the form for defining a chart to plot
- Makes various clarifying wording changes
- Adds a "this is in beta" disclaimer (the only change in the big rearranged 
  block)
- Explains how Subscriptions work

Gerv

Comment 2

[Gervase Markham [:gerv]]

Comment on attachment 131031 [details] [diff] [review]
Patch v.1

Dave,

You reviewed bug 16009 originally - could you look at this? Nothing
controversial here.

Gerv

Comment 3

[Gervase Markham [:gerv]]

Comment on attachment 131031 [details] [diff] [review]
Patch v.1

Rats. I thought I'd got this in for 2.17.6 - it's the patch which marks the
charting system as a beta. <sigh>. Never mind - Dave, could you review this?

Gerv

Comment 4

[Dave Miller [:justdave]]

Comment on attachment 131031 [details] [diff] [review]
Patch v.1

>+Administrators may mark data sets as public, so they show up in everyone's list. All others are not public, and you have to explicitly subscribe to them in order

There a reason this line wraps so wide?

Everything else looks good from a review of the code.  Haven't had a chance to
test it yet though.

Comment 5

[Christian Reis]

Comment on attachment 131031 [details] [diff] [review]
Patch v.1


Overall, the change looks okay. It would be nice to be able to see the changes
in a rendered page, but I won't hold your review for that (but I will nitpick
later if I don't like it <wink>) 

There are a couple of questions below, take them into account if you feel they
are relevant. 

I would consider it okay to move on my r+ to a newer patch, and request
approval on that (I might take a glance at it if I have a minute).

>Index: template/en/default/reports/create-chart.html.tmpl
>===================================================================
>RCS file: /cvsroot/mozilla/webtools/bugzilla/template/en/default/reports/create-chart.html.tmpl,v
>retrieving revision 1.1
>diff -u -r1.1 create-chart.html.tmpl
>--- template/en/default/reports/create-chart.html.tmpl	25 Jun 2003 23:23:02 -0000	1.1
>+++ template/en/default/reports/create-chart.html.tmpl	7 Sep 2003 16:11:07 -0000
>@@ -59,17 +59,93 @@
>   
> [% gttext = "Grand Total" %]
> 
>-<h3>Current Data Sets:</h3>
>+<form method="get" action="chart.cgi" name="chartform">  

This section was moved up, but its <h3> was not preserved (it used to be Select
Data Sets) -- intentional?

>+    <font color="red">
>+      Note: this new charting system is in beta. This means that retention of
>+      data or defined data sets is on a best-efforts basis only, and cannot be
>+      guaranteed. Please file any bugs you find or enhancement ideas you think
>+      of.
>+    </font>

I really can't appreciate the use of <font>, can't we change that so <span
color="red">? And do you feel this *needs* to be red?

>+          <i>You do not have permissions to see any data sets, or none
>+             exist.</i>

Perhaps make this nicer:

  There are currently no visible data sets.

or maybe:

  No data sets exist, or none are visible to you.

(but I consider messages with "or"s in them bad UI)

>+        <noscript>
>+          <td>
>+            <input type="submit" name="action-assemble" value="Update -->">

Shouldn't that be &gt;?

>+        <noscript>
>+          <td>
>+            <input type="submit" name="action-assemble" value="Update -->">

Again.

>+        <td align="left">
>+          <label for="name" accesskey="N">
>+            <select name="name" id="name" style="width: 15em"
>+                    size="5" multiple="multiple"
>+              [% FOREACH x = name.keys.sort %]
>+                <option value="[% name.$x FILTER html %]"
>+                  [%# " selected" IF lsearch(default.name, x) != -1 %]>

Why is this line commented out? It was originally commented out, but I'm more
curious at to the use of this "default" creature, which is created for action
'view' in chart.cgi but never used.

(It also makes me wonder why this template is called create-chart when the sub
is called view)

If it's broken, at least mark it with an XXX?

+	 <td style="text-align: center; vertical-align: middle;"> 

Why are we using style for this table cell but not for most of the others?
valign="middle" should work, no?

>+  <script>
>+    document.chartform.category[0].selected = true;
>+    catSelected();
>+    subcatSelected();
>+  </script>

I see you're using plain <script> in the charting templates, make sure this is
intentional (the rest of bugzilla uses type and language a bit inconsistently)

>               [% IF series.creator != 0 %]
>                 [% IF series.subscribed %]

This line is stale (it changed to isSubscribed), but what I am really pushing
for here is that this should *REALLY* be a n && clause, since you do [% END %]
[% ELSE %] below.

>+<p>
>+Administrators may mark data sets as public, so they show up in everyone's list. All others are not public, and you 

maybe s/so/which/ 

have to explicitly subscribe to them in order

perhaps s/have to/must/ 

>Index: template/en/default/reports/edit-series.html.tmpl
>+  <b>Creator</b>: 
>+  [% IF creator.email %]
>+    <a href="mailto:[% creator.email FILTER html %]">
>+    [% creator.email FILTER html %]</a>
>+  [% ELSE %]
>+    None (automatically created by [% terms.Bugzilla %])

This None echoes a bit awkward, so consider if we could drop it and just leave
the parenthesized explanation.

template/en/default/search/search-create-series.html.tmpl
>-[% button_name = "I'm Feeling Buggy" %]
>+[% button_name = "Create / Search" %]

A button labelled "Create / Search" is really going to confuse me when I look
at that page rendered (though the original isn't much better :)

Thanks!

Comment 6

[Gervase Markham [:gerv]]

> This section was moved up, but its <h3> was not preserved (it used to be Select
> Data Sets) -- intentional?

Yes. :-)

> (It also makes me wonder why this template is called create-chart when the sub
> is called view)

It's called create-chart rather than create because there are several sorts of
report in the "reports" template directory. It's create rather than view because
 a create() sub would imply it created something, which it doesn't really.

Perhaps it's confusing, but it's a bit late now. :-|

> A button labelled "Create / Search" is really going to confuse me when I look
> at that page rendered (though the original isn't much better :)

The problem is that what the page does depends on the radio button below. I
think people will want to try out searches to make sure they have the right one,
then flip the radio to "create" and create it. 

Gerv

Comment 7

[Gervase Markham [:gerv]]

Attachment: Patch v.2

This patch fixes all review comments.

Gerv

Comment 8

[Gervase Markham [:gerv]]

Requesting approval; kiko said his review carried, although I've marked it again
in case he wants to have another look (see top of comment 5).

Gerv

Comment 9

[Dave Miller [:justdave]]

errr... ?

Comment 10

[Gervase Markham [:gerv]]

Oops - sorry. Misset the flag :-)

Gerv

Comment 11

[Christian Reis]

I'll try and take a look at this, but here's something I thought up:

>> A button labelled "Create / Search" is really going to confuse me when I look
>> at that page rendered (though the original isn't much better :)
>
> The problem is that what the page does depends on the radio button below. I
> think people will want to try out searches to make sure they have the right one,
> then flip the radio to "create" and create it. 

Would it be possible to use two submit buttons instead of radiobutton +
Create/Search button? That would be cleaner, I think.

Could you post an HTML dump of the page somewhere so I can take a look?

Comment 12

[Christian Reis]

> +                  [%# " selected" IF lsearch(default.name, x) != -1 %]>

Did you miss this part of my review, or is this part intentional?

Also, there are some extra bits in series-common and chart.cgi, are they
supposed to be in here too?

Comment 13

[Dave Miller [:justdave]]

there's open questions from kiko still...

Comment 14

[Gervase Markham [:gerv]]

Attachment: Patch v.3

This addresses kiko's points - we now have two buttons, and that commented-out
line has been removed. The new code is intentional, yes :-)

HTML dumps are available here:
http://www.gerv.net/temp/query.cgi.html
http://www.gerv.net/temp/chart.cgi.html
(Images may not work.)

Gerv

Comment 15

[Christian Reis]

Comment on attachment 136479 [details] [diff] [review]
Patch v.3


I guess I should review this or else you'll keep adding surprise bits to this
patch overnight :-)

I have some comments on the general UI:

chart.cgi.html:

  - I think we're missing an "Add Data Set" heading for the top part (I had to
figure that out by myself <wink>). Maybe the correct approach is to place the
add data set bits *under* the list of data sets. 
  - The Add to List button should probably be top-valigned
  - It would be *really* nice if the Date Range entries had a colspan of 2 to
make it render nicely in a narrow window -- my browser windows are usually
700px and you get linebreaks there.
  - I don't particularly like the 2px border around the Data Sets controls --
the header already separates them from the controls above. Perhaps indenting
the controls under the header would be an idea.
  - It isn't very clear that Sum/Remove are relative to the sets in the list. I
would suggest displaying them horizontally aligned, and then using a horizontal
separator or spacing to group the "chart this list" controls.
  - It's unclear what cumulate does, but I'm not sure how to make that clearer.

query.cgi.html

  - The horizontal rule to separate the data sets is redundant, and should go.
  - The New (add below) select control should be the last one. It makes
understanding the textboxes under it a whole lot easier if they are grayed out
by default (when at least one set exists) and only editable when the user
intentionally makes a selection on the New item. Bonus points for a focus shift
to the input entry when selecting the new item!
  - Should the Run Search button be separated from the form controls and
perhaps prefixed with a label to the effect of "(To see the bugs that would
currently be a part of this data set, (( Run this Search ))"? I think the or is
just confusing there, since the real task at hand is creating new data sets. Or
at least that's how I read the UI :-)

I'm unsure as to how to handle this review-wise. On one hand I would like your
changes to go in ASAP; OTOH this bug's summary allows for this sort of
suggestion. The code in the patch looks fine to me, save a few comments above
that reflect on it. What do you think?

Comment 16

[Gervase Markham [:gerv]]

> chart.cgi.html:

> - I think we're missing an "Add Data Set" heading for the top part 

This would lead to one heading directly under another, which would look odd.

> Maybe the correct approach is to place the add data set bits *under* the list 
> of data sets. 

The original implementation had this; however, it made the page less stable
because the Add To List section kept moving further and further down.

>  - The Add to List button should probably be top-valigned

Any particular reason? It seems OK to me in the middle.

>  - It would be *really* nice if the Date Range entries had a colspan of 2 

Your wish is my command.

> - It isn't very clear that Sum/Remove are relative to the sets in the list. I
> would suggest displaying them horizontally aligned, and then using a  
> horizontal separator or spacing to group the "chart this list" controls.

I'm not quite sure what you mean here; but "Chart This List" ignores the
checkboxes, so I wouldn't want to put Sum and Remove near that button.

> query.cgi.html

> - Should the Run Search button be separated from the form controls and
>  perhaps prefixed with a label to the effect of 

See what you think of the new positioning. It has the unfortunate side-effect of
making it the default button, but I think it's no win the right place.

All other comments addressed as requested. "Screenshots" updated.

Gerv

Comment 17

[Gervase Markham [:gerv]]

Attachment: Patch v.4

Comment 18

[Christian Reis]

More comments:

Create Data Set:

- Shouldn't the boolean chart come *before* the data set controls?
- How about having Run every X day(s) and Visible to all laid out horizontally
under the select controls to make the page width more or less nicely consistent?

Create Chart:

- Sum/Remove confuse me still. Are they Add/Remove? Sum/Subtract?
- I still think the Add to List button could use a position tweak; we could try
under the select boxes (to indicate it applies to all of them), or top-aligning
(you didn't like that though).

I'll take a good look at the code now.

Comment 19

[Christian Reis]

Comment on attachment 136629 [details] [diff] [review]
Patch v.4


As a note, it would be cool if you could use an HTML checker on the pages
generated to try and pick up errors left over..

>Index: template/en/default/reports/create-chart.html.tmpl
>+        <th>Name:</th>

[snip]

>+        <td align="left">
>+          <label for="name" accesskey="N">

Should this label be around the Name: above, or is this the right way to do
what you want here?

>+    <table cellspacing="2" cellpadding="2">
>       <tr>
>         <th>Select</th>
>-        <th>As</th>
>+        <th>Label</th>
>         <th></th>
>         <th>Data Set</th>
>-        <th>Subs</th>
>+        <th>Subs (<a href="#subs">?</a>)</th>

The Subs header and the link don't do much for me; Subs is just confusing, and
the link will usually do only a partial scroll of the page (since the help text
is at the bottom). This is an opinion/nit, feel free to ignore.

>Index: template/en/default/reports/series-common.html.tmpl
>===================================================================
>RCS file: /cvsroot/mozilla/webtools/bugzilla/template/en/default/reports/series-common.html.tmpl,v
>retrieving revision 1.1
>diff -u -r1.1 series-common.html.tmpl
>--- template/en/default/reports/series-common.html.tmpl	25 Jun 2003 23:23:01 -0000	1.1
>+++ template/en/default/reports/series-common.html.tmpl	1 Dec 2003 23:06:52 -0000
>@@ -66,7 +66,7 @@
>   }
>   
>   [% IF newtext %]
>-    subcatwidget.options[i] = new Option("[% newtext FILTER js %]", "");
>+    subcatwidget.options[i++] = new Option("[% newtext FILTER js %]", "");
>   [% END %]  

Hmmm, I don't see much the point of this change, since i is a local variable
and doesn't seem to be used after this statement, but I may be just confused.

>Index: template/en/default/reports/series.html.tmpl
> 
> <script>
>+  document.chartform.category[0].selected = true;
>+  catSelected();
>   checkNewState();
> </script>

This won't validate, script wants a type and language.

These are just nits and you can take what you will, you've got r=kiko.

Comment 20

[Gervase Markham [:gerv]]

> Should this label be around the Name: above, or is this the right way to do
> what you want here?

Doesn't make too much odds; putting it around the actual label makes it
clickable, but that's not necessarily useful here.

> The Subs header and the link don't do much for me

Link removed.

> Hmmm, I don't see much the point of this change, since i is a local variable

Remnant of aborted code change. Reversed.

> This won't validate, script wants a type and language.

Fixed.

Gerv

Comment 21

[Gervase Markham [:gerv]]

Attachment: Patch v.5

This is the patch for checkin, just for the record.

Gerv

Comment 22

[Gervase Markham [:gerv]]

Fixed.

Checking in template/en/default/reports/create-chart.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/reports/create-chart.html.tmpl,v
 <--  create-chart.html.tmpl
new revision: 1.3; previous revision: 1.2
done
Checking in template/en/default/reports/edit-series.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/reports/edit-series.html.tmpl,v
 <--  edit-series.html.tmpl
new revision: 1.2; previous revision: 1.1
done
Checking in template/en/default/reports/series.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/reports/series.html.tmpl,v
 <--  series.html.tmpl
new revision: 1.2; previous revision: 1.1
done
Checking in template/en/default/search/search-create-series.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/search/search-create-series.html.tmpl,v
 <--  search-create-series.html.tmpl
new revision: 1.3; previous revision: 1.2
done
Checking in template/en/default/search/form.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/search/form.html.tmpl,v 
<--  form.html.tmpl
new revision: 1.21; previous revision: 1.20
done

Gerv

Comment 23

[Vlad Dascalu]

The tree is red due to this as well. Words "bugs" must be templatized in templates.

Comment 24

[Gervase Markham [:gerv]]

Bustage fixed.

Checking in template/en/default/search/search-create-series.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/search/search-create-series.html.tmpl,v
 <--  search-create-series.html.tmpl
new revision: 1.4; previous revision: 1.3
done
Checking in template/en/default/reports/create-chart.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/reports/create-chart.html.tmpl,v
 <--  create-chart.html.tmpl
new revision: 1.4; previous revision: 1.3
done

Gerv