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/9889 Refactor RRM Setup CTA Banner #9938

Merged
merged 64 commits into from
Jan 10, 2025
Merged
Show file tree
Hide file tree
Changes from 24 commits
Commits
Show all changes
64 commits
Select commit Hold shift + click to select a range
f5388ef
Remove rendering of WidgetNull.
jimmymadon Dec 23, 2024
5fa92aa
Wrap RRM notification registration with a feature flag check.
jimmymadon Dec 23, 2024
f3f92c2
Register RRM Setup CTA notification to the setup CTAs queue.
jimmymadon Dec 23, 2024
64e66e5
Remove direct rendering of RRM Setup CTA banner.
jimmymadon Dec 23, 2024
7118da0
Move canActivateModule check to notification registration.
jimmymadon Dec 23, 2024
3c2169d
Refactor the RRM Setup CTA Component to use the new notification comp…
jimmymadon Dec 23, 2024
1a24942
Add propTypes and remove HOC.
jimmymadon Dec 23, 2024
1bfffcb
Remove duplicated confirm GA tracking event.
jimmymadon Dec 23, 2024
5f61c3a
Remove redundant dismissal logic and GA tracking view event.
jimmymadon Dec 23, 2024
accf8e2
Remove all common dismissal logic.
jimmymadon Dec 23, 2024
5aec9ef
Fix incorrect title.
jimmymadon Dec 23, 2024
79a4bb7
Add support to dismiss and redisplay a notification.
jimmymadon Dec 23, 2024
20cc27e
Check prompts and dismissed items to determine notification dismissal…
jimmymadon Dec 23, 2024
007a69d
Use new notification prompt dismissal in RRM CTA component.
jimmymadon Dec 23, 2024
12ccc05
Prevent notification from showing after tooltip is rendered.
jimmymadon Dec 23, 2024
51fbdb1
Fix styles to match the legacy component.
jimmymadon Dec 23, 2024
48a16cd
Add the dismissRetries option to notification registration.
jimmymadon Dec 23, 2024
d10597f
Use dismissRetries property from notification property instead of pas…
jimmymadon Dec 23, 2024
7c12db9
Move dismissRetries for RRM setup banner from react component to noti…
jimmymadon Dec 23, 2024
91eed68
Only check for prompts if dismissRetries exists as a property.
jimmymadon Dec 23, 2024
7465e69
Add a default value to dismissExpires to prevent undefined checks.
jimmymadon Dec 23, 2024
010e78b
Improve notification dismissed selecter docs.
jimmymadon Dec 23, 2024
1570bae
Add a selector to fetch if a notification is on its final retry.
jimmymadon Dec 23, 2024
2775ad5
Add support for multiple dismiss labels.
jimmymadon Dec 23, 2024
e4c4f28
Fix undefined map select errors.
jimmymadon Dec 23, 2024
cec66b7
Move multiple dismiss labels out of common component.
jimmymadon Dec 26, 2024
af84a83
Refactor breakpoint SVG rendering for RRM banner.
jimmymadon Dec 26, 2024
a0a4c64
Refactor breakpoint SVG rendering for FPM banner.
jimmymadon Dec 26, 2024
3d2b8f3
Rename selector.
jimmymadon Dec 26, 2024
80bd3d5
Check if notification isDismissible before checking number of retries.
jimmymadon Dec 26, 2024
a17cb06
Provide FPM notification data for isDismissed check.
jimmymadon Dec 28, 2024
06b4c47
Provide FPM Notification data for isDismissed check in Ads settings.
jimmymadon Dec 28, 2024
45015c3
Separate out section of tests for isNotificationDismissed using dismi…
jimmymadon Dec 28, 2024
42f8b1b
Add tests for the isItemDismissed selector when using prompts.
jimmymadon Dec 28, 2024
7fc96bd
Merge branch 'develop' into enhancement/9889-refactor-rrm-setup-cta-b…
jimmymadon Jan 2, 2025
9217fed
Update rendering of RRM notification component in JS tests.
jimmymadon Jan 2, 2025
f6ac09f
Remove redundant trackEvent tests for RRM setup notification.
jimmymadon Jan 2, 2025
fa4d52d
Update test description.
jimmymadon Jan 2, 2025
ac4fe55
Refactor RRM notification registration to use an object of notificati…
jimmymadon Jan 2, 2025
dd8277f
Fix onCTAClick test.
jimmymadon Jan 2, 2025
0d23768
Prevent triggering survey when component is not rendered.
jimmymadon Jan 3, 2025
523073e
Fix all remaining tests for the RRM setup banner component.
jimmymadon Jan 3, 2025
1d99a46
Remove unnecessary use of view context when rendering the setup compo…
jimmymadon Jan 3, 2025
0fe3b7d
Update dismissed key to use notification ID.
jimmymadon Jan 3, 2025
99f56bc
Update storybook.
jimmymadon Jan 3, 2025
49527b5
Fix JSON response in test.
jimmymadon Jan 3, 2025
1af899d
Revert adding common setup banner svg wrapper styles.
jimmymadon Jan 5, 2025
2c65ee3
Duplicate Setup CTA styles for SVG wrapper just for RMM module.
jimmymadon Jan 6, 2025
c0b7aa6
Merge branch 'develop' into enhancement/9889-refactor-rrm-setup-cta-b…
jimmymadon Jan 6, 2025
72de7b0
Revert description to be exactly same as before.
jimmymadon Jan 6, 2025
fc2618b
Prevent RRM notification if it was dismissed before refactoring.
jimmymadon Jan 9, 2025
8b7e10a
Merge branch 'develop' into enhancement/9889-refactor-rrm-setup-cta-b…
jimmymadon Jan 9, 2025
aefd8e0
Inline variable.
jimmymadon Jan 9, 2025
f1ca9bf
Remove unnecessary white space before description.
jimmymadon Jan 9, 2025
1e1afa4
Standardise RRM banner heading.
jimmymadon Jan 9, 2025
00b207a
Remove unnecessary className change.
jimmymadon Jan 9, 2025
56b727d
Add default values in documentation.
jimmymadon Jan 9, 2025
2de7739
Add unit tests for isNotificationDismissalFinal selector.
jimmymadon Jan 9, 2025
8f1a8ed
Add test for legacy dismissal key check.
jimmymadon Jan 9, 2025
2606b6a
Update VRTs to standardise banner title font weight.
jimmymadon Jan 9, 2025
753116d
Merge branch 'develop' into enhancement/9889-refactor-rrm-setup-cta-b…
jimmymadon Jan 9, 2025
2406bda
Merge branch 'develop' into enhancement/9889-refactor-rrm-setup-cta-b…
aaemnnosttv Jan 10, 2025
0889c1b
Rename RRM dismissal key to be legacy.
aaemnnosttv Jan 10, 2025
c2172ee
Simplify setup notification check requirements.
aaemnnosttv Jan 10, 2025
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
10 changes: 0 additions & 10 deletions assets/js/components/DashboardMainApp.js
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,6 @@ import {
AudienceSegmentationSetupCTAWidget,
AudienceSelectionPanel,
} from '../modules/analytics-4/components/audience-segmentation/dashboard';
import ReaderRevenueManagerSetupCTABanner from '../modules/reader-revenue-manager/components/dashboard/ReaderRevenueManagerSetupCTABanner';
import EntitySearchInput from './EntitySearchInput';
import DateRangeSelector from './DateRangeSelector';
import HelpMenu from './help/HelpMenu';
Expand Down Expand Up @@ -88,7 +87,6 @@ import {

export default function DashboardMainApp() {
const audienceSegmentationEnabled = useFeature( 'audienceSegmentation' );
const readerRevenueManagerEnabled = useFeature( 'rrmModule' );

const [ showSurveyPortal, setShowSurveyPortal ] = useState( false );

Expand Down Expand Up @@ -276,14 +274,6 @@ export default function DashboardMainApp() {
</Fragment>
) }

{ ! viewOnlyDashboard && (
<Fragment>
{ readerRevenueManagerEnabled && (
<ReaderRevenueManagerSetupCTABanner />
) }
</Fragment>
) }

<Notifications
areaSlug={ NOTIFICATION_AREAS.BANNERS_BELOW_NAV }
groupID={ NOTIFICATION_GROUPS.SETUP_CTAS }
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ export default function ActionsCTALinkDismiss( {
ctaLabel,
onCTAClick,
onDismiss = () => {},
dismissLabelInitial,
dismissLabel = __( 'OK, Got it!', 'google-site-kit' ),
dismissExpires = 0,
dismissOptions = {},
Expand All @@ -57,6 +58,7 @@ export default function ActionsCTALinkDismiss( {
<Dismiss
id={ id }
primary={ false }
dismissLabelInitial={ dismissLabelInitial }
dismissLabel={ dismissLabel }
dismissExpires={ dismissExpires }
disabled={ isNavigatingToCTALink }
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,14 +27,15 @@ import { __ } from '@wordpress/i18n';
/**
* Internal dependencies
*/
import { useDispatch } from 'googlesitekit-data';
import { useDispatch, useSelect } from 'googlesitekit-data';
import useNotificationEvents from '../../hooks/useNotificationEvents';
import { CORE_NOTIFICATIONS } from '../../datastore/constants';
import { Button } from 'googlesitekit-components';

export default function Dismiss( {
id,
primary = true,
dismissLabelInitial,
dismissLabel = __( 'OK, Got it!', 'google-site-kit' ),
dismissExpires = 0,
disabled,
Expand All @@ -49,6 +50,10 @@ export default function Dismiss( {

const { dismissNotification } = useDispatch( CORE_NOTIFICATIONS );

const isDismissalFinal = useSelect( ( select ) =>
select( CORE_NOTIFICATIONS ).isNotificationRetryFinal( id )
);

const handleDismiss = async ( event ) => {
await onDismiss?.( event );
trackEvents.dismiss(
Expand All @@ -67,7 +72,7 @@ export default function Dismiss( {
onClick={ handleDismiss }
disabled={ disabled }
>
{ dismissLabel }
{ isDismissalFinal ? dismissLabel : dismissLabelInitial }
aaemnnosttv marked this conversation as resolved.
Show resolved Hide resolved
</Button>
);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,10 @@ export default function NotificationWithSVG( {
</Cell>
<Cell
alignBottom
className={ `googlesitekit-setup-cta-banner__svg-wrapper--${ id }` }
className={ classNames(
'googlesitekit-setup-cta-banner__svg-wrapper',
`googlesitekit-setup-cta-banner__svg-wrapper--${ id }`
) }
{ ...svgSizeProps }
>
<SVG />
Expand Down
67 changes: 63 additions & 4 deletions assets/js/googlesitekit/notifications/datastore/notifications.js
Original file line number Diff line number Diff line change
Expand Up @@ -64,10 +64,11 @@ export const actions = {
* @param {WPComponent} [settings.Component] React component used to display the contents of this notification.
* @param {number} [settings.priority] Notification's priority for ordering (lower number is higher priority, like WordPress hooks). Ideally in increments of 10. Default 10.
* @param {string} [settings.areaSlug] The slug of the area where the notification should be rendered, e.g. notification-area-banners-above-nav.
* @param {string} [settings.groupID] The ID of the group of notifications that should be rendered in their own individual queue.
* @param {string} [settings.groupID] Optional. The ID of the group of notifications that should be rendered in their own individual queue.
* @param {Array.<string>} [settings.viewContexts] Array of Site Kit contexts, e.g. VIEW_CONTEXT_MAIN_DASHBOARD.
* @param {Function} [settings.checkRequirements] Optional. Callback function to determine if the notification should be queued.
* @param {boolean} [settings.isDismissible] Flag to check if the notification should be queued and is not dismissed.
* @param {boolean} [settings.isDismissible] Optional. Flag to check if the notification should be queued and is not dismissed.
* @param {number} [settings.dismissRetries] Optional. An integer number denoting how many times a notification should be shown again on dismissal.
aaemnnosttv marked this conversation as resolved.
Show resolved Hide resolved
* @return {Object} Redux-style action.
*/
registerNotification(
Expand All @@ -80,6 +81,7 @@ export const actions = {
viewContexts,
checkRequirements,
isDismissible,
dismissRetries = 0,
}
) {
invariant(
Expand Down Expand Up @@ -117,6 +119,7 @@ export const actions = {
viewContexts,
checkRequirements,
isDismissible,
dismissRetries,
},
},
type: REGISTER_NOTIFICATION,
Expand Down Expand Up @@ -226,6 +229,25 @@ export const actions = {
return;
}

// Use prompts if a notification should be shown again until it
// is dismissed for a certain number of retries.
if ( notification.dismissRetries > 0 ) {
const dismissCount = registry
.select( CORE_USER )
.getPromptDismissCount( id );

const expirationInSeconds =
dismissCount < notification.dismissRetries
? expiresInSeconds
: 0;

return yield commonActions.await(
registry.dispatch( CORE_USER ).dismissPrompt( id, {
expiresInSeconds: expirationInSeconds,
} )
);
}

return yield commonActions.await(
registry
.dispatch( CORE_USER )
Expand Down Expand Up @@ -407,8 +429,8 @@ export const selectors = {
/**
* Determines whether a notification is dismissed or not.
*
* Currently, this selector simply forwards the call to the dismissed items API.
* We can potentially add more notification-specific logic here in the future.
* If the notification should appear again for a certain number of times after dismissal,
* then we store them as prompts. So we check for dismissed prompts instead of dismissed items.
*
* @since 1.132.0
*
Expand All @@ -418,9 +440,46 @@ export const selectors = {
*/
isNotificationDismissed: createRegistrySelector(
( select ) => ( state, id ) => {
const notification =
select( CORE_NOTIFICATIONS ).getNotification( id );

if ( notification.dismissRetries > 0 ) {
return select( CORE_USER ).isPromptDismissed( id );
}

return select( CORE_USER ).isItemDismissed( id );
}
),
/**
* Determines whether a notification that can reappear again for a fixed number of times
* on dismissal is at its final appearance.
*
* @since n.e.x.t
*
* @param {Object} state Data store's state.
* @param {string} id Notification id.
* @return {(boolean|undefined)} TRUE if notification is on its final retry, otherwise FALSE, `undefined` if not resolved yet.
*/
isNotificationRetryFinal: createRegistrySelector(
( select ) => ( state, id ) => {
const notification =
select( CORE_NOTIFICATIONS ).getNotification( id );

// If a notification does not have retries, it always will be on its final render.
if ( notification.dismissRetries === 0 ) {
return true;
}
aaemnnosttv marked this conversation as resolved.
Show resolved Hide resolved

const dismissCount =
select( CORE_USER ).getPromptDismissCount( id );

if ( dismissCount >= notification.dismissRetries ) {
return true;
}

return false;
}
),
};

export default {
Expand Down
Loading
Loading