google / site-kit-wp

Site Kit is a one-stop solution for WordPress users to use everything Google has to offer to make them successful on the web.
https://sitekit.withgoogle.com
Apache License 2.0
1.23k stars 284 forks source link

Different Date Formats on Site Kit Dashboard widgets #5830

Closed wpdarren closed 7 months ago

wpdarren commented 2 years ago

Bug Description

Creating this ticket for UX/UI discussion. Is there a reason why we have two different date formats on the dashboard. In the All Traffic widget we have MonDD (i.e. Sep 03) where in the charts are, i.e. Search traffic widget, the format is MM/DD/YY Wondering if the date formats should be consistent?

Screenshots

image.png


Do not alter or remove anything below. The following sections will be managed by moderators only.

Acceptance criteria

image

Implementation Brief

Note that the Google Chart does reduce the number of ticks to avoid overcrowding based on the chart width, so we may be able to leverage the native behaviour for the last AC point.

Changing the date format:

Enforce the chart labels to never wrap in multiple lines and limit items along the x-axis:

Update the existing charts to not pass the first date item as a tick: Removing the first item from the ticks, prevents the charts from automatically cutting of the first item and replacing it with an elipsis.

Test Coverage

QA Brief

Changelog entry

andreas271828 commented 1 year ago

Worse yet, it's all American and not considering the locale or format settings of the Wordpress site.

ronnyadsetts commented 1 year ago

See this issue re. localities for the date format:

https://github.com/google/site-kit-wp/issues/1518

It surely can't be that hard to respect the locality preferences of the end users' site when displaying a date. Either that or use an unambiguous format rather than one that leaves you having to think about it.

derweili commented 9 months ago

Google charts automatically reduce the number of ticks based on the available space. So the only setting we to set here it to ensure the labels never wrap. This can be done with the hAxis.maxTextLines option.

tofumatt commented 9 months ago

The IB here looks good, but I wonder about this line in the ACs:

The All Visitors widget chart should be updated so it displays the number of date labels using the same logic as the Search Traffic and Performance widgets, providing a consistent look across all SK Dashboard charts.

The issue reported is about the date format, but not the number of ticks in the chart. Our reduced tick count is an intentional so I don't think should be changed.

@techanvil Was that point brought up elsewhere? If it's a request from UX that's cool, but the original reduced tick was—if I recall correctly—part of a series of chart UX updates and was meant to reduce noise in that chart 🙂


(Original IB review feedback follows, written before realising the issue is in the ACs.)

What is the reason for removing the manual haxis.ticks creation in our charts?

If we do that, the number of ticks is quite high. It gets pretty cramped and harder-to-read, especially because if you want the exact date (and year, omitted from the new string, reasonably) you can hover over the chart line.

Here's Google Charts handling the ticks:

CleanShot 2023-11-29 at 23 15 07

Here's our tick creation:

CleanShot 2023-11-29 at 23 15 19

derweili commented 9 months ago

There is an alternative option to manually setting the ticks. Google charts has an minTextSpacing option.

Using this, a spacing between the automatic ticks can be defined. If there is not enough space available, some ticks will be hidden.

tofumatt commented 9 months ago

@derweili Oh, awesome, okay, that seems better than us manually creating the ticks.

I'll move this back to IB with you but I think we just need to add a minTextSpacing to things so we preserve (approximately) the current tick spacing and this can be good-to-go then 🙂

techanvil commented 9 months ago

Hi @tofumatt, to answer your question:

@techanvil Was that point brought up elsewhere? If it's a request from UX that's cool, but the original reduced tick was—if I recall correctly—part of a series of chart UX updates and was meant to reduce noise in that chart 🙂

This is a request from Mariya, following a question from Sigal - here's the relevant Slack conversation.

It seems to me we should set the same minTextSpacing across all of these charts to provide a consistent look, while preventing the ticks from looking too crowded. Even the Search Traffic and Performance charts can look a bit crowded with the default minTextSpacing value, so a higher value all round should be an overall improvement.

@derweili, an additional point, not explicitly stated in the AC but to ensure that visual consistency we should make sure that the first tick on the All Traffic chart doesn't get cut off in the way that can be seen in Matt's first screenshot:

image

AFAIK this doesn't happen on the Search Traffic and Performance charts with the stock behaviour, so it's worth reviewing the differences between these charts and the All Traffic one to see how to resolve this.

derweili commented 9 months ago

@techanvil good point. This actually happens as soon as you delete the manual ticks. Will need to investigate the chart options on how to fix that.

derweili commented 9 months ago

I tested the minTextSpacing, unfortuntally this does not perform well and ends up in weird spacings.

Bildschirmfoto 2023-11-30 um 15 31 51

It also results in the first tick being cut off.

derweili commented 9 months ago

I did some more research on the chart library. There are multiple ways to limit the number of ticks, but all of them can result in the issue seen in the above screenshot, where the ticks are unevenly placed along the x-axis. Therefore I think there is no way to to fix this using the configuration options provided by the chart library.

Instead I think we should reuse the logic we already have implemented for the UserCountGraph component to limit the ticks. Therefore we could create a custom useLineChartTicks() hook, that generates the ticks and subscribes to resize event.

derweili commented 9 months ago

I was unsure where to place the new custom hook in. I know we have a hooks folder, but this new hook only makes sense to be used with the GoogleChart so I thought placing it next to the GoogleChart component makes more sense to me then placing it in the generic hooks folder.

techanvil commented 9 months ago

Hey @derweili, if you review this Slack thread you'll see that we've been explicitly asked to move away from this existing UserCountGraph logic due to the results it gives. Therefore, applying it to all of the graphs would not be the correct approach here.

Actually, I've taken a look and found the minTextSpacing option is working OK for me. Could you have applied it to the UserCountGraph graph without replacing the tick logic which could explain your odd results?

As to the first tick getting cut off - if we provide a full range of dates for the ticks and remove the first one, that seems sufficient to avoid the problem.

Here's a quick PoC and a video of the results:

https://github.com/google/site-kit-wp/assets/18395600/04908304-6e4d-4024-b221-5f05d0141ab2

If we do want to show the first tick, we can achieve this by increasing the chartArea.left value to make room for it. However this means we lose the left-hand alignment of the chart edge to the surrounding context, which we've aimed to avoid to date.

diff --git a/assets/js/modules/analytics/components/dashboard/DashboardAllTrafficWidgetGA4/UserCountGraph.js b/assets/js/modules/analytics/components/dashboard/DashboardAllTrafficWidgetGA4/UserCountGraph.js
index 119b11aeaf..891ebdeaf2 100644
--- a/assets/js/modules/analytics/components/dashboard/DashboardAllTrafficWidgetGA4/UserCountGraph.js
+++ b/assets/js/modules/analytics/components/dashboard/DashboardAllTrafficWidgetGA4/UserCountGraph.js
@@ -124,8 +124,6 @@ export default function UserCountGraph( props ) {
        tick.setDate( tick.getDate() + 1 );
    }

-   chartOptions.hAxis.ticks.shift(); // Get rid of the first tick which may otherwise be cut off.
-
    // Set the `max` height of the chart to `undefined` so that the chart will
    // show all content, but only if the report is loaded/has data.
    if (
@@ -184,7 +182,7 @@ UserCountGraph.chartOptions = {
    width: '100%',
    colors: [ '#3c7251' ],
    chartArea: {
-       left: 7,
+       left: 20,
        right: 40,
        height: 300,
        top: 21,

image

Alternatively we could consider moving the Y axis to the left hand of the chart as that should also provide some additional space for the X axis to render.

Anyhow, to move this along, I think we should take the approach of dropping the first tick, we can see how we get on with that and iterate on it from there.

derweili commented 9 months ago

I tested the options with the minTextSpacing again. Was not able to replicate either. Maybe I had a weird combination of options when trying this yesterday.

Updated IB to not use the manual tick generation but to use the automatic one provided by the Chart library.

The value 100 for minTextSpacing seams to replicate a similar behavior to what we currently have.

tofumatt commented 9 months ago

@derweili The IB mentions useLineChartTicks() in the tests, but I think that can be removed.

Regarding the creation of manual tick marks for WPDashboardUniqueVisitorsChartGA4 and other places… can Google Charts manage the ticks with minTextSpacing everywhere without us actually providing explicit ticks? No worries if not, but given that dimension is already using our date data, I'm curious if we could omit the tick data generation entirely.

Otherwise looks good. 🙂

derweili commented 9 months ago

@tofumatt Google Charts could do this, but this results in the issue @techanvil outlined in this comment, where the first item get's cut of and replaced with a button and a tooltip.

The trick to prevent this is to remove the first tick. That's why we have to explicitly provide in the ticks.

tofumatt commented 9 months ago

Ah, right, sounds good then, thanks for all the investigation on this one! 👍🏻

IB ✅

mohitwp commented 8 months ago

QA Update ⚠️

@kuasha420 I noticed that under viewports having width 324px and less. The Dates are getting hide and not showing.

image

image

Pass cases-

![image](https://github.com/google/site-kit-wp/assets/94359491/b7a6edb3-74d5-499b-888c-5de8a3d5733c) ![image](https://github.com/google/site-kit-wp/assets/94359491/74577998-b5e6-442b-a5cf-76ea27251b13) ![image](https://github.com/google/site-kit-wp/assets/94359491/ff029291-0cee-4c42-b364-b69c6a766064) ![image](https://github.com/google/site-kit-wp/assets/94359491/1e0ca294-11b6-43c6-9cb3-a3dc7620a805) ![image](https://github.com/google/site-kit-wp/assets/94359491/db3cf6da-cb23-4ea1-98c3-f3c424829239) ![image](https://github.com/google/site-kit-wp/assets/94359491/c56b13b0-b80d-46e8-8ad9-5ee353719250) ![image](https://github.com/google/site-kit-wp/assets/94359491/75291b61-f9d6-4711-b8b7-710e0b9ac198) ![image](https://github.com/google/site-kit-wp/assets/94359491/3e79e712-b494-4079-91c2-1f95c7b05062) ![image](https://github.com/google/site-kit-wp/assets/94359491/c7b1e845-ed76-466f-82b0-2abe99742b90) ![image](https://github.com/google/site-kit-wp/assets/94359491/ee333867-f140-40c2-9b5a-4613539d8fb3) ![image](https://github.com/google/site-kit-wp/assets/94359491/4f702960-147b-45cd-9bfd-c940428b36ec)
mohitwp commented 8 months ago

@kuasha420 Did you get chance to look into this ? Can I move this to execution ?

mohitwp commented 8 months ago

QA Update ⚠️

@kuasha420

Tested on dev environment. Note : I've noticed that "All visitors" widget graph first date is getting cut on viewports having width 338px and less than it. It is a regression due to changes introduced in this ticket because on latest environment "All visitors" widgets date is not getting cut.

Let me know if this is quick fix and you want to create follow up issue. For now I created a separate ticket as this is minor issue. https://github.com/google/site-kit-wp/issues/8151

main environment

image

Latest environment

image

mohitwp commented 7 months ago

QA Update ✅

As discussed with @kuasha420 in slack. Issue reported above will be fix in a separate PR - I created a new ticket for this https://github.com/google/site-kit-wp/issues/8151 .

![image](https://github.com/google/site-kit-wp/assets/94359491/9f4351f4-fb57-417f-828e-dd40459b60d6) ![image](https://github.com/google/site-kit-wp/assets/94359491/e4a8d9ff-e673-40f3-ae8a-d05e7f38ec97) ![image](https://github.com/google/site-kit-wp/assets/94359491/b7a6edb3-74d5-499b-888c-5de8a3d5733c) ![image](https://github.com/google/site-kit-wp/assets/94359491/74577998-b5e6-442b-a5cf-76ea27251b13) ![image](https://github.com/google/site-kit-wp/assets/94359491/ff029291-0cee-4c42-b364-b69c6a766064) ![image](https://github.com/google/site-kit-wp/assets/94359491/1e0ca294-11b6-43c6-9cb3-a3dc7620a805) ![image](https://github.com/google/site-kit-wp/assets/94359491/db3cf6da-cb23-4ea1-98c3-f3c424829239) ![image](https://github.com/google/site-kit-wp/assets/94359491/c56b13b0-b80d-46e8-8ad9-5ee353719250) ![image](https://github.com/google/site-kit-wp/assets/94359491/75291b61-f9d6-4711-b8b7-710e0b9ac198) ![image](https://github.com/google/site-kit-wp/assets/94359491/3e79e712-b494-4079-91c2-1f95c7b05062) ![image](https://github.com/google/site-kit-wp/assets/94359491/c7b1e845-ed76-466f-82b0-2abe99742b90) ![image](https://github.com/google/site-kit-wp/assets/94359491/ee333867-f140-40c2-9b5a-4613539d8fb3) ![image](https://github.com/google/site-kit-wp/assets/94359491/4f702960-147b-45cd-9bfd-c940428b36ec)