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

Ads module: Use vanilla JS instead of jQuery #14770

Merged
merged 3 commits into from
Feb 25, 2020
Merged

Ads module: Use vanilla JS instead of jQuery #14770

merged 3 commits into from
Feb 25, 2020

Conversation

dero
Copy link
Contributor

@dero dero commented Feb 22, 2020

For some(†) legacy themes the top ad is dynamically moved to a better
position using JavaScript. This commit rewrites the funcionality
to use vanilla JavaScript instead of depending on jQuery.

† twentyseventeen, twentyfifteen, twentyfourteen

Changes proposed in this Pull Request:

  • Drop a small dependency on jQuery in favor of using vanilla JS.

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

  • This is an iteration on the existing WordAds module.

Testing instructions:

  • Go to WP Admin > Jetpack > Settings > Traffic.
  • Enable Ads and make sure the Top of each page is enabled.
  • Go to WP Admin > Appearance > Themes and enable one of the themes that use this functionality: twentyseventeen, twentyfifteen or twentyfourteen.
  • Go to the front-page of the site and observe the top ad being placed correctly.
  • Make sure there are no JS errors raised in the browser's dev tools.

Proposed changelog entry for your changes:

  • Replaces jQuery code in the Ads module by vanilla JS.

@dero
Insert the top ad using vanilla JS instead of jQuery
c05176e 
For some(†) legacy themes the top ad is dynamically moved to a better
position using JavaScript. This commit rewrites the funcionality
to use vanilla JavaScript instead of depending on jQuery.

† twentyseventeen, twentyfifteen, twentyfourteen
@dero dero added the [Status] Needs Review To request a review from Crew. Label will be renamed soon. label Feb 22, 2020
@dero dero requested a review from a team as a code owner February 22, 2020 13:35
@dero dero self-assigned this Feb 22, 2020
@jetpackbot
Copy link

jetpackbot commented Feb 22, 2020 •
edited
Loading

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: March 3, 2020.
Scheduled code freeze: February 25, 2020

Generated by 🚫 dangerJS against 2be199d

@kraftbj kraftbj added [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it [Focus] Performance [Feature] WordAds labels Feb 24, 2020
@jeherve jeherve added this to the 8.3 milestone Feb 24, 2020
kraftbj
kraftbj previously approved these changes Feb 24, 2020
View reviewed changes
Copy link
Contributor

@kraftbj kraftbj left a comment

Choose a reason for hiding this comment

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

Worked as advertised in FF/Chrome.

@kraftbj kraftbj added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review To request a review from Crew. Label will be renamed soon. labels Feb 24, 2020
dbspringer
dbspringer requested changes Feb 24, 2020
View reviewed changes
Copy link
Member

@dbspringer dbspringer left a comment •
edited
Loading

Choose a reason for hiding this comment

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

This generally seems to work (tested in Chrome/FF/Safari) except for twentyfourteen. Though it seems to be generating an error for the article:first selector. Does this method work if the selector is a pseudo class?

Screen Shot 2020-02-24 at 1 06 20 PM

Thanks for taking a look at this!

@kraftbj kraftbj added [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Feb 24, 2020
@jeherve jeherve removed this from the 8.3 milestone Feb 24, 2020
@dero
Copy link
Contributor Author

dero commented Feb 25, 2020

@dbspringer Good catch, thank you. I didn't notice it's not a valid CSS selector.

:first is a jQuery selector that selects only the first matched element. That's what querySelector does by default, so stripping it solves the issue and produces identical results in the vanilla JS implementation. Done in 96f8913.

@dero dero requested a review from dbspringer February 25, 2020 09:11
@jeherve jeherve added [Status] Needs Review To request a review from Crew. Label will be renamed soon. and removed [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! labels Feb 25, 2020
@jeherve jeherve added this to the 8.4 milestone Feb 25, 2020
@kraftbj
Copy link
Contributor

kraftbj commented Feb 25, 2020

With the extra line added at the end, I removed them and ran PHPCBF while we were in the file (and since the PR has been generally reviewed). Spinning up a JN and testing all indicated themes.

dbspringer
dbspringer approved these changes Feb 25, 2020
View reviewed changes
Copy link
Member

@dbspringer dbspringer left a comment

Choose a reason for hiding this comment

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

Looks good, thanks for the quick fix 👍

@dbspringer dbspringer removed the [Status] Needs Review To request a review from Crew. Label will be renamed soon. label Feb 25, 2020
@dbspringer dbspringer added the [Status] Ready to Merge Go ahead, you can push that green button! label Feb 25, 2020
kraftbj
kraftbj approved these changes Feb 25, 2020
View reviewed changes
Copy link
Contributor

@kraftbj kraftbj left a comment

Choose a reason for hiding this comment

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

I concur with the fine gentleman from San Diego (@dbspringer) and with this stamp, mark the PR as approved. Let it be merged.

@kraftbj kraftbj merged commit 4a46d48 into master Feb 25, 2020
@kraftbj kraftbj deleted the update/wordads-vanilla-js branch February 25, 2020 17:35
@kraftbj kraftbj added [Status] Needs Changelog and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Feb 25, 2020
@dbspringer dbspringer restored the update/wordads-vanilla-js branch March 5, 2020 19:07
jeherve added a commit that referenced this pull request Mar 23, 2020
@jeherve
Changelog: add #14770
12ec5a6
jeherve added a commit that referenced this pull request Mar 31, 2020
@jeherve
8.4 Changelog (#15075)
0eb0b35 
* Initial changelog entry

* Changelog: add #14904

* Changelog: add #14910

* Changelog: add #14913

* Changelog: add #14916

* Changelog: add #14922

* Changelog: add #14924

* Changelog: add #14925

* Changelog: add #14928

* Changelog: add #14840

* Changelog: add #14841

* Changelog: add #14842

* Changelog: add #14826

* Changelog: add #14835

* Changelog: add #14859

* Changelog: add #14884

* Changelog: add #14888

* Changelog: add #14817

* Changelog: add #14814

* Changelog: add #14819

* Changelog;: add #14797

* Changelog: add #14798

* Changelog: add #14802

* Changelog: add #13676

* Changelog: add #13744

* Changelog: add #13777

* Changelog: add #14446

* Changelog: add #14739

* Changelog: add #14770

* Changelog: add #14784

* Changelog: add #14897

* Changelog: add #14898

* Changelog: add #14968

* Changelog: add #14985

* Changelog: add #15044

* Changelog: add #15052

* Update to remove Podcast since it remains in Beta

* Changelog: add #14803

* Changelog: add #15028

* Changelog: add #15065

* Changelog:add #14886

* Changelog: add #15118

* Changelog: add #14990

* Changelog: add #14528

* Changelog: add #15120

* Changelog: add #15126

* Changelog: add #15049

* Chanegelog: add #14852

* Changelog: add #15090

* Changelog: add #15138

* Changelog: add #15124

* Changelog:add #15055

* Changelog: add #15017

* Changelog: add #15109

* Changelog: add #15145

* Changelog:add #15096

* Changelog:add #15153

* Changelog: add #15133

* Changelog: add #14960

* Changelog: add #15127

* Changelog: add #15056

* Copy current changelog to changelog archive.

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

@kraftbj kraftbj kraftbj approved these changes

@dbspringer dbspringer dbspringer approved these changes

Assignees

@dero dero

Labels
[Feature] WordAds [Focus] Performance [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it
Projects
None yet
Milestone
8.4
Development

Successfully merging this pull request may close these issues.
5 participants
@dero @jetpackbot @kraftbj @dbspringer @jeherve