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.

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

Remove unused JS logic in email sharing #6339

Merged
merged 2 commits into from
Feb 10, 2017
Merged

Remove unused JS logic in email sharing #6339

merged 2 commits into from
Feb 10, 2017

Conversation

kasparsd
Copy link
Contributor

@kasparsd kasparsd commented Feb 9, 2017

Fixes #1223

Changes proposed in this Pull Request:

  • Remove the JS that was previously removing a dummy input field value for spam protection.
  • Print inline scripts that depend on jQuery after WP core is done with print_footer_scripts(). We are not using wp_add_inline_script() because all the display_footer() methods echo instead of return their output.

Testing instructions:

  • Share a post via email. Ensure that the email was sent.

@jeherve jeherve added [Feature] Sharing Post sharing, sharing buttons [Pri] High [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. [Type] Bug When a feature is broken and / or not performing as intended labels Feb 9, 2017
Copy link

@grappler grappler left a comment

Choose a reason for hiding this comment

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

Thank you @kasparsd for created the PR. The changes make sense.

Copy link
Member

@zinigor zinigor 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, sharing via email works fine, no more JavaScript error when enqueueing jQuery in the footer.

@zinigor
Copy link
Member

zinigor commented Feb 10, 2017

To test you need to make jQuery load in the footer, it can be done using a filter like this:

function wpse_173601_enqueue_scripts() {
    $wp_scripts = wp_scripts();
    $wp_scripts->add_data( 'jquery', 'group', 1 );
    $wp_scripts->add_data( 'jquery-core', 'group', 1 );
    $wp_scripts->add_data( 'jquery-migrate', 'group', 1 );
}
add_action( 'wp_enqueue_scripts', 'wpse_173601_enqueue_scripts', 20 );

Keep in mind that in order for jQuery to remain in the footer, it has to have no dependent scripts loading in the header. One such script is the related posts JS, which you may have to disable or enqueue in the footer as well.

@zinigor zinigor 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 Feb 10, 2017
@dereksmart dereksmart merged commit bba03ce into Automattic:master Feb 10, 2017
@dereksmart dereksmart added [Status] Needs Changelog and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Feb 10, 2017
@jeherve jeherve added the [Status] Needs Testing We need to add this change to the testing call for this month's release label Feb 10, 2017
jeherve added a commit that referenced this pull request Feb 21, 2017
dereksmart pushed a commit that referenced this pull request Feb 28, 2017
* Changelog: update stable tag and move changelog to changelog.txt

Also remove old releases from readme.txt to keep the changelog tab short.

* Changelog: add #5883

Also update the filter's docblock to match new version.

* Changelog: add #5938

* Changelog: add #6298

* Changelog: add #3405

* Changelog: add #5941

* Changelog: add #6239

* Changelog: add #6281

* Changelog: add #6303

* Changelog: add #6018

* Changelog: add #6300

* Changelog: add #6296

* Changelog: add #6130

* Changelog: add #6292

* Readme: remove extra "on".

* Changelog: add #6307

* Changelog: add #3297

* Changelog: add #6275

* Changelog: add #6321

* Changelog: add #6297

* Readme: update the support forum link anchor.

Anchor changed when WordPress.org forums were updated to bbPress 2

* Readme: update list of a12s, it wasn't up to date anymore!

* Changelog: add #6338

* Changelog: add #6337

* Changelog: add #6335

* Changelog: add #6333

* Testing List: first version of the 4.7 testing list.

* Changelog: add #6332

* Changelog: add #6325

* Changelog: add #6326

* Changelog: add #6339

* Changelog: add #6342

* Changelog: add #6343

* Changelog: add #6346

* Changelog: add #6347

* Changelog: add #6279

* Changelog: add #6306

* Changelog: add #6312

* Changelog: add #6316

* Changelog: add #6171

* Changelog: add #6317

* Changelog: add #6246

* Changelog: add #6263

* Changelog: add #4220

* Changelog: add #5888

* Changelog: add #3406

* Changelog: add #3637

* Changelog: add #6320

* Changelog: add #5992

* Changelog: add #6322

* Changelog: add #6324

* Changelog: add #6352

* Changelog: add #6355

* Changelog: add #6360

* Changelog: add #6362

* Changelog: add #6369, #6382

* Changelog: add #6370

* Changelog: add #6375

* Changelog: add #6383

* Changelog: add #6384

* Changelog: add #6386

* Changelog: add #6395

* Changelog: add #6403

* Changelog: add #6406

* Changelog: add #6418

* Changelog: add #6419

* Changelog: add #6434

* Changelog: add #6446

* Changelog: add #6006

* Changelog: add #6096

* Changelog: add #6399

* Changelog: fix typo.

@see #6331 (comment)

* Changelog: add #6440

* Changelog: add #6443

* Changelog: add #6445

* Changelog: add #6463

* Changelog: add #6468

* Changelog: add #6471

* Changelog: add #6474

* Changelog: add #6480

* Changelog: add #6497

* Changelog: add #6499

* Changelog: add #6514

* Changelog: add #6267

* Changelog: add #5940

* Changelog: add #6492

* Changelog: add #5281

* Changelog: add #6327

* Changelog: add #6451

* Changelog: add #6525

* Changelog: add #6530
jeherve added a commit that referenced this pull request Mar 13, 2017
Fixes #6640
Reverts changes introduced in #6339

`$service->display_footer()` includes calls to `js_dialog` when not using Jetpack's Official sharing buttons.
`js_dialog` often uses `wp_add_inline_script()`, thus recreating the issues described in #6640.
zinigor added a commit that referenced this pull request Mar 14, 2017
eliorivero added a commit that referenced this pull request Mar 14, 2017
Reverted part of the changes introduced in #6339.
@jeherve jeherve removed the [Status] Needs Testing We need to add this change to the testing call for this month's release label May 23, 2017
jeherve added a commit that referenced this pull request Nov 13, 2017
This JS was previously removed in #6339, but then added back (I think during a rebase) in #6332.
oskosk pushed a commit that referenced this pull request Nov 23, 2017
This JS was previously removed in #6339, but then added back (I think during a rebase) in #6332.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Sharing Post sharing, sharing buttons [Pri] Normal Touches WP.com Files [Type] Bug When a feature is broken and / or not performing as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants