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

Widgets: Migrate front-end jQuery code to vanilla JS #14910

Merged
merged 2 commits into from
Mar 17, 2020
Merged

Widgets: Migrate front-end jQuery code to vanilla JS #14910

merged 2 commits into from
Mar 17, 2020

Conversation

dero
Copy link
Contributor

@dero dero commented Mar 6, 2020

These pieces of the codebase have been reviewed:

  • /modules/widget-visibility
  • /modulels/widgets/*

These widgets are using jQuery in some capacity:

  • social-icons: In the admin context only.
  • simple-payments: In the admin context only.
  • contact-info: In the admin context only.
  • search: Ordering + filters functionality on the front-end + admin context.
  • eu-cookie-law: In the admin context only. (Frontend handled by @kienstra in https://github.com/Automattic/jetpack/pull/14753/files)
  • milestone: Dynamic countdown on the front-end + admin context.
  • gravatar-profile: In the admin context only.
  • gallery: FE + admin context, but not addressed, because the widget is not available in WP 4.9+ (superseded by the core gallery widget).
  • rsslinks: In the admin context only.
  • customizer-utils: In the admin context only.
  • twitter-timeline: In the admin context only.
  • `widget-visibility: feature: In the admin context only.

The two widgets in bold have been converted to vanilla JS. Others are either not relevant anymore (gallery) or use jQuery in the admin context only, which was not in scope for this PR.

It doesn't seem like a good use of development time to rewrite the admin jQuery scripts because jQuery is arguably going to remain to be loaded in the admin context for a long time. However if this assumption is incorrect and we want to fully strip all jQuery code, please let me know.

Fixes #14864

Changes proposed in this Pull Request:

  • Rewrite the FE jQuery code to vanilla JS in the search and milestone widgets.

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

  • This is an update to the existing extra Jetpack widgets.

Testing instructions:

  • Go to WP Admin > Jetpack > Settings > Writing.
  • Enable "Make extra widgets available for use on your site including subscription forms and Twitter streams".
  • Add the Search (Jetpack) and Milestone (Jetpack) widgets to the website.
  • Test the added plugins
    • Search widget: Make sure that when the "Sort by" select box value is changed, the page is automatically refreshed and new sorting rules are applied.
    • Search widget: Make sure the category filters that appear after any initial search work as intended.
    • Milestone widget: Set the milestone to happen in the next minute.
    • Milestone widget: On the front-end, observe the live countdown, i.e. the seconds are updated live and when the timer hits 0, the milestone message is displayed.
      • Note: Please be aware of an existing issue with twentytwenty (and potentially other themes) and pick a there that exposes the widget ID. Failing to do that will lead to the widget not updating on the front-end.

Tested in:

  • Chrome 80 (Ubuntu).
  • IE 11 (Win 10).

Proposed changelog entry for your changes:

  • Rewritten the front-end jQuery code of the Search and Milestone widgets into vanilla JS.

@dero dero added [Feature] Extra Sidebar Widgets [Focus] Performance [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Mar 6, 2020
@dero dero requested a review from a team March 6, 2020 15:47
@jetpackbot
Copy link

jetpackbot commented Mar 6, 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: April 7, 2020.
Scheduled code freeze: March 31, 2020

Generated by 🚫 dangerJS against 6e50664

@dero dero force-pushed the update/widgets-vanilla-js-fe branch from 568ea08 to 6e50664 Compare March 6, 2020 15:54
@dero
Copy link
Contributor Author

dero commented Mar 6, 2020

@kienstra Could you please review this when you have a moment?

kienstra
kienstra approved these changes Mar 10, 2020
View reviewed changes
Copy link
Contributor

@kienstra kienstra 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!

Hi @dero,
This looks really good.

The only point I made isn't related to this diff.

modules/widgets/milestone/milestone.js
var Milestone = function( args ) {
var $widget = $( '#' + args.id ),
var widget = document.getElementById( args.id ),
id = args.id,
refresh = args.refresh * 1000;

Copy link
Contributor

Choose a reason for hiding this comment

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

This isn't about your work, but a general comment about the widget. This diff didn't cause the issue.

What do you think about exiting here if ! widget ?

if ( ! widget ) {
        return;
}

It looks like in Twenty Twenty, widgets don't have an id:

widget-ids-here

...so the var widget = document.getElementById( args.id ) will be null.

Though maybe this isn't a problem in other themes. For example, Twenty Sixteen works fine:

ids-widget

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kienstra You're absolutely right, Ryan, and I've noticed that issue as well and opened a separate #14904 a couple days ago - didn't want to mix a bug fix with optimization in one PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, nice! You're ahead of the game, sorry for missing that issue.

modules/widgets/milestone/milestone.js
success: function( result ) {
$widget.find( '.milestone-countdown' ).replaceWith( result.message );
refresh = result.refresh * 1000;
xhr.onload = function() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice use of xhr instead of $.ajax, this worked well when stepping through this in the browser.

modules/widgets/search.php
jetpackSearchModuleSorting();
} else {
document.addEventListener( 'DOMContentLoaded', jetpackSearchModuleSorting );
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice alternative to jQuery( document ).ready()

modules/widgets/search.php
orderBy.value = values[0];
order.value = values[1];

form.submit();
Copy link
Contributor

Choose a reason for hiding this comment

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

This select works as expected:

form-do

@jeherve jeherve added this to the 8.4 milestone Mar 11, 2020
leogermani
leogermani approved these changes Mar 16, 2020
View reviewed changes
Copy link
Contributor

@leogermani leogermani left a comment

Choose a reason for hiding this comment

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

PR Looks good to me!

I've tested all the scenarios described and everything worked as expected. I also looked at the code. All good.

I've opened #15009 to suggest an improvement to the widget. It's not related to this PR, but I found this out while testing it.

@matticbot
Copy link
Contributor

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

@jeherve jeherve self-assigned this Mar 17, 2020
@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 Mar 17, 2020
jeherve
jeherve approved these changes Mar 17, 2020
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.

This tests well for me. Merging.

@jeherve jeherve merged commit 726d10b into master Mar 17, 2020
@jeherve jeherve deleted the update/widgets-vanilla-js-fe branch March 17, 2020 12:04
@matticbot matticbot added [Status] Needs Changelog and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Mar 17, 2020
jeherve added a commit that referenced this pull request Mar 20, 2020
@jeherve
Changelog: add #14910
2d4df84
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

@kienstra kienstra kienstra approved these changes

@leogermani leogermani leogermani approved these changes

@jeherve jeherve jeherve approved these changes

Assignees

@jeherve jeherve

Labels
[Feature] Extra Sidebar Widgets [Focus] Performance Touches WP.com Files
Projects
None yet
Milestone
8.4
Development

Successfully merging this pull request may close these issues.

Extra Widgets: Update to not require jQuery
6 participants
@dero @jetpackbot @matticbot @jeherve @leogermani @kienstra