-
Notifications
You must be signed in to change notification settings - Fork 286
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
Use engagementRate and display it as a percentage. #6690
Use engagementRate and display it as a percentage. #6690
Conversation
Size Change: +7 B (0%) Total Size: 1.23 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @techanvil – just a few small points to address 👍
@@ -43,6 +43,7 @@ const ANALYTICS_4_METRIC_TYPES = { | |||
conversions: 'TYPE_INTEGER', | |||
screenPageViews: 'TYPE_INTEGER', | |||
engagedSessions: 'TYPE_INTEGER', | |||
engagementRate: 'TYPE_INTEGER', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See #6689 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I've fixed this on the #6689 branch and cherry-picked the commit here as well.
@@ -188,7 +188,7 @@ function ModulePopularPagesWidgetGA4( props ) { | |||
hideOnMobile: true, | |||
field: 'metricValues.2.value', | |||
Component: ( { fieldValue } ) => ( | |||
<span>{ numFmt( fieldValue, { style: 'decimal' } ) }</span> | |||
<span>{ numFmt( Number( fieldValue ) / 100, '%' ) }</span> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we maybe use parseFloat
instead?
Also, fieldValue
for engagementRate
will be a number between 0 and 1 (inclusive) so we shouldn't need to divide by 100.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't look like we need parseFloat()
here, just passing fieldValue
in to numFmt()
without dividing it, while specifying the %
format seems fine.
6a3ba27
to
b264e84
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
Summary
As discussed on Slack we need to change from using the
engagedSessions
to theengagementRate
metric, and display it as a percentage.Addresses issue:
ModulePopularPagesWidget
#6219Relevant technical choices
PR Author Checklist
Do not alter or remove anything below. The following sections will be managed by moderators only.
Code Reviewer Checklist
Merge Reviewer Checklist