Closed mattab closed 12 years ago
Attachment: Patch for 'visits by day of week' report. 3184.diff.tar.gz
Attachment: Patch for date selector changes. 3184_dateselector.diff.tar.gz
Attachment: New patch for this issue. 3184_dateselector.diff.tar.2.gz
Attachment: Patch for all websites title tooltip. 3184_allwebsites.diff.tar.gz
(In [6546]) Refs #3184, added ability to specify custom tooltips for datatable labels & modified Referers>Websites table to show full referer url as the tooltip. Also tweaked tooltip CSS style.
I uploaded a patch for the visits by day of week report. It wasn't a straightforward change, though, so a review might be a good idea.
Some notes on the patch:
Let me know what if it's ok to commit.
Since this patch is beyond the simple UX improvement I created a new ticket please commit there: #3275
Code review
VERY nice change, well done... also I really like that it only shows on the span, much better this way!
I put up a preliminary patch for the changes to the date selecting widget. It works, but I'm going to review it from a performance perspective. Either way, it's good to review :)
Code review calendar ux
firstOfViewedMonth = new Date(viewedYear, viewedMonth, 1),
the + in front of viewedMonth is the unary plus symbol
Did you put it because it was needed, ie. can the viewedMonth be a string?
If so it is should be better to do it at time of init variable.
I attached a new version of the date selector patch. I've optimized it to the point where calendar.js takes ~1/3-1/4 the time of the datepicker functions themselves. I've tested on chrome, firefox & IE and everything seems to work. There might be some issues w/ selecting a year on firefox, but it should be fixed. Let me know if it's good to commit.
(In [6632]) Refs #3184, hide anonymous user settings if no website visible by anonymous user.
(In [6634]) Refs #3184, hide 'done' div in UsersManager.tpl in HTML not in javascript, and move it to right of table so page doesn't jump when it's toggled.
(In [6635]) Refs #3184, remove unused translation entry.
Replying to capedfuzz:
I attached a new version of the date selector patch. I've optimized it to the point where calendar.js takes ~1/3-1/4 the time of the datepicker functions themselves. I've tested on chrome, firefox & IE and everything seems to work. There might be some issues w/ selecting a year on firefox, but it should be fixed. Let me know if it's good to commit.
Looks good! One more suggestion: when 2 dates are selected in left and right calendars, could you color all dates in between? this would clarify users that the active selection is a range :)
Replying to matt:
Looks good! One more suggestion: when 2 dates are selected in left and right calendars, could you color all dates in between? this would clarify users that the active selection is a range :)
I've thought about this some and I think it's possible but would require modifying the widget to do something similar to what GA does. Right now the two calendars can be for the same month, so if the range is in that one month, we'd either have to show the highlighted range in both, or just one. Either way, it'd be confusing.
The modification to make it more like GA's range picking widget would take some time though. I think it would be better to commit what I've got right now and work on the range selector change in a new ticket. What do you think?
Good point for date ranges within 1 month. Looks good to me!
I've run into an issue w/ adding the 'Total Conversions' column. Adding it stretches the table out so it overlaps w/ the table next to it. I think there needs to be some sort of UX abstraction to deal w/ a potentially large number of columns. Here are some ideas off the top of my head:
Commit [6656] refs this issue.
Add a right arrow icon to the end of a data table. When clicked, the rest of the page fades to a a darker color and the entire table is shown in a sort-of pop-up. For this idea, could also replace the last column w/ a blank column showing '...'. Hovering over the icon would show a list of the columns that are available.
I think this could be very nice, but it would actually "push" the other content to the right as it would be nice to still see other reports on the same page, but in darker layer.
Do you mind creating a separate ticket for this new feature? We could think more about it especially how we would use in other reports.
Feedback on the calendar:
Feedback calendar continued:
(In [6683]) Refs #3184, highlight 'today' in period selector.
@matt Created the ticket #3305 for the 'showing many columns in DataTables' issue.
(In [6693]) Refs #3184, add ability to set tooltip for links in top bar.
(In [6696]) Refs #3184, highlight whitespace when appropriate in period selector when period == week
(In [6697]) Refs #3184, color period selector whitespace when week is selected, and change period when whitespace is clicked for week & year periods.
(In [6698]) Refs #3184, make cursor a pointer when hovering over week/year periods that use period selector whitespace.
(In [6828]) Refs #3184, display tooltips for top bar links.
(In [6829]) Refs #3184, show N/A for anonymous user + admin access in Users Manager tab.
(In [6830]) Refs #3184, make sparkline in All Websites dashboard a link to individual site's dashboard.
(In [6875]) Refs #3184, change current period when date selector's selected period is clicked again.
(In [6877]) Refs #3184, add tooltip for selected period in period selector.
The all websites 'hover title' item was a bit more complex than I anticipated. I've uploaded a patch, could you do a quick review?
(In [6888]) Refs #3184, underline selected period in period selector when it can be clicked & refactor 'change period' tooltip showing code.
Replying to capedfuzz:
The all websites 'hover title' item was a bit more complex than I anticipated. I've uploaded a patch, could you do a quick review?
Looks good! Only suggestion is:
(In [6912]) Refs #3184, added tooltip to All Websites Dashboard's total summary that describes total visit evolution.
Notes:
// NOTE: no easy way to set last period date metadata when range of dates is requested.
// will be easier if DataTable_Array::metadata is removed, and metadata that is
// put there is put directly in Piwik_DataTable::metadata.
Do we not need the ability to set the matedata to the Array itself? if not, then please let's make the change if easy enough?
Replying to matt:
// NOTE: no easy way to set last period date metadata when range of dates is requested. // will be easier if DataTable_Array::metadata is removed, and metadata that is // put there is put directly in Piwik_DataTable::metadata.
Do we not need the ability to set the matedata to the Array itself? if not, then please let's make the change if easy enough?
The metadata in DataTable_Array is the metadata for each DataTable it holds. For example, the Array returned from an IndexedByDate query will associate idSite w/ Piwik_Site instances:
'0' => new Piwik_Site(0), '1' => new Piwik_Site(1), ...
along w/ other info like the period.
If the information is set in DataTable::metadata, however, then there's no need for DataTable_Array::metadata.
I've made the change in my local branch for #3124, however, I don't think it's that high a priority. What do you think?
If the change improves or does not worsen memory usage, and if it simplifies the code a bit, it would be nice if you could commit it (technical debt is best avoided when possible :))
(In [6920]) Fixes #3184, show pageviews in all websites dashboard instead of actions.
(In [6931]) Refs #3184, fixing build.
(In [6933]) Refs #3184, fixing build.
(In [6937]) Refs #3184, fix ApiGetReportMetadata_yearTest.
(In [6950]) Refs #3184 fixes sparklines for Pageviews
(In [7092]) Refs #3184, fix firefox period selector issue when double clicking the selected period and make sure double clicking period selector labels doesn't select the text.
I've spent few hours with first time piwik users and here are some thoughts:
General UI misc screens
All websites dashboard