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

Make "New" metric badge layout more consistent between Data Blocks and Report Tables. #7001

Closed
techanvil opened this issue May 2, 2023 · 15 comments
Labels
Exp: SP Module: Analytics Google Analytics module related issues P0 High priority Type: Enhancement Improvement of an existing feature

Comments

@techanvil
Copy link
Collaborator

techanvil commented May 2, 2023

Feature Description

Since the implementation of #6904, the "New" badges in Report Tables display above the metric titles. However, the badges in Data Blocks still display beside/below the titles, which is a bit inconsistent, particularly so at narrower viewport sizes. This is most evident on the Entity Dashboard. To illustrate:

The new Report Table layout:

image.png

The Data Block layout:

image.png


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

Acceptance criteria

  • The "New" metric badges in Data Blocks should follow the same design as those in the Report Tables.
  • Specifically that means the Conversions metric in the Search Traffic widget on the Main Dashboard, and the Engagement Rate metric on the Search Traffic widget and Sessions and Engagement Rate metrics on the Overall Page Metrics widget on the Entity Dashboard should be updated to appear above the metric names instead of beside/below them.

Implementation Brief

Within assets/js/components/DataBlock/index.js:

  • Update the badge prop to accept a boolean or React node. If the value true is passed, render a hidden Badge in order to allow the layout to align with a badge-containing Data Block.
  • Move the badge above the rendered title.

Within assets/js/modules/search-console/components/dashboard/SearchFunnelWidgetGA4/Overview/index.js and assets/js/modules/analytics/components/dashboard/DashboardOverallPageMetricsWidgetGA4.js:

  • Pass true for the badge prop to the DataBlock components which don't have a "New" badge.

Make additional DOM and CSS changes as necessary to bring the with-badge layout more in line with Report Tables as per the AC. Note, though, that due to the way Data Blocks are designed, it doesn't make sense to try to right-align the badges/titles, rather they should be left or center-aligned (for the default vs "button" case) as per the existing design.

Finish off this PR: #7008

Test Coverage

  • Update VRT reference images as needed.

QA Brief

  • Ensure ga4Reporting feature flag is enabled.
  • Set up Site Kit on a site with Analytics data and Analytics connected, including GA4 and GA4 data.
  • Scroll down to "Search traffic over the last 28 days" and observe the "new" badge above "Conversions". It should be above the "Conversions" text and not beside it, with no layout issues in the rest of the data blocks/tab selects for the chart, eg.:
  • Navigate to the Entity Dashboard, and verify the "new" badge is correctly aligned for the "Engagement Rate" metric on the "Search traffic" widget, and "Sessions" and "Engagement Rate" metrics on the Overall Page Metrics widget.
  • Verify these at various breakpoints.
CleanShot 2023-05-11 at 12 09 07

QA Update

Changelog entry

  • Enhance consistency of layouts with "New" badges.
@techanvil techanvil added Module: Analytics Google Analytics module related issues Type: Enhancement Improvement of an existing feature labels May 2, 2023
@marrrmarrr
Copy link
Collaborator

@techanvil thanks for raising this! definitely makes sense to fix this so it aligns with how the new badges are displayed on the main dashboard. +1 on following the design from the main dashboard.

@tofumatt
Copy link
Collaborator

tofumatt commented May 5, 2023

@techanvil Seems good, but is there any reason not to have the "should fit badge"/fitBadge behaviour available all of the time? Having to provide either a badge or a signal to render normally "without" a badge is a little ergonomically awkward to me.

If we need it not to break other DataBlocks that's fine, though I'm not clear on why and it'd be nicer to have it "just work". 🤔

@tofumatt tofumatt assigned techanvil and unassigned tofumatt May 5, 2023
@techanvil
Copy link
Collaborator Author

Hey @tofumatt, maybe I wasn't clear enough in the IB - the fitBadge should default to false, so it only needs to be supplied in the case where a DataBlock without a badge is rendered next to one with a badge, in which case it needs to match the layout with an empty space where the badge would be.

I've updated the IB to specify that fitBadge should default to false to make things a bit clearer. Hopefully that sorts it out, please see what you think.

@techanvil techanvil assigned tofumatt and unassigned techanvil May 5, 2023
@tofumatt
Copy link
Collaborator

tofumatt commented May 5, 2023

@techanvil Ah, yes, I did understand that, but I'm wondering why we can't have that styling/behaviour always be present for the DataBlock? Why not have the fitBadge behaviour, that will allow for placement next to a "badged" block, regardless? Does it mess up styling for some DataBlocks when there are no badges? It seems like it'd be nicer to never need to use that prop.

@tofumatt tofumatt assigned techanvil and unassigned tofumatt May 5, 2023
@techanvil
Copy link
Collaborator Author

Hey @tofumatt, thanks for clarifying. It's a good suggestion, I've looked into it and have determined it is possible to do this in a way that avoids the conditional styling, we just need to pass in a prop to tell the component it should render a hidden badge to allow it to match the with-badge layout. I've updated the IB and PoC, please see what you think.

@techanvil techanvil assigned tofumatt and unassigned techanvil May 9, 2023
@tofumatt
Copy link
Collaborator

tofumatt commented May 9, 2023

@techanvil I think this is better, but I still wonder about needing to pass the boolean at all. Is it possible to use the "with-badge" layout all of the time and render a hidden badge when a component isn't passed?

I'd just love it if we didn't need to pass that prop unless we are actually sending it our own badge 🤔

@tofumatt tofumatt assigned techanvil and unassigned tofumatt May 9, 2023
@techanvil
Copy link
Collaborator Author

@techanvil I think this is better, but I still wonder about needing to pass the boolean at all. Is it possible to use the "with-badge" layout all of the time and render a hidden badge when a component isn't passed?

I'd just love it if we didn't need to pass that prop unless we are actually sending it our own badge thinking

The problem with this is we don't always want to show an empty space above the title where a badge would be. We only want to do so for Data Blocks which are displayed next to ones with a badge, which means those in the GA4 Search Traffic and Overall Page Metrics widgets; the DataBlock component is used in many other places.

An additional point to consider is the "New" badges will be removed in time, but we may yet find other cases where a badge would be useful, it generally seems quite a good idea to have a flexible component in this regard. But really this is a bit of a moot point as, AFAICT, the fact we use DataBlock in various scenarios where we don't want the empty space is the overriding factor.

@techanvil techanvil removed their assignment May 9, 2023
@tofumatt tofumatt assigned tofumatt and unassigned tofumatt May 9, 2023
@techanvil techanvil assigned techanvil and unassigned techanvil May 11, 2023
@techanvil techanvil assigned aaemnnosttv and unassigned techanvil May 12, 2023
@aaemnnosttv aaemnnosttv removed their assignment May 12, 2023
@wpdarren wpdarren self-assigned this May 15, 2023
@wpdarren
Copy link
Collaborator

QA Update: ⚠️

@techanvil this passes the QAB after checking on various desktop and mobile viewports but I do have an observation about the consistency of how the new badges are aligned in the different areas. It seems we have a mix of centre, left and right alignments as you can see from the screencast below.

I feel we should have the badge aligned to the centre, but it's likely that there's a reason why we haven't gone down this path for all of the data titles. Interested in your thoughts.

db.mp4

@techanvil
Copy link
Collaborator Author

techanvil commented May 15, 2023

Hi @wpdarren, thanks for raising this, it's a good question.

The design choices here stem from the first issue where we moved the "New" badges above the titles in the Top Content widget (or, in more generic terms, the ReportTable, although the only instance of it with the badges is in this widget).

Reviewing that issue, #6904, the AC specified that the positioning should follow this design in Figma, with the "New" badges right-aligned to match the titles.

Realising that the "New" button placement now looked inconsistent for the other widgets (the badge still flowing to the right of the titles), we created this issue to bring the remaining widgets in line with the Top Content widget (again in more generic terms this meant updating the DataBlock component which the remaining widgets that have badges use).

However, when looking at the title alignment in these widgets, it's not right-aligned like the Top Content widget, rather the Search Funnel widget has centre-aligned titles and the Overall Page Metrics widget has left-aligned. Clearly right-aligning the badges won't look good here, so we've followed the principle taken for the Top Content widget to align the badges with their titles.

Personally I think this is the right approach stylistically as I don't think a centre-aligned badge will look so good with a left or right-aligned title below it. However if you want we can double check this with Mariya or Sigal. What do you think?

@techanvil techanvil removed their assignment May 15, 2023
@wpdarren
Copy link
Collaborator

@techanvil I am happy to go with what you have said. Will get this into approval.

@wpdarren
Copy link
Collaborator

QA Update: ✅

Verified:

  • On the "Search traffic over the last 28 days" widget the "new" badge is above "Conversions".
  • Checked on various desktop, mobile and tablet viewpoints.
  • On the Entity Dashboard the "new" badge is correctly aligned for the "Engagement Rate" metric on the "Search traffic" widget, and "Sessions" and "Engagement Rate" metrics on the Overall Page Metrics widget.
  • Checked on various desktop, mobile and tablet viewpoints.
Screenshots

image
image
image
image
image
image
image

Side note: I did notice a few styling issues on mobile, which initially I thought were related to the badge, but I have been able to recreate them in a previous version of Site Kit, so will submit a ticket for them.

@wpdarren wpdarren removed their assignment May 15, 2023
@techanvil
Copy link
Collaborator Author

As @wpdarren has spotted a regression caused by this issue I'm moving it back to Execution for a fix.

Morning team, I’ve just spotted a small regression on the All Traffic widget. This is on all versions of the dashboard (i.e. UA/GA4 main and entity dashboard) When you click on a legend on the pie chart, ie Social, there’s a styling issue as highlighted on the screenshot.

image (5).png

@techanvil techanvil self-assigned this May 16, 2023
@techanvil techanvil assigned tofumatt and unassigned techanvil May 16, 2023
@tofumatt tofumatt assigned wpdarren and unassigned tofumatt May 16, 2023
@wpdarren
Copy link
Collaborator

QA Update: ✅

Verified:

  • Checked that the issue reported with the All Traffic widget is fixed on the main, entity and view only dashboard.
  • Checked the above also with GA4Reporting enabled and both UA and GA4 versions. All good.
  • Ran through a quick check that the new badges continue to be display as expected. No issues found.
  • Ran through checks of the All Traffic and other widgets to ensure no additional styling problems.

image

@wpdarren wpdarren removed their assignment May 16, 2023
@aaemnnosttv
Copy link
Collaborator

LGTM 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Exp: SP Module: Analytics Google Analytics module related issues P0 High priority Type: Enhancement Improvement of an existing feature
Projects
None yet
Development

No branches or pull requests

5 participants