Enhancement/10175 setup banner #10208
Conversation
…eRedirectModal' into enhancement/10175-setup-banner.
|
Build files for d9b3a1d are ready:
|
…ooCommerceRedirectModal.
…quirements to use ads datastore.
This comment was marked as resolved.
This comment was marked as resolved.
| @@ -20,13 +20,3 @@ export const CORE_SITE = 'core/site'; | |||
|
|
|||
| export const AMP_MODE_PRIMARY = 'primary'; | |||
| export const AMP_MODE_SECONDARY = 'secondary'; | |||
|
|
|||
There was a problem hiding this comment.
selectors Inline data has been moved from core site datastore to the Ads datastore, otherwise issues will remain broken and blocked, so this is done as part of this issue. More details in PR description
| @@ -55,13 +55,13 @@ export const selectors = { | |||
|
|
|||
| if ( isModuleConnected ) { | |||
| return __( | |||
| 'Ad blocker detected; please disable it to get the latest Ads data', | |||
| 'To get the latest Ads data you will need to disable your Ad blocker', | |||
There was a problem hiding this comment.
Adsense module warning message will be replaced in #10193. This will be the new message we will use for ad blocker everywhere (verified with Sigal and Mariya in UX channel)
There was a problem hiding this comment.
Thanks, @zutigrm. This is a very strong start, but we need to make a few improvements. Please, see my comments.
|
|
||
| function AdsModuleSetupCTAWidget( { WidgetNull, Widget } ) { | ||
| export default function AdsModuleSetupCTAWidget( { id, Notification } ) { |
There was a problem hiding this comment.
Ok, we need to move this component to the Ads module because we register it there. Plus this is no longer a widget, thus we need to rename it accordingly.
| @@ -35,6 +35,7 @@ export default function Description( { | |||
| text, | |||
| learnMoreLink, | |||
| errorText, | |||
| AdditionalComponent, | |||
There was a problem hiding this comment.
This should be children.
| SVG={ breakpointSVGMap[ breakpoint ] || AdsSetupSVG } | ||
| /> | ||
| </Notification> | ||
| <WooCommerceRedirectModal |
There was a problem hiding this comment.
Let's move this to the Notification component children and remove Fragment usage.
| <WooCommerceRedirectModal | ||
| dialogActive={ openDialog } | ||
| onDismiss={ onModalDismiss } | ||
| /> |
There was a problem hiding this comment.
Perhaps instead of always rendering this component, we should render it conditionally?
{ openDialog && <WooCommerceRedirectModal onDismiss={ onModalDismiss } dialogActive /> }| id: PropTypes.string, | ||
| Notification: PropTypes.elementType, |
There was a problem hiding this comment.
These props should be required.
|
|
||
| if ( isTooltipVisible ) { | ||
| return ( | ||
| <Fragment> |
There was a problem hiding this comment.
Fragment is not needed here.
Summary
Addresses issue:
Relevant technical choices
Priority for ads banner has changed after this discussion with Mariya, Ads banner should go before audience segmentation
Note
Since follow up PR for #10169 moved the inline data to
Adsdatastore as persistent module data, it broke 3 issues which implement the selectors from site datastore, including one currently in QA. Due to the time constrains and to avoid getting 3 ready for review testing issues getting blocked, selectors were moved to the Ads datastore in this issue.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