Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

3rd Party: Remove code from the global scope #14264

Merged
merged 1 commit into from
Dec 19, 2019
Merged

3rd Party: Remove code from the global scope #14264

merged 1 commit into from
Dec 19, 2019

Conversation

kraftbj
Copy link
Contributor

@kraftbj kraftbj commented Dec 19, 2019

Browsing the codebase and saw that we had this globally scoped. I'd hate to somehow run into a conflict with other globally scoped things.

Changes proposed in this Pull Request:

  • Encapsulate loading code in a function.

Is this a new feature or does it add/remove features to an existing part of Jetpack?

  • n/a

Testing instructions:

Confirm the 3rd-party code loads. Debug bar is an easy way to verify JP code is loading as a panel.

  • Run yarn docker:wp jetpack module activate search on a test site with Professional.
  • Install Debug Bar
  • Confirm a Jetpack Search panel.

Proposed changelog entry for your changes:

  • I'd include it in a general line item about improved adherence to coding standards.

@kraftbj kraftbj added [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it General [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Dec 19, 2019
@kraftbj kraftbj added this to the 8.1 milestone Dec 19, 2019
@kraftbj kraftbj requested a review from a team December 19, 2019 00:45
@kraftbj kraftbj self-assigned this Dec 19, 2019
@jetpackbot
Copy link

Thank you for the great PR description!

When this PR is ready for review, please apply the [Status] Needs Review label. If you are an a11n, please have someone from your team review the code if possible. The Jetpack team will also review this PR and merge it to be included in the next Jetpack release.

Scheduled Jetpack release: January 14, 2020.
Scheduled code freeze: January 7, 2020

Generated by 🚫 dangerJS against 0bad9cf

@matticbot
Copy link
Contributor

Caution: This PR has changes that must be merged to WordPress.com
Hello kraftbj! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer, review, and approve D36932-code before merging this PR. Thank you!

jeherve
jeherve approved these changes Dec 19, 2019
View reviewed changes
Copy link
Member

@jeherve jeherve left a comment

Choose a reason for hiding this comment

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

Good catch!

@jeherve jeherve added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Dec 19, 2019
@kraftbj kraftbj merged commit 8537cdd into master Dec 19, 2019
@kraftbj kraftbj deleted the fix/no-global-vars branch December 19, 2019 17:00
@matticbot matticbot added [Status] Needs Changelog and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Dec 19, 2019
jeherve added a commit that referenced this pull request Dec 20, 2019
@jeherve
Changelog: add #14264
d8ebcc4
zinigor added a commit that referenced this pull request Dec 30, 2019
@jeherve @zinigor
Changelog: 8.1 additions (#14166)
fff089c 
* Changelog: 8.1 additions

* Changelog: add #13858

* Changelog: add #13963

* Changelog: add #14174

* Changelog: add #14178

* Changelog: add #14175

* Changelog: add #14192

* Changelog: add #14196

* Changelog: add #14182

* Changelog: add #14218

* Changelog: add #14214

* Changelog: add #13757

* Changelog: add #14190

* Changelog: add #14131

* Changelog: add #14101

* Changelog: add #14203

* Changelog: add #14211

* Changelog: add #14224

* Changelog: add #14230

* Changelog: add #14241

* Changelog: add #14249

* Changelog: add #14264

* Changelog: add #14263

* Changelog: add #14256

* Changelog: add #10189

* Changelog: add #14240

* Changelog: add #14239

Also added some new entries to the testing file.

Co-authored-by: Igor Zinovyev <[email protected]>
zinigor added a commit that referenced this pull request Dec 30, 2019
@jeherve @zinigor
Changelog: 8.1 additions (#14166)
d4a7565 
* Changelog: 8.1 additions

* Changelog: add #13858

* Changelog: add #13963

* Changelog: add #14174

* Changelog: add #14178

* Changelog: add #14175

* Changelog: add #14192

* Changelog: add #14196

* Changelog: add #14182

* Changelog: add #14218

* Changelog: add #14214

* Changelog: add #13757

* Changelog: add #14190

* Changelog: add #14131

* Changelog: add #14101

* Changelog: add #14203

* Changelog: add #14211

* Changelog: add #14224

* Changelog: add #14230

* Changelog: add #14241

* Changelog: add #14249

* Changelog: add #14264

* Changelog: add #14263

* Changelog: add #14256

* Changelog: add #10189

* Changelog: add #14240

* Changelog: add #14239

Also added some new entries to the testing file.

Co-authored-by: Igor Zinovyev <[email protected]>
@kraftbj
Copy link
Contributor Author

kraftbj commented Apr 14, 2020

r205837-wpcom

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jeherve jeherve jeherve approved these changes

Assignees

@kraftbj kraftbj

Labels
General Touches WP.com Files [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it
Projects
None yet
Milestone
8.1
Development

Successfully merging this pull request may close these issues.
4 participants
@kraftbj @jetpackbot @matticbot @jeherve