Move tile percentage badges to bottom of tile

[mxbclang]

Feature Description

Per discussion in this Asana Bug Bash task, we should move the percentage badges on tiles to be below the tile data to match this Figma design:

cc @sigal-teller

---

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

Acceptance criteria

• The metric change badge in numeric and text type key metric tiles should be relocated to the bottom of the tile according to the Figma design.
• It should be ensured that the typography of & spacing between different elements of the metric tiles are consistent with the Figma design.
• It should be ensured that the increased height of the metric tiles due to the above changes is consistently reflected across all metric tiles.

Implementation Brief

• Modify assets/js/components/KeyMetrics/MetricTileNumeric.js
  • Move ChangeBadge component bellow p.googlesitekit-km-widget-tile__subtext
  • Wrap it with div.googlesitekit-km-widget-tile__metric-change-container
  • Remove the current wrapper to prevent duplication
• Ensure all metric tiles align with the design. For reference, you can utilize the following style files:
  • assets/sass/components/key-metrics/_googlesitekit-km-widget-tile.scss
  • assets/sass/components/global/_googlesitekit-change-badge.scss
• Perform a cleanup to remove any styles that became redundant or unnecessary due to the aforementioned changes.

Test Coverage

• Update VRT reference images

QA Brief

• Setup Site Kit with keyMetrics and ga4Reporting feature flags
• Add key metrics that include badge
• Verify that they look like in the figma design

Changelog entry

• Move the Key Metric tile percentage badges to below the metrics.

[sigal-teller]

Noting that additional minor tweaks were made to the tiles so they would be able to contain the badge in the bottom:

1. Tiles height was increased to 150 px
2. String values (like top traffic source) font was changes to “title medium”
3. The spacing between the value and the text below (i.e “of x visitors) was reduced
4. In tiles that contains lists (like top pages, keywords etc.) the spacing between the list and the tile title was reduced by 3 pixels to align with the other tiles
5. Note that in the visit length value new format the “m” and the “s” are in smaller fonts then the numbers

Figma is updated with the new tiles layout, You can find it here

If needed for comparison, old tiles layout (current one) can be found here

[aaemnnosttv]

AC ✅

5. Note that in the visit length value new format the “m” and the “s” are in smaller fonts then the numbers

@sigal-teller GA shows the abbreviated units using the same font size and face (it's all one value) which is how we do it as well. Can we keep it this way?

[techanvil]

Hi @zutigrm, thanks for this IB. It's generally looking good, albeit a bit of a "PR in text form". Sometimes it's appropriate to be very explicit, though. The only real concern I have is around the duplication of .googlesitekit-km-widget-tile__metric-change-container within MetricTileNumeric.

By moving the change badge out of the current .googlesitekit-km-widget-tile__metric-change-container it means that current element is no longer a "change container" and more of a "metric container". Note how it would compare to MetricTileText where the metric and change badge are rendered separately but there's only one use of .googlesitekit-km-widget-tile__metric-change-container.

As an aside, drilling into the styling of .googlesitekit-km-widget-tile__metric-change-container it's evident that some of its styling will no longer be needed - gap for sure. It's not entirely clear the align-items would still have an effect either. This sort of thing would hopefully be addressed at implementation/review time anyway.

Of course, we don't need or want to get right into every last detail in the IB, but to my main point, I do think it's worth reconsidering the suggested use of .googlesitekit-km-widget-tile__metric-change-container to avoid a bit of semantic drift here. Wrapping the ChangeBadge in this class does make sense but retaining its use on the current element would not be so appropriate.

[zutigrm]

@techanvil thank you for the feedback. That is a very good point, thanks. I updated the IB

[techanvil]

Thanks for the rewrite, that looks great! IB LGTM. :white_check_mark:

[wpdarren]

QA Update: ❌

@zutigrm on smaller viewports, i.e. mobile, the height of the Most engaged traffic source and Top traffic source metric tiles are different than the other numeric and text type key metric tiles. See screenshots below. In the example, for iPhone 12 Pro, the other tiles are 358 x 109.91, but the height is different on these two tiles.

Screenshots

 

[zutigrm]

@wpdarren Thanks for checking this. I am not sure height of the tile is part of the scope, as per recent discussion height on mobile is supposed to be fluid, and there is this issue #7763 that is going to address some edits in design (and update the current design a bit). Changes we make to the height as part of this issue will most likely be overriden in the mentioned issue.

@techanvil Let me know what is your opinion here, as you are familiar with both issues as well

[wpdarren]

Thanks @zutigrm

I am happy to go with what you think is best. The reason why I queried it was because of this section in the AC:

It should be ensured that the increased height of the metric tiles due to the above changes is consistently reflected across all metric tiles.

Admittedly, it doesn't mention smaller viewports and all looks fine on desktop.

[zutigrm]

@wpdarren Yeah the the new issue details come after the AC is written for this one, as there is also new design added by Sigal just few days ago. Let's see what @techanvil thinks is the best approach here

[techanvil]

Thanks both, definitively worth checking but as @zutigrm has pointed out, the mobile tile height is intended to be derived from the content, so it's OK for tile heights to differ on mobile viewports.

Admittedly, the height is out by a few pixels from the current design (see below), but as Aleksej also mentioned we've got an update to the design coming in #7763, which will affect the tile heights, so I think it's safe to approve this as it is and take a fresh look in the QA for 7763.

[zutigrm]

Thanks @techanvil

Moving it back with you @wpdarren

[wpdarren]

QA Update: ✅

Verified:

• The metric change badge in numeric and text type key metric tiles are relocated to the bottom of the tile according to the Figma design.
• The typography of & spacing between different elements of the metric tiles are consistent with the Figma design.
• The increased height of the metric tiles due to the above changes is consistently reflected across all metric tiles.

Note: I observed an issue with the height of the tiles on smaller viewports, and this should be improved in 7763.

Screenshots

 

[nfmohit]

Hi @zutigrm & @wpdarren.

I can reproduce a broken layout for the "numeric" tiles when there is another tile with an error state present in the KMW area. Please see this screenshot:

It looks like this is a regression from this issue, specifically from this change.

CC: @techanvil @aaemnnosttv

[wpdarren]

QA Update: ❌

Thanks, @nfmohit. I forgot to include error states in my testing. Good spot. I have moved this back to Execution.

[zutigrm]

I have reverted the height which was there to align the content to the bottom of the tile. Since this was causing gaps when error tile is present. And error tile height, and content alignment in tiles is handled in other issues. For background, refer to this thread

[techanvil]

Back to you for another look, @wpdarren.

[wpdarren]

QA Update: ✅

Verified:

• I retested this to ensure that the percentage badges continued to appear underneath as per Figam designs but I also tested with different styles of tiles, including an error tile and a non-numeric.