Enhance/#6252 - Create the "Top cities driving traffic" key metric widget tile #7279

hussain-t · 2023-07-10T12:50:59Z

Summary

Addresses issue:

Create the "Top cities driving traffic" key metric widget tile #6252

Relevant technical choices

In this PR, we've made an update to improve our code's adherence to established React naming conventions.

We previously used the prop name zeroState for the MetricTileTable component, which expected a function returning a React element (PropTypes.func). According to React conventions, we use PascalCase for component names and camelCase for nodes. Hence, zeroState has been renamed to ZeroState to better reflect its nature as a function that returns a component.

This refactor affects the MetricTileTable component and any other components or places in the code where this prop was used. The functionality remains unchanged, but the prop's name reflects its nature better.

PR Author Checklist

My code is tested and passes existing unit tests.
My code has an appropriate set of unit tests which all pass.
My code is backward-compatible with WordPress 5.2 and PHP 5.6.
My code follows the WordPress coding standards.
My code has proper inline documentation.
I have added a QA Brief on the issue linked above.
I have signed the Contributor License Agreement (see https://cla.developers.google.com/).

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

Code Reviewer Checklist

Run the code.
Ensure the acceptance criteria are satisfied.
Reassess the implementation with the IB.
Ensure no unrelated changes are included.
Ensure CI checks pass.
Check Storybook where applicable.
Ensure there is a QA Brief.

Merge Reviewer Checklist

Ensure the PR has the correct target branch.
Double-check that the PR is okay to be merged.
Ensure the corresponding issue has a ZenHub release assigned.
Add a changelog message to the issue.

github-actions · 2023-07-10T12:59:19Z

Build files for 5794203 have been deleted.

github-actions · 2023-07-10T13:03:04Z

Size Change: +215 B (0%)

Total Size: 1.4 MB

Filename	Size	Change
`./dist/assets/css/googlesitekit-admin-css-********************.min.css`	50.4 kB	+18 B (0%)
`./dist/assets/js/googlesitekit-activation-********************.js`	24 kB	+1 B (0%)
`./dist/assets/js/googlesitekit-ad-blocking-recovery-********************.js`	52.3 kB	+9 B (0%)
`./dist/assets/js/googlesitekit-adminbar-********************.js`	33.1 kB	+4 B (0%)
`./dist/assets/js/googlesitekit-components-gm2-********************.js`	5.41 kB	-3 B (0%)
`./dist/assets/js/googlesitekit-components-gm3-********************.js`	9.98 kB	-65 B (-1%)
`./dist/assets/js/googlesitekit-data-********************.js`	2.16 kB	-1 B (0%)
`./dist/assets/js/googlesitekit-datastore-site-********************.js`	16.5 kB	+2 B (0%)
`./dist/assets/js/googlesitekit-datastore-user-********************.js`	23.3 kB	-3 B (0%)
`./dist/assets/js/googlesitekit-entity-dashboard-********************.js`	66.5 kB	+3 B (0%)
`./dist/assets/js/googlesitekit-main-dashboard-********************.js`	80.1 kB	+7 B (0%)
`./dist/assets/js/googlesitekit-modules-********************.js`	21.6 kB	-4 B (0%)
`./dist/assets/js/googlesitekit-modules-analytics-********************.js`	88.2 kB	+84 B (0%)
`./dist/assets/js/googlesitekit-modules-analytics-4-********************.js`	62.8 kB	+281 B (0%)
`./dist/assets/js/googlesitekit-modules-optimize-********************.js`	19.6 kB	+2 B (0%)
`./dist/assets/js/googlesitekit-modules-pagespeed-insights-********************.js`	21.3 kB	-5 B (0%)
`./dist/assets/js/googlesitekit-modules-search-console-********************.js`	57 kB	-42 B (0%)
`./dist/assets/js/googlesitekit-modules-tagmanager-********************.js`	31.4 kB	+9 B (0%)
`./dist/assets/js/googlesitekit-settings-********************.js`	49.2 kB	-3 B (0%)
`./dist/assets/js/googlesitekit-splash-********************.js`	67.2 kB	-96 B (0%)
`./dist/assets/js/googlesitekit-user-input-********************.js`	42.1 kB	-1 B (0%)
`./dist/assets/js/googlesitekit-vendor-********************.js`	312 kB	-1 B (0%)
`./dist/assets/js/googlesitekit-widgets-********************.js`	24 kB	+27 B (0%)
`./dist/assets/js/googlesitekit-wp-dashboard-********************.js`	62.8 kB	-8 B (0%)

ℹ️ View Unchanged

Filename	Size
`./dist/assets/css/googlesitekit-adminbar-css-********************.min.css`	11.2 kB
`./dist/assets/css/googlesitekit-wp-dashboard-css-********************.min.css`	7.55 kB
`./dist/assets/js/30-********************.js`	2.8 kB
`./dist/assets/js/31-********************.js`	2.28 kB
`./dist/assets/js/32-********************.js`	3.72 kB
`./dist/assets/js/33-********************.js`	929 B
`./dist/assets/js/34-********************.js`	888 B
`./dist/assets/js/35-********************.js`	3.12 kB
`./dist/assets/js/analytics-advanced-tracking-********************.js`	769 B
`./dist/assets/js/googlesitekit-api-********************.js`	9.88 kB
`./dist/assets/js/googlesitekit-datastore-forms-********************.js`	9.3 kB
`./dist/assets/js/googlesitekit-datastore-location-********************.js`	2.09 kB
`./dist/assets/js/googlesitekit-datastore-ui-********************.js`	9.41 kB
`./dist/assets/js/googlesitekit-i18n-********************.js`	3.92 kB
`./dist/assets/js/googlesitekit-modules-adsense-********************.js`	105 kB
`./dist/assets/js/googlesitekit-polyfills-********************.js`	379 B
`./dist/assets/js/runtime-********************.js`	1.3 kB

_{compressed-size-action}

nfmohit

Thank you for your amazing work here, @hussain-t!

I have left a few minor comments for you to take a look, mostly reflecting on changes required after #7061 was merged.

Apart from the changes highlighted, I'd request a couple more changes:

Could we add a test case or two for this widget, similar to TopConvertingTrafficSourceWidget.test.js?
Could we update the QAB (Note 4) and the PR description, since now we've confirmed that we're using plain text instead of navigational links?

Thank you!

assets/js/modules/analytics-4/components/widgets/TopCitiesWidget.js

nfmohit · 2023-07-17T07:14:14Z

assets/js/modules/analytics-4/components/widgets/TopCitiesWidget.stories.js

+export const Loading = Template.bind( {} );
+Loading.storyName = 'Loading';
+Loading.args = {
+	setupRegistry: ( { dispatch } ) => {
+		dispatch( MODULES_ANALYTICS_4 ).startResolution( 'getReport', [
+			reportOptions,
+		] );
+	},
+};


Is there a reason we don't have VRT scenarios for the loading state?

We don't have VRT scenarios for the loading state for any other KM widget stories. I didn't create it to follow other widgets. Apart from that, IMO, we don't need a VRT scenario for the loading state. cc @jimmymadon

assets/js/modules/analytics-4/components/widgets/TopCitiesWidget.stories.js

assets/sass/components/key-metrics/_googlesitekit-km-widget-tile.scss

Fix typo. Export default function.

nfmohit

Thank you for the kind updates here, @hussain-t!

I've left another small comment for your attention. Apart from them, could we update the QAB a bit further so that the QA team can view the widget in the dashboard since all widgets no longer appear unconditionally?

We should be good to go as soon as the above are addressed, thank you!

assets/js/modules/analytics-4/components/widgets/TopCitiesWidget.stories.js

nfmohit

LGTM, brilliant work, @hussain-t!

aaemnnosttv

Thanks @hussain-t – this one is almost there, just a few points to address 👍

aaemnnosttv · 2023-07-17T19:22:09Z

assets/js/modules/analytics-4/components/widgets/TopCitiesWidget.js

+		select( MODULES_ANALYTICS_4 ).getReport( topcCitiesReportOptions )
+	);
+
+	const loading = useInViewSelect(


The in-view select variant is only intended for selectors that get reports since it will return undefined unconditionally when not in view which would not be expected for a selector like hasFinishedResolution which would never return anything other than a boolean. It could be used for other things too but it's essentially intended for guarding selectors that have resolvers and their respective side-effects.

Thanks, I have updated it to use useSelect. However, I have checked other KM widgets, and they use the useInViewSelect selector, which needs to be addressed.

aaemnnosttv · 2023-07-17T19:23:35Z

assets/js/modules/analytics-4/components/widgets/TopCitiesWidget.js

+				const [ title ] = fieldValue;
+
+				return (
+					<p className="googlesitekit-km-widget-tile__table-plain-text">


Lets extract this to a component since it will be used by others as well rather than relying on the same class name in each instance manually.

I didn't extract it into a separate component since it will be used only in Cities and Countries. However, it's a good idea to extract it 👍

aaemnnosttv · 2023-07-17T19:31:03Z

assets/js/modules/analytics-4/components/widgets/TopCitiesWidget.js

+		{
+			field: 'metricValues.0.value',
+			Component: ( { fieldValue } ) => (
+				<strong>{ numFmt( fieldValue / totalUsers, '%' ) }</strong>


This is consistent with other uses we have (except the ChangeBadge) but the design only shows a precision of 1 decimal place whereas numFmt defaults to 2. Let's update this for consistency with the design.

aaemnnosttv · 2023-07-17T19:34:58Z

assets/js/modules/analytics-4/components/widgets/TopCitiesWidget.js

+			loading={ loading }
+			rows={ rows }
+			columns={ columns }
+			zeroState={ ZeroDataMessage }


Not unique to this issue since I see it is following others, but zeroState should be ZeroState and expect an elementType as is our convention. i.e. PascalCase = element type; camelCase = node.

We can fix this in a separate issue if you'd prefer, I'm just calling what I see :)

Good catch. I have incorporated the changes in this PR to save a few hours 😃

aaemnnosttv · 2023-07-17T19:35:57Z

assets/js/modules/analytics-4/components/widgets/TopCitiesWidget.stories.js

+};
+Ready.scenario = {
+	label: 'KeyMetrics/TopCitiesWidget/Ready',
+	delay: 250,


Let's not add delay unless it's really needed for some reason, in which case let's annotate why.

aaemnnosttv · 2023-07-17T19:40:48Z

assets/js/modules/analytics-4/components/widgets/TopCitiesWidget.js

+	const { rows = [] } = topCitiesReport || {};
+
+	const totalUsers =
+		topCitiesReport?.totals?.[ 0 ]?.metricValues?.[ 0 ]?.value;


We're already destructuring from topCitiesReport above with a fallback, how about we do the same with totals above as well? We'd still need some chaining here but it could be simplified a bit.

…onventions.

aaemnnosttv

LGTM, thanks @hussain-t !

hussain-t added 4 commits July 10, 2023 18:15

Implement TopCitiesWidget logic.

86d83ff

Apply custom styles to the cities.

e19d27d

Add city array to the mock data.

7929340

Add TopCitiesWidget stories.

d92df6c

hussain-t added 5 commits July 10, 2023 18:41

Add TopCitiesWidget VRT images.

4607b5b

Simplify JSX render.

7739774

Apply plain text styles.

853fc9a

Merge branch 'develop' into enhance/#6252-km-top-cities-driving-traffic.

189c5fe

Update VRT for CSS changes.

e76f952

nfmohit requested changes Jul 17, 2023

View reviewed changes

hussain-t added 7 commits July 17, 2023 14:42

Merge branch 'develop' into enhance/#6252-km-top-cities-driving-traffic.

80ab73a

Remove the condition that renders WidgetNull.

8d74279

Fix typo. Export default function.

Use receiveIsUserInputCompleted instead of global.

eac651a

Apply CSS changes.

f3fea56

Add basic unit test for TopCitiesWidget.

402d100

Add snapshot for TopCitiesWidget.

59410ed

Update VRT images.

edbf618

nfmohit requested changes Jul 17, 2023

View reviewed changes

assets/js/modules/analytics-4/components/widgets/TopCitiesWidget.stories.js Outdated Show resolved Hide resolved

Remove receiveIsUserInputCompleted action.

666631f

nfmohit approved these changes Jul 17, 2023

View reviewed changes

aaemnnosttv requested changes Jul 17, 2023

View reviewed changes

hussain-t added 7 commits July 18, 2023 14:23

Use useSelect for hasFinishedResolution.

7edccc5

Create MetricTileTablePlainText component.

6298942

Use MetricTileTablePlainText component.

9f8860c

Fix percentage precision.

b403efb

Destructure totals from topCitiesReport.

3ce47da

Remove delay from the scenario.

4f6898c

Update VRT images for changing the precision.

f49aafb

hussain-t added 5 commits July 18, 2023 15:38

Update snapshot for changing the precision.

1aefc4b

Merge branch 'develop' into enhance/#6252-km-top-cities-driving-traffic.

672b200

Change 'zeroState' prop to 'ZeroState' to follow elementType naming c…

b2f6412

…onventions.

Update the story to pass an element type.

bb266e1

Merge branch 'develop' into enhance/#6252-km-top-cities-driving-traffic.

5794203

aaemnnosttv approved these changes Jul 19, 2023

View reviewed changes

aaemnnosttv merged commit 94ff3d9 into develop Jul 19, 2023

aaemnnosttv deleted the enhance/#6252-km-top-cities-driving-traffic branch July 19, 2023 15:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enhance/#6252 - Create the "Top cities driving traffic" key metric widget tile #7279

Enhance/#6252 - Create the "Top cities driving traffic" key metric widget tile #7279

hussain-t commented Jul 10, 2023 •

edited

Loading

github-actions bot commented Jul 10, 2023 •

edited

Loading

github-actions bot commented Jul 10, 2023 •

edited

Loading

nfmohit left a comment

nfmohit Jul 17, 2023

hussain-t Jul 17, 2023

nfmohit left a comment

nfmohit left a comment

aaemnnosttv left a comment

aaemnnosttv Jul 17, 2023

hussain-t Jul 18, 2023

aaemnnosttv Jul 17, 2023

hussain-t Jul 18, 2023

aaemnnosttv Jul 17, 2023

aaemnnosttv Jul 17, 2023

hussain-t Jul 18, 2023

aaemnnosttv Jul 17, 2023

aaemnnosttv Jul 17, 2023

aaemnnosttv left a comment

Enhance/#6252 - Create the "Top cities driving traffic" key metric widget tile #7279

Enhance/#6252 - Create the "Top cities driving traffic" key metric widget tile #7279

Conversation

hussain-t commented Jul 10, 2023 • edited Loading

Summary

Relevant technical choices

PR Author Checklist

Code Reviewer Checklist

Merge Reviewer Checklist

github-actions bot commented Jul 10, 2023 • edited Loading

github-actions bot commented Jul 10, 2023 • edited Loading

nfmohit left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nfmohit left a comment

Choose a reason for hiding this comment

nfmohit left a comment

Choose a reason for hiding this comment

aaemnnosttv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaemnnosttv left a comment

Choose a reason for hiding this comment

hussain-t commented Jul 10, 2023 •

edited

Loading

github-actions bot commented Jul 10, 2023 •

edited

Loading

github-actions bot commented Jul 10, 2023 •

edited

Loading