Enhancement/11578 AdSense Alert CTA should open in a new tab #11705
Conversation
|
Storybook is ready:
|
|
Build files for 1daeb60 are ready:
|
|
Size Change: +1.08 kB (+0.05%) Total Size: 2.19 MB ℹ️ View Unchanged
|
tofumatt
left a comment
tofumatt
left a comment
There was a problem hiding this comment.
I know it's not part of this issue, but we might as well fix the Reader Revenue ad blocker notification not having an external "Learn more" link while we're at it 🤔
More generally though: this is a roundabout way of handling the already-server-supplied target prop and overloading its meaning. I wouldn't expect marking something to open in a new window to be the same as an external link, nor the inverse. But now: I have to say "anything in a new window is external", even though that might not be the case.
And, though it's less likely: we can't mark anything as external but keep it opening in the same tab.
I think the IB here isn't the right approach; we should be adding an external prop from the server notifications and using both it and the target prop to set the CTAButton's props accordingly.
| isSaving={ inProgress } | ||
| onClick={ onClick } | ||
| href={ href } | ||
| target={ external ? '_blank' : '_self' } |
There was a problem hiding this comment.
Why explicitly specify target="_self" here? We can just do undefined if the link isn't external and the attribute will be omitted entirely, which is arguably better.
| target={ external ? '_blank' : '_self' } | |
| target={ external ? '_blank' : undefined } |
There was a problem hiding this comment.
@tofumatt This currently follows the exact pattern introduced in the other similar assets/js/components/Notice/CTAButton.js component. So I wouldn't want to deviate for the exact same logic - and if we do, perhaps we should change this in both places. But this feels too minor to warrant additional testing, etc? Same for the comment below.
There was a problem hiding this comment.
Yes, let's change it in both places then. There's no reason to specify target="_self", so let's change it as part of this PR.
It will result in snapshots needing updating but the behaviour is the same, so let's just remove the extra (unneeded) code here so we don't continue with the target="_self" code 😄
| onClick, | ||
| href, | ||
| external, | ||
| hideExternalIndicator, |
There was a problem hiding this comment.
This is undefined by default, but we should be a bit more explicit and mark this as false by default.
There was a problem hiding this comment.
Let's update it in both places then, those kinds of small improvements are fine to make as a result of code review here 🙂
|
@tofumatt Thanks for raising these points!
I believe this is a pattern followed throughout Site Kit: site-kit-wp/assets/js/components/Link.js Lines 118 to 120 in 33909dd So if we do think otherwise, we would have to be consistent with our definitions across the plugin. |
tofumatt
left a comment
tofumatt
left a comment
There was a problem hiding this comment.
I understand that an external link will always open in a new window, and that is actually a fair rule to implement. I think it's expected, as well, that marking something as external will always result in the link's behaviour changing.
What I don't think is a reasonable expectation is that marking any link as needing to open in a new window means it's a link external to Site Kit. Even if that's currently always the case, it's not intuitive that specifying target="_blank" on a button will mean that button is external to Site Kit and should be marking as such.
I don't think that's the API we should be building—but I do understand that the issue here is we are dealing with a notifications API on the Proxy/Service that we would need to change.
Ultimately I think the appropriate thing to do is to surface this "overloading" on the ctaTarget attribute in as few places as possible and actually to have the notifications API on the Service send better data down, but I'm not entirely convinced that part is worth the effort.
I think it's worth documenting this in the code but we don't need to add prop names.
Let's clean up some of the undefined vs false code and the target="_self" stuff though; we don't need to re-implement things that aren't explicit/optimal if we can straightforwardly refactor them in this PR. 🙂
I forgot to reply to this in my last round - but this does seem to have an external link. I'm not sure which component the screenshot you've pasted is referring to. 
|
Co-authored-by: Matthew Riley MacPherson <[email protected]>
… with Banner CTAButton.
…nal and maintain consistency with Banner CTAButton.
|
@tofumatt The 3 VRT failures here are related to charts not loading and hence are irrelevant to this PR. |
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