mattab commented 13 years ago

It would be great for usability, reporting efficiency and consistency, that all graphs in Piwik have possibility to select which metrics to plot.

In addition to the current implementation, the following features would be nice:

Saving and restoring the picked metrics for each user
Comparing the conversions of different goals
Make cart metrics selectable in ecommerce reports. This is hard to do because ecommerce conversions are tracked with idgoal=ecommerceOrder, which means they are not in the main report but in the goals dimension.

mattab commented 12 years ago

Attachment: Overflow displayed in ff8 overflow not hidden.PNG

mattab commented 13 years ago

Note: modules could also define the default metrics to plot. For example, under "Goals > By segment", when clicking on the graph icon, by default the Goal Conversions count would be plotted (rather than the default Visits for others).

mattab commented 13 years ago

See also: #2159 maybe similar implementation.

For ui we can get some look at how it could be done

mattab commented 13 years ago

Now that we support Custom Date Range, by default Piwik will plot the "days" within the range. However it would be cool / useful to be able to plot Weekly / monthly / yearly stats for the selected date range.

timo-bes commented 12 years ago

(In [5408]) refs #1820 metrics picker, refs #1454 using Api.get for cross-plugin evolution graphs

timo-bes commented 12 years ago

(In [5421]) refs #1820 added metrics picker to more core reports. some language updates to make metrics consistent and short. new api methods for comparing metrics in Referers and VisitsSummary. minor tweaks.

timo-bes commented 12 years ago

(In [5422]) refs #1820 added expected test output for new method Referers.getVisitsPerRefererType

mattab commented 12 years ago

Feedback

Warning: it is NOT recommended to update translations (appart from english) in SVN. Currently the translation process uses a server run by Noah from the team which is used by most translators, this contains the latest version of all translators. Some also send it by email. But the mods to the German translation will be erased in the next update! Please contact at translations@piwik.org for details on how to modify translations.
Very Cool new feature!!! Will make data analysis so much easier and more powerful.

Code review

I have some requests for usability
- when the box is ticked, the new metrics should be plotted in the background. So that the user can see in real time what the graph will be when he exists the mouse. When box is unticked, metric is removed in the graph in the background (popover still displayed).
- I like that the icon displays on hover. But, can you also add the text "Metrics to plot" next to the icon at all times, this makes it more visible (some people won't see the new icon) and easier to understand.
Metrics selector does not appear in the Ecommerce Overview graph, or in any Goal report.
Function "getVisitsPerRefererType": useful, but
- only returns visits, using standard columns would be better practise here. Also output is not standard api output.
- I think controllers should only use the available getRefererType (see output). But, is the problem that it does not contain a fixed metadata ID for each row to easily identify them (eg. "website", "direct") regarding of language? I could easily add this info in the api output if that is the problem?
- vote for revert changes in controller and this new function
array_slice($selectableColumns, 1, -4), => please no magic numbers, try replace with variable name or even just inline the variable like function($countPossibleItems = 10);

Cheers!

timo-bes commented 12 years ago

Replying to matt:

I like that the icon displays on hover. But, can you also add the text "Metrics to plot" next to the icon at all times, this makes it more visible (some people won't see the new icon) and easier to understand.

The text "Metrics to plot" is quite long and we would have to ensure that there is enough space. This would mean that the legend would have very little space when evolution graphs are displayed in reports with two columns. When there's not enough space, the text "metrics to plot" appears left of the icon at the moment. But that only works because the popover is covering part of the legend.

Function "getVisitsPerRefererType": useful, but

only returns visits, using standard columns would be better practise here. Also output is not standard api output.

I think controllers should only use the available getRefererType (see output). But, is the problem that it does not contain a fixed metadata ID for each row to easily identify them (eg. "website", "direct") regarding of language? I could easily add this info in the api output if that is the problem?

vote for revert changes in controller and this new function

I came up with another solution. The metrics picker can now select rows and columns. See the Referrers report after my next commit (coming in a few minutes). This way, we don't need the new API method anymore. I will revert.

array_slice($selectableColumns, 1, -4), => please no magic numbers, try replace with variable name or even just inline the variable like function($countPossibleItems = 10);

This refers to Piwik_VisitFrequency_Controller::getEvolutionGraph. I create the $selectableColumns array and modify it immediately afterwards. The metrics that are only available for period=day are inserted in the middle. The numbers in array_slice are used to determine where to put the additional metrics. Since the array that is sliced is created only 5 lines before, I don't see a problem with that.

timo-bes commented 12 years ago

(In [5443]) refs #1820 reverted Referers.getVisitsPerRefererType

timo-bes commented 12 years ago

(In [5444]) refs #1820 added row picker to evolution graphs

timo-bes commented 12 years ago

(In [5445]) refs #1820 metrics picker for goal reports

timo-bes commented 12 years ago

Replying to matt:

Metrics selector does not appear in the Ecommerce Overview graph, or in any Goal report.

I added a picker for conversions and conversion rate to goal reports. I don't have ecommerce data set up but the changes might apply to ecommerce as well. Can you please check?

It would be nice to compare the conversions of different goals but that would be quite hard to do and this has already taken way more time than I thought. We can add that to the list of future improvements. I would like to create a list in the ticket description if you give me the rights to do so.

mattab commented 12 years ago

Great code updates, nice!

Code review:

Good that you added 2 metrics for goals, there is also a third one to add for all goals: "revenue"
The graph on the Ecommerce only shows "conversion rate" and "ecommerce orders", it does not show abandoned cart metrics and Total revenue, AOV, etc. Is it possible to add it?
- For ecommerce data, you can use the VisitorGenerator plugin which will generate ecommerce activity (make sure you enable ecommerce for the website first).
the legend would have very little space when evolution graphs are displayed in reports with two columns. I did some tests and I think that with 2 metrics plotted, there is still space to write "Metrics to plot", in 2 column report. I think it would help discoverability of the feature, since already the icon only displays on hover of a small vertical band of pixels, so we must make the feature more visible I think :)
In Referers > Overview > In the metrics dropdown, if I untick "Direct entry", then the graph reloads with "No data". Instead when a tick is missing restore the default tick. This is working for the metric itself (it restores to "Visits") but not with the report below.
// TODO: please review - this code doesn't work anymore with multiple rows What exactly does not work with multiple rows? I don't see any impact in the UI of this change? So yes it should be ok. You can remove from en.php file only, it will be automatically removed from other languages files during next release package.
It would be nice to compare the conversions of different goals but that would be quite hard to do it would be interesting, but not urgent to add (definitely later iterations)... because it is hard to compare goals together, since they often represent different things.
How much work would it be to add the Metrics picker to Bar and Pie charts as well, to provide consistent experience across all Piwik?

Note: You should now have permission to edit tickets

Keep up the great UI improvements Timo!

mattab commented 12 years ago

(In [5447]) Refs #1820 Timo sorry for this feedback, you're right in this case magic numbers are acceptable, adding a small comment to clarify

timo-bes commented 12 years ago

Replying to matt:

The graph on the Ecommerce only shows "conversion rate" and "ecommerce orders", it does not show abandoned cart metrics and Total revenue, AOV, etc. Is it possible to add it? I added items and avg_order_revenue for goals reports. The cart metrics are in a separate report (idGoal=ecommerceAbandonedCart), therefore it's not easy to display them.

the legend would have very little space when evolution graphs are displayed in reports with two columns. I did some tests and I think that with 2 metrics plotted, there is still space to write "Metrics to plot", in 2 column report. I think it would help discoverability of the feature, since already the icon only displays on hover of a small vertical band of pixels, so we must make the feature more visible I think :)

But what if you have three metrics or a language like German where labels are 1.5-2times as long? I made the icon always visible but faded out. This should improve the visibility of the feature as well.

// TODO: please review - this code doesn't work anymore with multiple rows What exactly does not work with multiple rows? I don't see any impact in the UI of this change? So yes it should be ok. You can remove from en.php file only, it will be automatically removed from other languages files during next release package.

The column translation was set to "Visits (from Search Engines)" for example. That doesn't work with multiple rows because the label needs to be different in each row. The UI code handles that by default if a label is set in the column that should be plotted. The text is now "Search Engines (Visits)".

timo-bes commented 12 years ago

(In [5449]) refs #1820 misc improvements to the metrics picker

timo-bes commented 12 years ago

(In [5450]) refs #1820 updating all languages and tests after removing language string

timo-bes commented 12 years ago

(In [5451]) refs #1820 metrics picker for pie and bar chart

timo-bes commented 12 years ago

For pie charts, only one metric can be plotted. The picker is rendered as a select in the data table footer.

For bar charts, we can plot multiple metrics - picker is rendered like for line charts.

timo-bes commented 12 years ago

(In [5453]) refs #1820 metrics picker for pie/bar charts in more reports

mattab commented 12 years ago

Great stuff, exciting to see this feature coming together and implemented nicely in the new graph framework. Glad to see already the first major improvements to our "old graphs"!! :)

The text is now "Search Engines (Visits)". Sounds good.

Also, no problem not to include abandoned cart metrics for now.

Feedback:

Amazing to finally be able to plot "Page views per server hour" in Piwik!! such a basic requirement now met :) good stuff...
Supported metrics in picker: Metric Picker currently displays Unique Visitors/Visits/Actions. For consistency and completeness other columns should be available in particular:
- Goal conversions and Revenue: these can be displayed when enableShowGoals() was called in the controller. Maybe somehow you can reuse this function call to add the metrics to the list in setMetricsVariablesView() ?
- Ecommerce conversions (for reports enriched with ecommerce: countries, server time, custom variables, referrer...)
- Purchased products (idem)
For example, when looking at the Visits per Country chart, we can easily compare visits and Revenue and see if they are correlated for example. Also, it would be great to plot Purchased products metrics by country to see the number of items sold abroad
Pie chart metric picker Proposal: reuse the new picker for Pie charts too. Instead of displaying SELECT field in the graph footer, here is an idea. When picker icon hovered, the picker with radio buttons (instead of checkboxes) would open, and allow to plot another serie in this pie chart instead. It would be nice to always display "Visits" on the graph if visits are being plotted currently. I propose to add the metrics name "Visits" next to the picker icon on the right. When hovered, "Visits" would be replaced with the standard "Metric to plot". This way we keep the same design element which is much better and prettier too.
- Note: after this change, if it's easy also please export the metric name as part of the PNG export. The pie graph is more useful as PNG when showing the metrics name :)
But what if you have three metrics or a language like German where labels are 1.5-2times as long? I made the icon always visible but faded out. This should improve the visibility of the feature as well. Indeed this is much better thanks!! I have one last suggestion regarding this change:
- can we display the metrics picker on hover on the icon, rather than click (the popover will be just below the mouse so the experience would be clean) ?
- OR (other solution?) could we display the text "Metrics to plot" on hover, on the right of the icon, and show the popover onclick as it is now. In this case we should also add a alt= on the icon with "metrics to plot".
I noticed the picker was not in the dashboard, any particular reason? I think dashboard would definitely hugely benefit from it :)

mattab commented 12 years ago

(In [5456]) Refs #1820 Adding picker in few reports

timo-bes commented 12 years ago

(In [5457]) refs #1820 goal metrics in picker, normal metrics picker for pie chart, improved unit / tooltip handling for bar and pie chart

timo-bes commented 12 years ago

(In [5459]) refs #1820 metrics picker shows on hover

mattab commented 12 years ago

ohhh close to perfection ;)

timo-bes commented 12 years ago

For consistency and completeness other columns should be available in particular:

Goal conversions and Revenue: these can be displayed when enableShowGoals() was called in the controller. Maybe somehow you can reuse this function call to add the metrics to the list in setMetricsVariablesView() ?

Good idea to reuse this method. There is nb_conversions, nb_visits_converted and revenue. Can you explain the rationale behind the first two? I get the difference between the metrics but in a test setup I have, there are conversions but no visits converted and I don't know why. If only of them is present for each report, we should only enable the available metric. But how can we find out, which is "active"?

Ecommerce conversions (for reports enriched with ecommerce: countries, server time, custom variables, referrer...)

Purchased products (idem) Also, it would be great to plot Purchased products metrics by country to see the number of items sold abroad

Ecommerce conversions are tracked with idgoal=ecommerceOrder, which means they are not in the main report but in the goals dimension. We can add them later together with the conversions for user defined goals. At the moment, I don't have time for that because it's a largish change.

Pie chart metric picker

I reused the new picker. The legend is displayed in gray since there are no series colors. Apart from that, it looks like the legend in the other reports. This way, the user recognizes the UI elements instantaneously and we can reuse almost all the code.

Note: after this change, if it's easy also please export the metric name as part of the PNG export. The pie graph is more useful as PNG when showing the metrics name :)

The metric name is in the export done via JS. It's not in the static image graphs rendered by PHP. That's a task for the authors of the ImageGraph plugin.

I noticed the picker was not in the dashboard, any particular reason? I think dashboard would definitely hugely benefit from it :)

The widgets have overflow: hidden set. This cuts off the picker in many cases (especially in a narrow browser window). That's why I disabled the picker on the dashboard.

mattab commented 12 years ago

Code review: *``` // TODO: please review $requestString .= '&disable_generic_filters=1'; // if disable_generic_filters is set, sort filters won't be applied. // therefore, the parameters filter_sort_column and filter_sort_order don't have any effect. // sorting is needed if the plotted metric is changed on a bar graph and the new metric has // a different order of the rows than the main metric (e.g. most visits in france, but most // conversions in germany). // on the other hand, if this line is removed, table reports can't be displayed with goal // metrics anymore.

 This thing has always been a bit dodgy I must admit. Can you think of a good solution here? Could the call when picking a different metrics manually set disable_generic_filters=0 then the code in ViewDataTable.php would only set it to 1 if not already set to 0 ? maybe that works, but i'm not sure why we disable the generic filters here in the first place (bad memory of mine)
* I noticed that icon goes opacity 1 but the popover does not show. The hover works on a smaller surface. This causes a delay in the popover to appear. please put the popover on the larger surface possible
* Metrics to plot should be "Metric to plot" on the Pie chart since it plots one metric?
* Radio/Checkbox buttons and the metric name on the right, should have "cursor:pointer;" on hover to show they are clickable
* Btw the Bar charts look really cool, like the flashy colors... ;)
* > The widgets have `overflow: hidden` set. This cuts off the picker in many
 cases (especially in a narrow browser window). That's why I disabled the
 picker on the dashboard.
OK but IMO still very good to have for the large window browsers (I am in 1600 so could easily plot multiple columns!).
Please add in dashboard (ok if hidden for some users) :) killer feature!!

  * Self reminder to record these picked columns in the dashboard layout (and restore)

* Noticed that when you hover, then quickly out of canvas by the top of the icon, the popover stays displayed but the mouse is out hovering on the title for example.
 Somehow, the graph should detect that when the mouse is outside the graph the popover should hide. It seems to work in some cases but not all when leaving from the to of canvas..
* Otherwise, great changes and refactoring, and children method enableShowGoals() - clean!

* IMHO Still missing for being perfect usability-wise: Plot/unplot on ticket/uncheck

mattab commented 12 years ago

(In [5460]) Refs #1820 Updating icon for slightly cleaner version

timo-bes commented 12 years ago

$requestString .= '&disable_generic_filters=1';

I didn't solve this myself because I don't understand the effects either. If you remove the line, some reports don't work anymore. Could you maybe find out why?

In a "per goal" report, the picker could include the "Revenue, Conversions" for this specific Goal, in the picker list. For example, plot "Conversions to goal subscribe newsletter" for the top 10 keywords.

Can you explain this in more detail?

Please add in dashboard (ok if hidden for some users)

Why do we need overflow: hidden? Can we maybe remove that?

timo-bes commented 12 years ago

(In [5461]) refs #1820 metrics picker ui improvements

timo-bes commented 12 years ago

(In [5462]) refs #1820 new icon should be more opaque, metrics picker on dashboard

timo-bes commented 12 years ago

IMHO Still missing for being perfect usability-wise: Plot/unplot on ticket/uncheck

How about closing the popover and repolotting on click? Since the popover now opens on hover, it's easy to open it multiple times if you want to do more than one modification.

mattab commented 12 years ago

How about closing the popover and repolotting on click? Since the popover now opens on hover, it's easy to open it multiple times if you want to do more than one modification.

That sounds like the best of both worlds indeed! Except, don't remove the graph on Untick, unless leaving the popover.

This should be possible: 1) Visit is plotted 2) Want to plot Revenue only, untick Visit, popover stays open, tick Revenue 3) popover closes and revenue only is loaded on the graph

Does it sound good to you?

PS: the Pie chart picker at least could close/replot on click on the radio button.

Why do we need overflow: hidden? Can we maybe remove that?

I think it was needed to hide the images used for "column sort", that would otherwise appear in the widget on the next right out of nowhere. But, if you remove it, maybe you can find a better solution to hide these sort icons, than use overflow hidden?

edit: thanks for all follow up to the review!

mattab commented 12 years ago

(In [5464]) Refs #1820

Fix // TODO: please review $requestString .= '&disable_generic_filters=1' Now disabling generic filters speficically in charts
Also fix for issue that columns sorted was kept when changing to another table view, but the column might not exist or be visible

mattab commented 12 years ago

(In [5465]) Refs #1820 Fixing few bugs when plotting and switching views, and fixing sorting issue with generic filters

timo-bes commented 12 years ago

Why do we need overflow: hidden? Can we maybe remove that? I think it was needed to hide the images used for "column sort", that would otherwise appear in the widget on the next right out of nowhere. But, if you remove it, maybe you can find a better solution to hide these sort icons, than use overflow hidden?

Sorting columns in tables works just fine without overflow:hidden (at least for me). If I remember correctly, I changed the markup for that when introducing column documentations. Might that have done the trick? Do you remember in which browsers the problem occurred? Please check again.

thanks for all follow up to the review!

Thanks for all the review! Polishing takes a lot of time but it definately wort it.

timo-bes commented 12 years ago

(In [5471]) refs #1820 replot on metric select

timo-bes commented 12 years ago

In IE7, the icon doesn't have a transparent background. We used to do that wie PIE, but I can't find it anymore. Am I blind or did we remove the library? How can we do transparency in IE7 now?

mattab commented 12 years ago

Nearly there :)

See attached file that shows that the overflow is displayed outside of the widget, in FF8 (seems OK in Chrome and IE but not FF). It used to display only the sort images but now it looks like the whole cells are printed. There might be a trick to remove these and still leave the picker on (which looks nice)
When plotting 3 metrics, graphs can get small. One can use the "Maximise" feature of a widget. It would be perfect except the graphs is not redrawn after it opens in full screen. So, the graph legend still contains [...] even though the graph has white space on the right that canvas hasn't taken.
On a click on the exact tick box, the popover is not closed/replotted. only a click on the label does it
The checkbox and text next to it are not aligned horizontally, seems to be too low
In IE7, the icon doesn't have a transparent background. We used to do that wie PIE, but I can't find it anymore. Am I blind or did we remove the library? How can we do transparency in IE7 now? I think we removed this lib because it was causing some troubles... IE7, transparency, are 2 words that should not be mentioned together ;)

timo-bes commented 12 years ago

Ok, overflow:hidden is set on the widgets again. I worked around the problem by appending the popover to the body, not the graph. This makes the JS a little harder to understand but I couldn't find another way.

I also did cross-browser optimization. It works almost perfect in IE7 (the almost being a feature because it annoys IE7 users into updating) and perfect in IE89, Safari, Chrome and Firefox.

timo-bes commented 12 years ago

(In [5472]) refs #1820 cross-browser, dashboard optimizations, misc tweaks

timo-bes commented 12 years ago

(In [5473]) refs #1820 belongs to last commit

timo-bes commented 12 years ago

Everything should be done now (except the two things from the description). If nobody finds more bugs, I'll leave it like this for now.

mattab commented 12 years ago

Great the overflow works well. I don't really understand these hacks but it looks good ;)

One last (I tried fixing it but didn't make it). The text is not aligned well with the checkbox, it is slightly below which looks not as best as it should.

Feel free to close the ticket!

timo-bes commented 12 years ago

Replying to matt:

Great the overflow works well. I don't really understand these hacks but it looks good ;)

The popover is put into the body, therefore it won't be cut off by the widget container. The JS is pretty comlex already, imagine what a mess it would be if we would plot in the background.

The text is not aligned well with the checkbox, it is slightly below which looks not as best as it should.

That depends on which browser you use. I tried to find a configuration that works well in all browsers. I had to sacrifice a single pixel here and there to achieve that.

So, let's close the ticket. Yay! I still leave the future improvements in the description in case somebody wants to reopen it and work on those (maybe future me).

mattab commented 12 years ago

See Graphs picker wish list & improvements

timo-bes commented 12 years ago

(In [5533]) refs #1820 unique visitors can only be selected for period=day in referrer overview

timo-bes commented 12 years ago

(In [5580]) refs #1820 working around firefox css bug for metrics picker

nigawtester commented 9 years ago

x

matomo-org / matomo

Graphs should display a SELECT to choose the metrics to plot (visits, pages, etc.) #1820

Code review