Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sitekit Dashboard Locations Label Placement #2660

Closed
ivankruchkoff opened this issue Jan 20, 2021 · 5 comments
Closed

Sitekit Dashboard Locations Label Placement #2660

ivankruchkoff opened this issue Jan 20, 2021 · 5 comments
Labels
Module: Analytics Google Analytics module related issues P1 Medium priority QA: Eng Requires specialized QA by an engineer Rollover Issues which role over to the next sprint Type: Bug Something isn't working
Milestone

Comments

@ivankruchkoff
Copy link
Contributor

ivankruchkoff commented Jan 20, 2021

Bug Description

Site Kit Dashboard label for Locations overlaps with the text 'BY' when 100% of traffic comes from one location.

Steps to reproduce

  1. Using latest develop branch as I'm pretty sure the feature is behind a feature flag.
  2. Open Site Kit dashboard with Analytics module connected
  3. Scroll down to Users widget in Site Overview.
  4. Open Locations Tab.
  5. Ensure 100% of Locations are attributed to one location, e.g. United States
  6. See error

Screenshots

Screen Shot 2021-01-19 at 10 05 23 PM

Additional Context

  • PHP Version: NA
  • OS: OSX
  • Browser chrome Version 87.0.4280.141 (Official Build) (x86_64)
  • Plugin Version develop branch (7ee258d Head commit)
  • Device: [e.g. iPhone6]

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

Acceptance criteria

  • When the All Traffic widget pie chart only includes a single dimension (i.e. 100% of users come from that dimension), the percentage should still be displayed within the pie slice, it shouldn't overlap with the overall pie chart label.
    • This might be hard to reproduce consistently, please check in different browsers.

Implementation Brief

  • Inside UserDimensionsPieChart.js:
    • Perform a check to determine if the report prop for UserDimensionsPieChart only has 1 row:
      report?.[0]?.data?.rows?.length === 1 
  • If there is only one row, we need to add pieSliceText = 'none' to the UserDimensionsPieChart.chartOptions. This will hide the '100%' text that appears in the centre of the pie.
  • if there is more than one row, we do nothing, i.e. current behaviour
  • Note: You could do this around line 288 where a similar check is also performed

Test Coverage

  • All existing tests should pass; no new tests required

Visual Regression Changes

  • In cases where there is only a single row of data, the text at the centre of the pie should only read 'By {Dimension}', where Dimension is 'Channels', 'Locations' or 'Devices'. There should be no '100%' text.

QA Brief

  • There is an added story for this specific case under the All Traffic Widget now

To recreate the issue in WP:

  • Update assets/js/modules/analytics/components/dashboard/DashboardAllTrafficWidget/index.js so that pieChartReport.[0].data.rows only contains one row, for example:
    // Around line 107
    if ( pieChartReport && pieChartReport?.[0]?.data?.rows ) {
      pieChartReport[ 0 ].data.rows = pieChartReport[ 0 ].data.rows.slice( 0, 1 );
    }
  • Verify that the text at the centre of the pie in the All Traffic Widget only contains the dimension text (not the 100%):

Screenshot 2021-02-10 at 14 36 23

Changelog entry

  • Fix text issue with All Traffic pie chart.
@felixarntz felixarntz self-assigned this Jan 27, 2021
@felixarntz felixarntz added Module: Analytics Google Analytics module related issues P1 Medium priority Type: Bug Something isn't working Next Up labels Jan 27, 2021
@felixarntz felixarntz removed their assignment Feb 5, 2021
@johnPhillips johnPhillips self-assigned this Feb 10, 2021
@johnPhillips
Copy link
Contributor

johnPhillips commented Feb 10, 2021

Note: it is possible to recreate the effect of this bug on the 'channels', 'locations' and 'devices' tabs of the chart by changing the maxSlices chart option to be 1 here.

Edit: the above will visually recreate the issue, but just by telling the chart to only show one slice. To recreate the bug, the report prop itself must be modified to only contain one row of data, as described above in the QA brief.

@johnPhillips
Copy link
Contributor

When there is only a single row in the data as here, it seems the default behaviour of the chart is to put '100%' in the middle.

The problem is that we are overlaying a .googlesitekit-widget--analyticsAllTraffic__dimensions-chart-title div saying 'By channels', for example. So in the scenario where there is only one row of data, the two labels appear on top of each other.

We have a couple of options here:

  • Use pieSliceText: 'none' in the chart options to remove the '100%'. Note that we can change the text style or remove it, but we can't reposition it.
  • Remove our dimensions title overlay in this instance. An argument could be made that the user can already see that they selected 'by channels' and the percentage, so it might be a bit redundant in this scenario.
  • Reposition the dimensions title overlay in this scenario so that both it and the pieSliceText are visible.

Some (slightly rough and ready) examples to help visualise:

piesliceText: 'none' No overlay Repositioned overlay, e.g. to the top
Screenshot 2021-02-10 at 14 36 23 Screenshot 2021-02-10 at 14 42 17 Screenshot 2021-02-10 at 14 31 17

@felixarntz Let me know your preference, and I'll put together an IB 👍

@felixarntz
Copy link
Member

@johnPhillips The pieSliceText: 'none' approach looks good to me, let's go with that! We should of course make sure that that is only used for the 100% case (i.e. when there is just a single dimension value in the Analytics API response).

I agree that for 100% showing that number is somewhat pointless anyway since it is immediately visible because there is just one big slice for the entire pie.

@johnPhillips johnPhillips added the QA: Eng Requires specialized QA by an engineer label Feb 12, 2021
@fhollis fhollis added this to the Sprint 43 milestone Feb 15, 2021
@fhollis fhollis removed the Next Up label Feb 15, 2021
@felixarntz
Copy link
Member

IB ✅

@aaemnnosttv aaemnnosttv assigned tofumatt and unassigned tofumatt Mar 3, 2021
@aaemnnosttv aaemnnosttv assigned tofumatt and unassigned johnPhillips Mar 3, 2021
@tofumatt tofumatt assigned johnPhillips and unassigned tofumatt Mar 3, 2021
aaemnnosttv added a commit that referenced this issue Mar 5, 2021
Fix all traffic widget label when there is only a single row of data (#2660)
@aaemnnosttv aaemnnosttv removed their assignment Mar 5, 2021
@eugene-manuilov eugene-manuilov self-assigned this Mar 5, 2021
@eugene-manuilov
Copy link
Collaborator

QA ✔️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Module: Analytics Google Analytics module related issues P1 Medium priority QA: Eng Requires specialized QA by an engineer Rollover Issues which role over to the next sprint Type: Bug Something isn't working
Projects
None yet
Development

No branches or pull requests

7 participants