Skip to content

Conversation

@macobo
Copy link
Contributor

@macobo macobo commented Feb 10, 2025

Changes

Counting total conversion events doesn't really make sense in how we store engagement events. After this change this metric is either hidden or displayed as - when dealing with scroll goals.

Tests

  • Automated tests have been added
  • This PR does not require tests

Screenshots

Conversions table with scroll goals:
image

Conversions hidden in top stats when filtering by scroll goal:
image

No column for total conversions shown when it would be all -
image

Conversions is not hidden when filtering by other goals:
image

Counting conversions doesn't really make sense in this scenario.

If there are multiple goals, nulls are only returned for those affected.

If there is at least one scroll goal included by the filter, conversions
cannot be counted.
@macobo macobo added the preview label Feb 10, 2025
@github-actions
Copy link

Preview environment👷🏼‍♀️🏗️
PR-5059

@macobo macobo marked this pull request as ready for review February 10, 2025 13:16
@macobo macobo requested a review from RobertJoonas February 10, 2025 13:16

const stats = data && data.top_stats.map(renderStat)
const stats =
data && data.top_stats.filter((stat) => stat.value !== null).map(renderStat)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will also hide the scroll_depth metric in a page-filtered view, in case there's no engagement data. But I guess showing a - is irrelevant anyway, so I'm still approving this change :)

@macobo macobo added this pull request to the merge queue Feb 11, 2025
Merged via the queue into master with commit a7ff6fa Feb 11, 2025
8 checks passed
@RobertJoonas RobertJoonas deleted the hidden-total-conversions branch February 11, 2025 09:04
@macobo macobo mentioned this pull request Feb 11, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants