Consider Analytics Ads linked status in Consent Mode conditions #8431
Conversation
|
Build files for 6ae739b have been deleted. |
|
Size Change: -471 B (0%) Total Size: 1.41 MB
ℹ️ View Unchanged
|
| const isAdsConnected = useSelect( ( select ) => { | ||
| // TODO: Replace the `getAdsConversionID()` selector with its `ads` | ||
| // version once it's implemented. | ||
|
|
||
| const { getAdsConversionID, getAdsLinked } = | ||
| select( MODULES_ANALYTICS_4 ); | ||
|
|
||
| return getAdsConversionID() || getAdsLinked(); | ||
| } ); |
There was a problem hiding this comment.
This exact logic is used across three different components to do the same thing, let's create a new selector that returns a boolean from !!getAdsConversionID() || !!getAdsLinked() instead of doing this three different places 🙂
There was a problem hiding this comment.
As discussed, I've added a selector and re-used it in both places. Please let me know if this looks good now, thanks!
Addressed, thank you for the kind review, @tofumatt! |
| const { getAdsConversionID, getAdsLinked } = | ||
| select( MODULES_ANALYTICS_4 ); | ||
|
|
||
| return !! getAdsConversionID() || !! getAdsLinked(); |
There was a problem hiding this comment.
Oh, one question I just thought of: should we be returning undefined from here if these selectors are still loading? Right now undefined would return false, but that might cause this to go from a "false" to "true" state during loading, which isn't expected. 🤔
I don't think it affects any current components, but it might trip us up in the future. Can you add some undefined handling here? (Unless it isn't needed.)
There was a problem hiding this comment.
I agree. I have updated the selector to return undefined if any of these two selectors return undefined.
You're right. I also don't think this affects any current components, as a result, I haven't added any loading state into them. But please let me know if I should do so instead.
Please let me know if this looks good now, thank you!
Addressed, thank you Matt! |
Summary
Addresses issue:
Relevant technical choices
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