Settings: Allow Users to Insert Header Code #86079

Aurorum · 2024-01-06T15:37:05Z

Requires Automattic/jetpack#34881 (also, I need to add a version check here once that Jetpack PR is merged)

Proposed Changes

Allows user to directly edit their <head> element without relying on a third-party plugin. A plugin to insert code in the Header element is now the eighth most popular on WP.com (!!), so hopefully this will make things easier for both users and those in Support.

Rather than using a default textbox which created a strange experience, it felt more appropriate to use CodeMirror. Unfortunately, that means adding another package, but I've used the same one already used in Automattic projects like Newspack so I don't think that's a problem.

Testing Instructions

Apply the Jetpack PR
Go to Settings > General and scroll to "Site Tools" on an Atomic or Jetpack site (this didn't really seem to fit within any section, and the section cards are quite small for a code editor!)
Select "Header code"
Enter some header code, and verify that it's included on your site

Pre-merge Checklist

Can't access P2s, but have tested on Simple and Atomic - the former can't access this page from Settings, and a direct link shows an upgrade nudge.

Has the general commit checklist been followed? (PCYsg-hS-p2)
[] Have you written new tests for your changes?
Have you tested the feature in Simple (P9HQHe-k8-p2), Atomic (P9HQHe-jW-p2), and self-hosted Jetpack sites (PCYsg-g6b-p2)?
Have you checked for TypeScript, React or other console errors?
Have you used memoizing on expensive computations? More info in Memoizing with create-selector and Using memoizing selectors and Our Approach to Data
Have we added the "[Status] String Freeze" label as soon as any new strings were ready for translation (p4TIVU-5Jq-p2)?
For changes affecting Jetpack: Have we added the "[Status] Needs Privacy Updates" label if this pull request changes what data or activity we track or use (p4TIVU-ajp-p2)?

cc @tyxla - it looks like you've worked on Settings regularly before, might this maybe fall into your remit? thanks!! :)

Aurorum · 2024-01-06T15:39:05Z

@donalirl do you have a link to this document that I could include here? Thanks!

On the page, we could also link to the support doc being created in 1210-gh-Automattic/en.support-docs-content, so customers know how to use this screen without needing to contact us.

donalirl · 2024-01-08T06:39:00Z

@Aurorum Yes! This is the doc: https://wordpress.com/support/adding-code-to-headers/

Right now the instructions are for the plugin but we can update it to provide instructions for this feature instead.

Aurorum · 2024-01-08T08:33:08Z

Thanks @donalirl! I've added it to the text - let me know if there's anything that I should change.

tyxla

Thank you for working on this @Aurorum 🙌
I don't work on settings nowadays, but of course, happy to do a general broader review.
This could use a Jetpack person to review it - cc @anomiex since he seems to be helping with reviewing on the Jetpack side.

client/my-sites/site-settings/header-code/index.jsx

tyxla · 2024-01-08T15:03:53Z

client/my-sites/site-settings/controller.js

@@ -111,6 +112,11 @@ export function startSiteOwnerTransfer( context, next ) {
 	next();
 }

+export function headerCode( context, next ) {
+	context.primary = <HeaderCodeSettings />;


Will all site types support this? I see Jetpack sites will, but what about WP.com simple / Atomic / etc.?

The code in Automattic/jetpack#34881 explicitly excludes Simple. As things stand I don't see anything stopping it from Atomic.

In that case, we should make sure to redirect in Calypso if someone tries to access this with a WP.com simple site.

See my comments below!

client/my-sites/site-settings/site-tools/index.jsx

client/my-sites/site-settings/index.js

client/my-sites/site-settings/site-tools/index.jsx

package.json

anomiex · 2024-01-08T15:25:57Z

This could use a Jetpack person to review it - cc @anomiex since he seems to be helping with reviewing on the Jetpack side.

I can't say I know much about Calypso's settings either. 😅 Mostly I saw the OSS Citizen PR come across and added the Needs Review tag to it, then left a comment to answer a question asked regarding escaping.

tyxla · 2024-01-08T15:28:03Z

This could use a Jetpack person to review it - cc @anomiex since he seems to be helping with reviewing on the Jetpack side.

I can't say I know much about Calypso's settings either. 😅 Mostly I saw the OSS Citizen PR come across and added the Needs Review tag to it, then left a comment to answer a question asked regarding escaping.

I'm totally fine to do the Calypso-side review, but lack the product context, for which a Jetpack person could be useful (to test the feature at the bare minimum).

Aurorum · 2024-01-08T17:53:44Z

Thank you so much for the detailed review @tyxla - very much appreciated! I've addressed the recommendations you made.

Just wanted to touch a little further about access - this is primarily intended for WordPress.com Atomic sites to make it easier for them to insert code without needing to install a third-party plugin. Jetpack is only used to include the code on the user's site. I didn't see the harm in letting Jetpack sites access this too though. I've just worked on the basis of the the original issue, so let me know if things should be changed. :)

Custom code can't be inserted on Simple sites. I've handled this by hiding this option from the Settings page, and they still won't be able to add custom code if they use a direct link. Instead, they'll see an upgrade nudge and the button will be disabled:

If you'd like to redirect them away directly instead, just let me know! I think that a few other screens on Calypso take this approach (eg. the Earn page), but I don't think it'll make much difference either way.

I hope that clarifies access, and thanks again for the review!! :)

client/my-sites/site-settings/header-code/index.jsx

tyxla · 2024-01-09T15:43:15Z

client/my-sites/site-settings/header-code/index.jsx

+				<>
+					<NavigationHeader
+						title={ translate( 'Header Code' ) }
+						subtitle={ translate( 'Insert code within the Header element of your site.' ) }


"Header element" is definitely confusing when we talk about the "head" element. So we might want to change it indeed.

tyxla · 2024-01-09T15:44:41Z

client/my-sites/site-settings/header-code/index.jsx

+					</HeaderCake>
+					<Card>
+						<UpsellNudge
+							plan={ PLAN_BUSINESS }


But PLAN_BUSINESS is the WP.com plan, not the Jetpack plan. That upsell wouldn't make sense for a non-Atomic Jetpack site, because a non-Atomic Jetpack site can't purchase a WP.com plan. Does that makes sense?

client/my-sites/site-settings/header-code/index.jsx

tyxla · 2024-01-09T15:48:22Z

client/my-sites/site-settings/header-code/index.jsx

+					<Card>
+						<UpsellNudge
+							plan={ PLAN_BUSINESS }
+							title={ translate( 'Add custom code to your site with the %(planName)s plan.', {


What about Personal (AKA Starter) and Premium (AKA Explorer) plans? Does only the WP.com business plan have this feature?

client/my-sites/site-settings/header-code/style.scss

client/my-sites/site-settings/index.js

tyxla · 2024-01-09T15:56:10Z

Thank you so much for the detailed review @tyxla - very much appreciated! I've addressed the recommendations you made.

Thank you for the great work once more!

Just wanted to touch a little further about access - this is primarily intended for WordPress.com Atomic sites to make it easier for them to insert code without needing to install a third-party plugin. Jetpack is only used to include the code on the user's site. I didn't see the harm in letting Jetpack sites access this too though. I've just worked on the basis of the the original issue, so let me know if things should be changed. :)

Understood. I just wanted to clarify that Jetpack sites can't purchase a WP.com Creator plan because they are different plans. Jetpack Creator and WP.com Creator are two different products, and this isn't reflected in the plan upsell nudge and potentially a few other places. Let me know if that makes sense.

If you'd like to redirect them away directly instead, just let me know! I think that a few other screens on Calypso take this approach (eg. the Earn page), but I don't think it'll make much difference either way.

Yes, we should redirect those settings pages if they're not available for a particular site type.

Aurorum · 2024-01-09T16:07:09Z

Thanks @tyxla - I'll address the feedback tonight! However, just a very quick question - is it possible to have a Jetpack site which isn't Atomic? I thought that condition couldn't exist, and what would that mean in practice? My intention was that no Jetpack plan sees the nudge, and would be able to use the feature. That seems to work in my testing with a local site connected with Jetpack, so I suppose that I'm currently a little confused about what condition I might be missing!

…

On Tue, 9 Jan 2024, 15:56 Marin Atanasov, ***@***.***> wrote: Thank you so much for the detailed review @tyxla <https://github.com/tyxla> - very much appreciated! I've addressed the recommendations you made. Thank you for the great work once more! Just wanted to touch a little further about access - this is primarily intended for WordPress.com Atomic sites to make it easier for them to insert code without needing to install a third-party plugin. Jetpack is only used to include the code on the user's site. I didn't see the harm in letting Jetpack sites access this too though. I've just worked on the basis of the the original issue, so let me know if things should be changed. :) Understood. I just wanted to clarify that Jetpack sites can't purchase a WP.com Creator plan because they are different plans. Jetpack Creator and WP.com Creator are two different products, and this isn't reflected in the plan upsell nudge and potentially a few other places. Let me know if that makes sense. If you'd like to redirect them away directly instead, just let me know! I think that a few other screens on Calypso take this approach (eg. the Earn page), but I don't think it'll make much difference either way. Yes, we should redirect those settings pages if they're not available for a particular site type. — Reply to this email directly, view it on GitHub <#86079 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AKJWTFONLJGMWXYJV23JSFLYNVSCNAVCNFSM6AAAAABBPUELJKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQOBTGMYTONBYHA> . You are receiving this because you were mentioned.Message ID: ***@***.***>

tyxla · 2024-01-09T17:19:41Z

just a very quick question - is it possible to have a Jetpack site which isn't Atomic?

Yes. Atomic sites are essentially hosted on the WP.com Atomic platform, which is powered by Jetpack and a few more plugins. However, any WordPress site that's hosted anywhere can also have Jetpack installed and be connected to WP.com through Jetpack - we call those Jetpack sites, and they're not Atomic sites because they're not hosted on WP.com. Let me know if that makes sense!

Aurorum · 2024-01-09T17:24:48Z

Sorry - my mistake, I meant whether it was possible to have a Jetpack site that *is* Atomic!! I thought Atomic referred to WP.com sites on the Creator plan or above with plugins installed.

…

On Tue, 9 Jan 2024, 17:19 Marin Atanasov, ***@***.***> wrote: just a very quick question - is it possible to have a Jetpack site which isn't Atomic? Yes. Atomic sites are essentially hosted on the WP.com Atomic platform, which is powered by Jetpack and a few more plugins. However, any WordPress site that's hosted anywhere can also have Jetpack installed and be connected to WP.com through Jetpack - we call those Jetpack sites, and they're not Atomic sites because they're not hosted on WP.com. Let me know if that makes sense! — Reply to this email directly, view it on GitHub <#86079 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AKJWTFJN6K4I3G6D2QOVX6LYNV33TAVCNFSM6AAAAABBPUELJKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQOBTGQ3TEMBRGE> . You are receiving this because you were mentioned.Message ID: ***@***.***>

Aurorum · 2024-01-09T19:29:02Z

Thanks again for the review @tyxla! I've addressed the latest set of feedback - it now redirects WP.com Simple sites to General settings (whereas previously these sites would have seen the upgrade nudge.

If I'm still misunderstanding access even after this change, please let me know!

tyxla · 2024-01-10T09:26:55Z

Sorry - my mistake, I meant whether it was possible to have a Jetpack site that is Atomic!! I thought Atomic referred to WP.com sites on the Creator plan or above with plugins installed.

Atomic sites are powered by Jetpack, which also makes them Jetpack sites. Also, the jetpack property of an Atomic site is true.

Aurorum · 2024-01-10T09:31:23Z

Ah, okay, thank you! I would’ve assumed that the access was set correctly on that basis though - Atomic/Jetpack sites would be able to access this feature.

I’ve tested with an Atomic, Simple and Jetpack site and I’m pretty sure this is in the right place - but it’s the same logic as what I included in the upsell nudge, so please let me know if I’ve missed something! :)

tyxla · 2024-01-10T09:32:40Z

Thanks for the great work @Aurorum, I'll take another look 👍

Aurorum · 2024-01-10T09:33:26Z

No rush at all, thanks so much for taking the time to review. :)

tyxla

Thank you @Aurorum, great work so far! I've tested this and left some more feedback after giving it a try. Hope that helps, let me know if you have any questions.

client/my-sites/site-settings/header-code/index.jsx

tyxla · 2024-01-10T10:00:05Z

client/my-sites/site-settings/site-tools/index.jsx

@@ -219,6 +231,7 @@ export default compose( [
 				showClone: 'active' === rewindState.state && ! isAtomic,
 				showDeleteContent: isAtomic || ( ! isJetpack && ! isVip && ! isP2Hub ),
 				showDeleteSite: ( ! isJetpack || isAtomic ) && ! isVip && sitePurchasesLoaded,
+				showHeaderCode: isJetpack || isAtomic,


Should we show this only to admins? (if the current user has the capability to manage_options)

Not necessary in this case! The entire page is blocked for non-admins.

client/my-sites/site-settings/header-code/index.jsx

client/my-sites/site-settings/header-code/style.scss

tyxla · 2024-01-10T10:24:57Z

client/my-sites/site-settings/header-code/style.scss

+	margin-bottom: 1em;
+	min-height: 250px;


Would be nice to avoid adding extra styles and keep consistency with the existing design. Perhaps a min-height is not necessary, and the margin bottom can also be omitted.

I'd like to (respectfully!) push back on removing the min-height, if that's okay. The section already provides a lot of space so it feels weird without it, and it's also very hard to see much of the code without expanding it. That creates an extra step if, for example, a user wants to tweak the value of some code they've pasted.

I think a bit of extra styling is justified when it would make things a little easier and look better - but, of course, happy to be overruled!

What if we use the TextareaAutosize component which will automatically resize to show the text if it's more lines? Will that resolve your height concerns?

I think the trouble then emerges if some users have a lot of code and would need to scroll to the bottom for the "Save" button even to make a tiny change, which means it'd probably be necessary to have a max-height on that too (plus it still leaves a lot of space!).

Okay, let's go with what you suggested for now. We can always reconsider and change with TextareaAutosize at a later time. Thanks for considering it 👍

client/my-sites/site-settings/header-code/style.scss

client/my-sites/site-settings/index.js

client/my-sites/site-settings/site-tools/index.jsx

tyxla · 2024-01-10T10:30:04Z

Another thing: when the Jetpack PR gets landed, and Jetpack gets released with it, there will still be users who use the old Jetpack version. Once that happens, we might need a Jetpack version check here that ensure that we load and display this UI only when the API is supported by the current Jetpack version.

Aurorum · 2024-01-10T12:12:10Z

Thanks for another round of great feedback @tyxla! 🙌 Should be addressed now.

Not to worry about the Jetpack version check - very much got that in mind, but just waiting to see which Jetpack release the endpoint will be included in. :)

tyxla · 2024-01-10T12:15:42Z

Not to worry about the Jetpack version check - very much got that in mind, but just waiting to see which Jetpack release the endpoint will be included in. :)

It's important to keep in mind, because AFAIK we officially support "Jetpack's current version - 1", meaning that once the Jetpack API changes are released, we'll still need to support the previous version of Jetpack and not display this field. Alternatively, we could land this one on the next release after the one with the API. Then, no additional checks would be needed. Let me know if that makes sense.

Aurorum · 2024-01-10T12:20:34Z

Got it! I was intending to use versionCompare when the Jetpack PR lands (see this PR's description) but happy to add something now and change the value depending on how the Jetpack PR progresses.

tyxla · 2024-01-10T12:22:37Z

This looks pretty good @Aurorum, thank you for the great work 🙌 🚀

Other than the Jetpack version concerns, I think this will be good to go.

I'm happy to do a last round of review once the corresponding Jetpack API change is released and you take the version into consideration - let me know and I'll re-review and help you with shipping it.

tyxla · 2024-01-10T12:23:42Z

Got it! I was intending to use versionCompare when the Jetpack PR lands (see this PR's description) but happy to add something now and change the value depending on how the Jetpack PR progresses.

No need to add it now. It can wait until after the Jetpack release that contains the API additions.

Aurorum · 2024-01-10T12:24:24Z

Thanks so much @tyxla - very much appreciated! I'll give you a ping once the Jetpack PR lands and I've added a version check here. :)

Aurorum added 2 commits January 5, 2024 14:53

Update package.json

65ee5f3

Allow users to add header code

3181733

github-actions bot added the OSS Citizen label Jan 6, 2024

This was referenced Jan 6, 2024

WP.com API: Allow Users to Insert Header Code Automattic/jetpack#34881

Closed

Atomic Sites: Add a Built-In Method to Put Code In Header #56023

Open

Aurorum added 2 commits January 6, 2024 17:45

Split string

68d9726

Improve loading state

41b73cd

Add support link

6c3af5c

tyxla reviewed Jan 8, 2024

View reviewed changes

Aurorum added 2 commits January 8, 2024 17:01

Remove codemirror

e77dc8c

Address code review

c86ee66

tyxla reviewed Jan 9, 2024

View reviewed changes

Address feedback

2f8c8df

tyxla reviewed Jan 10, 2024

View reviewed changes

Address feedback

0597568

Settings: Allow Users to Insert Header Code #86079

Are you sure you want to change the base?

Settings: Allow Users to Insert Header Code #86079

Conversation

Aurorum commented Jan 6, 2024 • edited Loading

Proposed Changes

Testing Instructions

Pre-merge Checklist

Aurorum commented Jan 6, 2024

donalirl commented Jan 8, 2024

Aurorum commented Jan 8, 2024

tyxla left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anomiex commented Jan 8, 2024

tyxla commented Jan 8, 2024

Aurorum commented Jan 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tyxla commented Jan 9, 2024

Aurorum commented Jan 9, 2024 via email

tyxla commented Jan 9, 2024

Aurorum commented Jan 9, 2024 via email

Aurorum commented Jan 9, 2024 • edited Loading

tyxla commented Jan 10, 2024

Aurorum commented Jan 10, 2024

tyxla commented Jan 10, 2024

Aurorum commented Jan 10, 2024

tyxla left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tyxla commented Jan 10, 2024

Aurorum commented Jan 10, 2024

tyxla commented Jan 10, 2024

Aurorum commented Jan 10, 2024

tyxla commented Jan 10, 2024 • edited Loading

tyxla commented Jan 10, 2024

Aurorum commented Jan 10, 2024

Aurorum commented Jan 6, 2024 •

edited

Loading

Aurorum commented Jan 8, 2024 •

edited

Loading

Aurorum commented Jan 9, 2024 •

edited

Loading

tyxla commented Jan 10, 2024 •

edited

Loading