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

Enhancement/8981 Refactor RRM Setup Success Notification #9367

Open
wants to merge 39 commits into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
39 commits
Select commit Hold shift + click to select a range
3cd8bb8
Register RRM setup success subtle notification.
jimmymadon Sep 15, 2024
fd98e95
Merge branch 'develop' into enhancement/8981-refactor-RRM-setup-succe…
jimmymadon Sep 16, 2024
dedf402
Register notifications for RRM.
jimmymadon Sep 16, 2024
b79d4d7
Add check requirements to RRM Setup Success Notification.
jimmymadon Sep 16, 2024
bb6c8ad
Remove rendering logic from RRM Success Notification.
jimmymadon Sep 16, 2024
69410c9
Remove direct rendering of RRM Subtle notification.
jimmymadon Sep 16, 2024
d009d06
Remove redundant non-standard check.
jimmymadon Sep 16, 2024
4f12f9f
Add props to the RRM Setup Success Notification.
jimmymadon Sep 18, 2024
e90e228
Create a simple CTALink component for Subtle Notifications.
jimmymadon Sep 18, 2024
5a2af0f
Add a warning variant to the Subtle Notification layout.
jimmymadon Sep 18, 2024
81fca07
Refactor RRM success notification to use new Subtle Notification layo…
jimmymadon Sep 18, 2024
427d080
Merge branch 'develop' into enhancement/8981-refactor-RRM-setup-succe…
jimmymadon Sep 18, 2024
d226224
Allow passing GA event tracking label for dismiss event.
jimmymadon Sep 18, 2024
7349a9f
Pass GA event tracking label for dismiss event.
jimmymadon Sep 18, 2024
cfa1191
Allow passing GA event tracking label for confirm CTA click event.
jimmymadon Sep 18, 2024
8b11d79
Pass GA event tracking label for confirm CTA click event.
jimmymadon Sep 18, 2024
0ee7585
Allow passing GA event tracking label for view notification event.
jimmymadon Sep 18, 2024
8d1bc10
Pass GA event tracking label for view notification event.
jimmymadon Sep 18, 2024
5428eb1
Add missing PropTypes for the Notification component.
jimmymadon Sep 18, 2024
edfadd9
Add missing PropTypes for common Dismiss component.
jimmymadon Sep 18, 2024
71bc135
Merge branch 'develop' into enhancement/8981-refactor-RRM-setup-succe…
jimmymadon Sep 18, 2024
3ad16ee
Merge branch 'develop' into enhancement/8981-refactor-RRM-setup-succe…
jimmymadon Sep 21, 2024
efedf0d
Add support to pass custom CTA click callback.
jimmymadon Sep 21, 2024
bbd619b
Fix merge conflict resolution error.
jimmymadon Sep 21, 2024
a19e6fd
Fix storybook stories.
jimmymadon Sep 21, 2024
523770f
Render RRM Setup Success notification in tests with new props.
jimmymadon Sep 21, 2024
5fe4d30
Add notification component ID prop to the utility method instead of p…
jimmymadon Sep 23, 2024
d0c48cf
Update the dismiss button for onboarding complete version of the RRM …
jimmymadon Sep 23, 2024
478701c
Remove direct call to the audience segmentation setup success notific…
jimmymadon Sep 23, 2024
31d0654
Revert "Remove direct call to the audience segmentation setup success…
jimmymadon Sep 23, 2024
cff9f4c
Fix failing tests.
nfmohit Sep 24, 2024
a5282c3
Add a selector to get a notifcation from the state by its ID.
jimmymadon Sep 25, 2024
541ad91
Dismiss a notification in the DB only if it is dismissible.
jimmymadon Sep 25, 2024
f54affb
Add tests for skipping persistent dismissal if notification is not di…
jimmymadon Sep 25, 2024
339fd10
Flip dismissal condition to record a dismissal if notification is und…
jimmymadon Sep 25, 2024
5b4fe9a
Merge branch 'develop' into enhancement/8981-refactor-RRM-setup-succe…
nfmohit Sep 26, 2024
b835233
Fix dismissible issue.
nfmohit Sep 26, 2024
9c0e918
Improve test coverage.
nfmohit Sep 28, 2024
e6a961b
Merge branch 'develop' into enhancement/8981-refactor-RRM-setup-succe…
nfmohit Sep 29, 2024
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
2 changes: 1 addition & 1 deletion assets/js/components/notifications/Notifications.js
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ export default function Notifications( { areaSlug } ) {
}

const { id, Component: ActiveNotification } = queuedNotifications[ 0 ];
const props = { id, ...getNotificationComponentProps( id ) };
const props = { ...getNotificationComponentProps( id ) };

return <ActiveNotification { ...props } />;
}
Expand Down
3 changes: 0 additions & 3 deletions assets/js/components/notifications/SubtleNotifications.js
Original file line number Diff line number Diff line change
Expand Up @@ -26,15 +26,13 @@ import { Fragment } from '@wordpress/element';
*/
import { useFeature } from '../../hooks/useFeature';
import AudienceSegmentationSetupSuccessSubtleNotification from '../../modules/analytics-4/components/audience-segmentation/dashboard/AudienceSegmentationSetupSuccessSubtleNotification';
import { RRMSetupSuccessSubtleNotification } from '../../modules/reader-revenue-manager/components/dashboard';
import useViewContext from '../../hooks/useViewContext';
import Notifications from './Notifications';
import { NOTIFICATION_AREAS } from '../../googlesitekit/notifications/datastore/constants';

export default function SubtleNotifications() {
const viewContext = useViewContext();
const audienceSegmentationEnabled = useFeature( 'audienceSegmentation' );
const rrmModuleEnabled = useFeature( 'rrmModule' );

// Each notification component rendered here has its own logic to determine
// whether it should be displayed; in most cases none of these components
Expand All @@ -48,7 +46,6 @@ export default function SubtleNotifications() {
{ audienceSegmentationEnabled && (
<AudienceSegmentationSetupSuccessSubtleNotification />
) }
{ rrmModuleEnabled && <RRMSetupSuccessSubtleNotification /> }
<Notifications
viewContext={ viewContext }
areaSlug={ NOTIFICATION_AREAS.BANNERS_BELOW_NAV }
Expand Down
3 changes: 3 additions & 0 deletions assets/js/googlesitekit-modules-reader-revenue-manager.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,10 +21,13 @@
*/
import Data from 'googlesitekit-data';
import Modules from 'googlesitekit-modules';
import Notifications from 'googlesitekit-notifications';
import {
registerStore,
registerModule,
registerNotifications,
} from './modules/reader-revenue-manager';

registerStore( Data );
registerModule( Modules );
registerNotifications( Notifications );
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,11 @@
* limitations under the License.
*/

/**
* External dependencies
*/
import PropTypes from 'prop-types';

/**
* WordPress dependencies
*/
Expand All @@ -26,7 +31,12 @@ import ViewedStateObserver from './ViewedStateObserver';
import { useHasBeenViewed } from '../../hooks/useHasBeenViewed';
import useNotificationEvents from '../../hooks/useNotificationEvents';

export default function Notification( { id, className, children } ) {
export default function Notification( {
id,
className,
gaViewEventLabel,
Copy link
Collaborator

Choose a reason for hiding this comment

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

One question regarding this gaViewEventLabel prop: Looking at the trackEvent function, it also supports an additional value parameter:

return async function trackEvent( category, action, label, value ) {

Instead of accepting a gaViewEventLabel prop here, what if we received an array prop, say gaEventArgs, and spread that in the function, e.g. trackEvents.view( ...gaEventArgs )?

This will ensure that this mechanism is future proof and we do not need to make any changes in case an event also requires the value parameter.

What do you think?

children,
} ) {
const ref = useRef();
const viewed = useHasBeenViewed( id );
const trackEvents = useNotificationEvents( id );
Expand All @@ -36,10 +46,10 @@ export default function Notification( { id, className, children } ) {
// Track view once.
useEffect( () => {
if ( ! isViewedOnce && viewed ) {
trackEvents.view();
trackEvents.view( gaViewEventLabel );
setIsViewedOnce( true );
}
}, [ viewed, trackEvents, isViewedOnce ] );
}, [ viewed, trackEvents, isViewedOnce, gaViewEventLabel ] );

return (
<section id={ id } ref={ ref } className={ className }>
Expand All @@ -56,3 +66,10 @@ export default function Notification( { id, className, children } ) {
</section>
);
}

Notification.propTypes = {
id: PropTypes.string,
className: PropTypes.string,
gaViewEventLabel: PropTypes.string,
children: PropTypes.node,
};
Original file line number Diff line number Diff line change
@@ -0,0 +1,83 @@
/**
* Site Kit by Google, Copyright 2024 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.
*/

/**
* External dependencies
*/
import PropTypes from 'prop-types';

/**
* Internal dependencies
*/
import { useDispatch } from 'googlesitekit-data';
import { CORE_LOCATION } from '../../../datastore/location/constants';
import useNotificationEvents from '../../hooks/useNotificationEvents';
import { Button } from 'googlesitekit-components';
import ExternalSVG from '../../../../../svg/icons/external.svg';

export default function CTALinkSubtle( {
id,
ctaLink,
ctaLabel,
onCTAClick,
isCTALinkExternal = false,
gaConfirmEventLabel,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same thoughts as gaViewEventLabel above.

} ) {
const trackEvents = useNotificationEvents( id );
const { navigateTo } = useDispatch( CORE_LOCATION );

const handleCTAClick = async ( event ) => {
if ( ! event.defaultPrevented ) {
event.preventDefault();
}

await onCTAClick?.( event );

await trackEvents.confirm( gaConfirmEventLabel );

if ( isCTALinkExternal ) {
global.open( ctaLink, '_blank' );
} else {
navigateTo( ctaLink );
}
};
Comment on lines +42 to +56
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd vote to keep this natural, as handleDismiss in assets/js/googlesitekit/notifications/components/common/Dismiss.js. Using global.open in RRMSetupSuccessSubtleNotification was more of a hack to make the core/forms setValues action to work with useRefocus.

Suggested change
const handleCTAClick = async ( event ) => {
if ( ! event.defaultPrevented ) {
event.preventDefault();
}
await onCTAClick?.( event );
await trackEvents.confirm( gaConfirmEventLabel );
if ( isCTALinkExternal ) {
global.open( ctaLink, '_blank' );
} else {
navigateTo( ctaLink );
}
};
const handleCTAClick = async ( event ) => {
await onCTAClick?.( event );
await trackEvents.confirm( gaConfirmEventLabel );
};


return (
<Button
className="googlesitekit-subtle-notification__cta"
href={ ctaLink }
onClick={ handleCTAClick }
target={ isCTALinkExternal ? '_blank' : '_self' }
trailingIcon={
isCTALinkExternal ? (
<ExternalSVG width={ 14 } height={ 14 } />
) : undefined
}
>
{ ctaLabel }
</Button>
);
}

// eslint-disable-next-line sitekit/acronym-case
CTALinkSubtle.propTypes = {
id: PropTypes.string,
ctaLink: PropTypes.string,
ctaLabel: PropTypes.string,
onCTAClick: PropTypes.func,
isCTALinkExternal: PropTypes.bool,
gaConfirmEventLabel: PropTypes.string,
};
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,11 @@
* limitations under the License.
*/

/**
* External dependencies
*/
import PropTypes from 'prop-types';

/**
* WordPress dependencies
*/
Expand All @@ -34,14 +39,15 @@ export default function Dismiss( {
dismissExpires = 0,
disabled,
onDismiss = () => {},
gaDismissEventLabel,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same thoughts as gaViewEventLabel above.

} ) {
const trackEvents = useNotificationEvents( id );

const { dismissNotification } = useDispatch( CORE_NOTIFICATIONS );

const handleDismiss = async ( event ) => {
await onDismiss?.( event );
trackEvents.dismiss();
trackEvents.dismiss( gaDismissEventLabel );
dismissNotification( id, { expiresInSeconds: dismissExpires } );
};

Expand All @@ -55,3 +61,13 @@ export default function Dismiss( {
</Button>
);
}

Dismiss.propTypes = {
id: PropTypes.string,
primary: PropTypes.bool,
dismissLabel: PropTypes.string,
dismissExpires: PropTypes.number,
disabled: PropTypes.bool,
onDismiss: PropTypes.func,
gaDismissEventLabel: PropTypes.string,
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

This component has a few styling discrepancies when compared to its predecessor assets/js/components/notifications/SubtleNotification.js:

Item 1

The type of warning does not have the appropriate colours.

Original SubtleNotification of variant warning:

image

This SubtleNotification with type warning:

image

Item 2

In smaller viewports, the positioning of buttons does not match the designs.

Original SubtleNotification:

image

This SubtleNotification:

image


Could we update these to match? Thanks!

Original file line number Diff line number Diff line change
Expand Up @@ -20,37 +20,56 @@
* External dependencies
*/
import PropTypes from 'prop-types';
import classnames from 'classnames';

/**
* Internal dependencies
*/
import CheckFill from '../../../../../svg/icons/check-fill.svg';
import WarningSVG from '../../../../../svg/icons/warning.svg';
import { Grid, Cell, Row } from '../../../../material-components';

export default function SubtleNotification( {
title,
description,
dismissCTA,
additionalCTA,
type = 'success',
} ) {
return (
<Grid>
<Row>
<Cell
alignMiddle
size={ 12 }
className="googlesitekit-subtle-notification"
className={ classnames(
'googlesitekit-subtle-notification',
{
'googlesitekit-subtle-notification--success':
type === 'success',
'googlesitekit-subtle-notification--warning':
type === 'warning',
}
) }
>
<div className="googlesitekit-subtle-notification__icon">
<CheckFill width={ 24 } height={ 24 } />
{ type === 'success' && (
<CheckFill width={ 24 } height={ 24 } />
) }
{ type === 'warning' && (
<WarningSVG width={ 24 } height={ 24 } />
) }
</div>

<div className="googlesitekit-subtle-notification__content">
<p>{ title }</p>
<p className="googlesitekit-subtle-notification__secondary_description">
{ description }
</p>
</div>

{ dismissCTA }

{ additionalCTA }
</Cell>
</Row>
Expand All @@ -63,4 +82,5 @@ SubtleNotification.propTypes = {
description: PropTypes.node,
dismissCTA: PropTypes.node,
additionalCTA: PropTypes.node,
type: PropTypes.string,
};
21 changes: 21 additions & 0 deletions assets/js/googlesitekit/notifications/datastore/notifications.js
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,15 @@ export const actions = {
};
}

const notification = registry
.select( CORE_NOTIFICATIONS )
.getNotification( id );

// Skip persisting notification dismissal in database if the notification is not dismissible.
if ( notification?.isDismissible === false ) {
return;
}

return registry
.dispatch( CORE_USER )
.dismissItem( id, { expiresInSeconds } );
Expand Down Expand Up @@ -281,6 +290,18 @@ export const selectors = {
getNotifications: ( state ) => {
return state.notifications;
},
/**
* Fetches a registered notification by ID from state.
*
* @since n.e.x.t
*
* @param {Object} state Data store's state.
* @param {string} id Notification ID.
* @return {(Object|undefined)} The registered notification object or undefined if a notification with the given ID is not registered.
*/
getNotification: ( state, id ) => {
return state.notifications[ id ];
},
/**
* Fetches the queue of registered notifications which are filtered and sorted.
*
Expand Down
Loading
Loading