Skip to content

Conversation

@westonruter
Copy link
Contributor

@westonruter westonruter commented Nov 21, 2024

Summary

Addresses issue:

Relevant technical choices

Use wp_print_inline_script_tag() and wp_print_script_tag() rather than manually print script tags.

PR Author Checklist


Do not alter or remove anything below. The following sections will be managed by moderators only.

Code Reviewer Checklist

  • Run the code.
  • Ensure the acceptance criteria are satisfied.
  • Reassess the implementation with the IB.
  • Ensure no unrelated changes are included.
  • Ensure CI checks pass.
  • Check Storybook where applicable.
  • Ensure there is a QA Brief.
  • Ensure there are no unexpected significant changes to file sizes.

Merge Reviewer Checklist

  • Ensure the PR has the correct target branch.
  • Double-check that the PR is okay to be merged.
  • Ensure the corresponding issue has a ZenHub release assigned.
  • Add a changelog message to the issue.

Copy link
Member

@felixarntz felixarntz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@westonruter This looks good to me, but it would be great to get someone more active in the project to approve as a 2nd sign-off.

@aaemnnosttv Would you be able to get this into one of the next releases? It blocks WordPress/performance#1686 that we're exploring to improve performance implications of gtag.

(I believe Site Kit may have a few more spots where it would ideally use wp_print_inline_script_tag() rather than direct output of the script. Maybe that's something to do in a separate PR, if you agree.)

Copy link
Collaborator

@aaemnnosttv aaemnnosttv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @westonruter – this looks like a nice improvement, although like I mentioned on the issue, our minimum supported version of WP is 5.2 but this function wasn't added until 5.7. We do have a BC helper class which makes this available, but without it, this is breaking E2E in the older version.
https://github.com/google/site-kit-wp/actions/runs/11945338420/job/33297846941?pr=9726#step:11:387

I also noted another instance where this could be applied which may not have been there when you opened this, I'm not sure. LMK if you have any questions 👍

@westonruter
Copy link
Contributor Author

Note: I do not have Site Kit set up for local development, so these changes I applied via the GitHub UI and thus I'm relying on GHA to check for any problems.

@westonruter
Copy link
Contributor Author

The E2E tests are passing for WordPress 5.2.21, AMP 1.5.5.

But they're failing for latest and trunk. The errors don't seem to indicate they're related to the code in this PR, however, so perhaps they just need to be restarted.

Copy link
Collaborator

@aaemnnosttv aaemnnosttv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, thanks @westonruter !

@aaemnnosttv aaemnnosttv merged commit 7fd46d3 into google:develop Nov 26, 2024
18 of 19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants