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

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

Merged
merged 29 commits into from
Jul 19, 2023
Merged
Show file tree
Hide file tree
Changes from 17 commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
86d83ff
Implement TopCitiesWidget logic.
hussain-t Jul 10, 2023
e19d27d
Apply custom styles to the cities.
hussain-t Jul 10, 2023
7929340
Add city array to the mock data.
hussain-t Jul 10, 2023
d92df6c
Add TopCitiesWidget stories.
hussain-t Jul 10, 2023
4607b5b
Add TopCitiesWidget VRT images.
hussain-t Jul 10, 2023
7739774
Simplify JSX render.
hussain-t Jul 10, 2023
853fc9a
Apply plain text styles.
hussain-t Jul 11, 2023
189c5fe
Merge branch 'develop' into enhance/#6252-km-top-cities-driving-traffic.
hussain-t Jul 11, 2023
e76f952
Update VRT for CSS changes.
hussain-t Jul 11, 2023
80ab73a
Merge branch 'develop' into enhance/#6252-km-top-cities-driving-traffic.
hussain-t Jul 17, 2023
8d74279
Remove the condition that renders WidgetNull.
hussain-t Jul 17, 2023
eac651a
Use receiveIsUserInputCompleted instead of global.
hussain-t Jul 17, 2023
f3fea56
Apply CSS changes.
hussain-t Jul 17, 2023
402d100
Add basic unit test for TopCitiesWidget.
hussain-t Jul 17, 2023
59410ed
Add snapshot for TopCitiesWidget.
hussain-t Jul 17, 2023
edbf618
Update VRT images.
hussain-t Jul 17, 2023
666631f
Remove receiveIsUserInputCompleted action.
hussain-t Jul 17, 2023
7edccc5
Use useSelect for hasFinishedResolution.
hussain-t Jul 18, 2023
6298942
Create MetricTileTablePlainText component.
hussain-t Jul 18, 2023
9f8860c
Use MetricTileTablePlainText component.
hussain-t Jul 18, 2023
b403efb
Fix percentage precision.
hussain-t Jul 18, 2023
3ce47da
Destructure totals from topCitiesReport.
hussain-t Jul 18, 2023
4f6898c
Remove delay from the scenario.
hussain-t Jul 18, 2023
f49aafb
Update VRT images for changing the precision.
hussain-t Jul 18, 2023
1aefc4b
Update snapshot for changing the precision.
hussain-t Jul 18, 2023
672b200
Merge branch 'develop' into enhance/#6252-km-top-cities-driving-traffic.
hussain-t Jul 18, 2023
b2f6412
Change 'zeroState' prop to 'ZeroState' to follow elementType naming c…
hussain-t Jul 18, 2023
bb266e1
Update the story to pass an element type.
hussain-t Jul 18, 2023
5794203
Merge branch 'develop' into enhance/#6252-km-top-cities-driving-traffic.
hussain-t Jul 19, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -21,15 +21,97 @@
*/
import PropTypes from 'prop-types';

/**
* WordPress dependencies
*/
import { __ } from '@wordpress/i18n';

/**
* Internal dependencies
*/
import Data from 'googlesitekit-data';
import { CORE_USER } from '../../../../googlesitekit/datastore/user/constants';
import {
DATE_RANGE_OFFSET,
MODULES_ANALYTICS_4,
} from '../../datastore/constants';
import { ZeroDataMessage } from '../../../analytics/components/common';
import { numFmt } from '../../../../util';
import { MetricTileTable } from '../../../../components/KeyMetrics';
const { useSelect, useInViewSelect } = Data;

export default function TopCitiesWidget( { Widget } ) {
const dates = useSelect( ( select ) =>
select( CORE_USER ).getDateRangeDates( {
offsetDays: DATE_RANGE_OFFSET,
} )
);

const topcCitiesReportOptions = {
...dates,
dimensions: [ 'city' ],
metrics: [ { name: 'totalUsers' } ],
orderby: [
{
metric: {
metricName: 'totalUsers',
},
desc: true,
},
],
limit: 3,
};

const topCitiesReport = useInViewSelect( ( select ) =>
select( MODULES_ANALYTICS_4 ).getReport( topcCitiesReportOptions )
);

const loading = useInViewSelect(
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

( select ) =>
! select( MODULES_ANALYTICS_4 ).hasFinishedResolution(
'getReport',
[ topcCitiesReportOptions ]
)
);

const { rows = [] } = topCitiesReport || {};

const totalUsers =
topCitiesReport?.totals?.[ 0 ]?.metricValues?.[ 0 ]?.value;
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.


const columns = [
{
field: 'dimensionValues',
Component: ( { fieldValue } ) => {
const [ title ] = fieldValue;

return (
<p className="googlesitekit-km-widget-tile__table-plain-text">
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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 👍

{ title.value }
</p>
);
},
},
{
field: 'metricValues.0.value',
Component: ( { fieldValue } ) => (
<strong>{ numFmt( fieldValue / totalUsers, '%' ) }</strong>
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

),
},
];

return (
<Widget>
<div>TODO: UI for TopCitiesWidget</div>
</Widget>
<MetricTileTable
Widget={ Widget }
title={ __( 'Top cities driving traffic', 'google-site-kit' ) }
loading={ loading }
rows={ rows }
columns={ columns }
zeroState={ ZeroDataMessage }
Copy link
Collaborator

Choose a reason for hiding this comment

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

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 :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

/>
);
}

TopCitiesWidget.propTypes = {
Widget: PropTypes.elementType.isRequired,
WidgetNull: PropTypes.elementType.isRequired,
};
Original file line number Diff line number Diff line change
@@ -0,0 +1,145 @@
/**
* TopCitiesWidget component stories.
*
* Site Kit by Google, Copyright 2023 Google LLC
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* https://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

/**
* Internal dependencies
*/
import { CORE_USER } from '../../../../googlesitekit/datastore/user/constants';
import { MODULES_ANALYTICS_4 } from '../../datastore/constants';
import {
provideKeyMetrics,
provideModules,
} from '../../../../../../tests/js/utils';
import { withWidgetComponentProps } from '../../../../googlesitekit/widgets/util';
import { getAnalytics4MockResponse } from '../../utils/data-mock';
import { replaceValuesInAnalytics4ReportWithZeroData } from '../../../../../../.storybook/utils/zeroReports';
import WithRegistrySetup from '../../../../../../tests/js/WithRegistrySetup';
import TopCitiesWidget from './TopCitiesWidget';

const reportOptions = {
startDate: '2020-08-11',
endDate: '2020-09-07',
dimensions: [ 'city' ],
metrics: [ { name: 'totalUsers' } ],
orderby: [
{
metric: {
metricName: 'totalUsers',
},
desc: true,
},
],
limit: 3,
};

const WidgetWithComponentProps =
withWidgetComponentProps( 'test' )( TopCitiesWidget );

const Template = ( { setupRegistry, ...args } ) => (
<WithRegistrySetup func={ setupRegistry }>
<WidgetWithComponentProps { ...args } />
</WithRegistrySetup>
);

export const Ready = Template.bind( {} );
Ready.storyName = 'Ready';
Ready.args = {
setupRegistry: ( registry ) => {
const report = getAnalytics4MockResponse( reportOptions );
// Calculate sum of metricValues for all rows
const rowsSum = report.rows.reduce( ( total, row ) => {
return total + Number( row.metricValues[ 0 ].value );
}, 0 );

// Generate totalValueForAllCities that is higher than the sum
const totalValueForAllCities = rowsSum * 2;

// Adjust totals field in the mock response
report.totals = [
{
dimensionValues: [ { value: 'RESERVED_TOTAL' } ],
metricValues: [ { value: totalValueForAllCities.toString() } ],
},
];
registry.dispatch( MODULES_ANALYTICS_4 ).receiveGetReport( report, {
options: reportOptions,
} );
},
};
Ready.scenario = {
label: 'KeyMetrics/TopCitiesWidget/Ready',
delay: 250,
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

};

export const Loading = Template.bind( {} );
Loading.storyName = 'Loading';
Loading.args = {
setupRegistry: ( { dispatch } ) => {
dispatch( MODULES_ANALYTICS_4 ).startResolution( 'getReport', [
reportOptions,
] );
},
};
Comment on lines +88 to +96
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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


export const ZeroData = Template.bind( {} );
ZeroData.storyName = 'Zero Data';
ZeroData.args = {
setupRegistry: ( { dispatch } ) => {
const report = getAnalytics4MockResponse( reportOptions );
const zeroReport =
replaceValuesInAnalytics4ReportWithZeroData( report );

dispatch( MODULES_ANALYTICS_4 ).receiveGetReport( zeroReport, {
options: reportOptions,
} );
},
};
ZeroData.scenario = {
label: 'KeyMetrics/TopCitiesWidget/ZeroData',
delay: 250,
};

export default {
title: 'Key Metrics/TopCitiesWidget',
decorators: [
( Story, { args } ) => {
const setupRegistry = ( registry ) => {
provideModules( registry, [
{
slug: 'analytics-4',
active: true,
connected: true,
},
] );

registry.dispatch( CORE_USER ).setReferenceDate( '2020-09-08' );

provideKeyMetrics( registry );

// Call story-specific setup.
args.setupRegistry( registry );
};

return (
<WithRegistrySetup func={ setupRegistry }>
<Story />
</WithRegistrySetup>
);
},
],
};
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
/**
* TopCitiesWidget component tests.
*
* Site Kit by Google, Copyright 2023 Google LLC
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* https://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

/**
* Internal dependencies
*/
import { render } from '../../../../../../tests/js/test-utils';
import { provideKeyMetrics } from '../../../../../../tests/js/utils';
import { provideAnalytics4MockReport } from '../../utils/data-mock';
import { getWidgetComponentProps } from '../../../../googlesitekit/widgets/util';
import {
CORE_USER,
KM_ANALYTICS_TOP_CITIES,
} from '../../../../googlesitekit/datastore/user/constants';
import TopCitiesWidget from './TopCitiesWidget';

describe( 'TopCitiesWidget', () => {
const { Widget } = getWidgetComponentProps( KM_ANALYTICS_TOP_CITIES );

it( 'renders correctly with the expected metrics', async () => {
const { container, waitForRegistry } = render(
<TopCitiesWidget Widget={ Widget } />,
{
setupRegistry: ( registry ) => {
registry
.dispatch( CORE_USER )
.setReferenceDate( '2020-09-08' );

provideKeyMetrics( registry );
provideAnalytics4MockReport( registry, {
startDate: '2020-08-11',
endDate: '2020-09-07',
dimensions: [ 'city' ],
metrics: [ { name: 'totalUsers' } ],
orderby: [
{
metric: {
metricName: 'totalUsers',
},
desc: true,
},
],
limit: 3,
} );
},
}
);
await waitForRegistry();

expect( container ).toMatchSnapshot();
} );
} );
Loading