Skip to content

Conversation

@dero
Copy link
Contributor

@dero dero commented Mar 6, 2020

The Milestone widget relies on the widget_id being exposed by the before_widget argument passed to register_sidebar in the theme. That is not guaranteed and the widget's live countdown is currently broken in many themes, most prominently in the current twentytwenty.

Changes proposed in this Pull Request:

  • Create a custom ID value derived from widget_id and assign it to every widgets' .milestone-content container.
  • In JS, target the newly assigned ID instead of the unreliable widget_id.

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

  • This fixes a bug in the Milestone widget.

Testing instructions:

  • On master, add the Milestone widget to one of the twentytwenty's Footer sidebars.
  • Set the Milestone date and time to one minute in the future.
  • On the front-end, open the browser's Dev tools and observe XHR requests being sent every second. (Aside: This feels like a DDoS waiting to happen, but I'll take a look into this separately.)
  • Observe the countdown to not update.
  • Switch to this branch.
  • Refresh the page and set the Milestone time and date to one minute in the future again.
  • On the front-end, observe the countdown being updated live.

Proposed changelog entry for your changes:

  • Fix the live countdown feature of the Milestone widget in some themes.

@dero dero added [Type] Bug When a feature is broken and / or not performing as intended [Feature] Extra Sidebar Widgets [Status] Needs Review This PR is ready for review. labels Mar 6, 2020
@dero dero requested a review from a team March 6, 2020 14:05
@jetpackbot
Copy link
Collaborator

jetpackbot commented Mar 6, 2020

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 9944ec0

@jeherve jeherve added this to the 8.4 milestone Mar 11, 2020
@jeherve jeherve added [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. and removed [Status] Needs Review This PR is ready for review. labels Mar 17, 2020
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.

Do you think you could rebase that one now that we've merged #14864?

Thanks!

@dero dero force-pushed the fix/milestone-widget-id branch from 88b976b to 9944ec0 Compare March 17, 2020 13:54
@dero
Copy link
Contributor Author

dero commented Mar 17, 2020

@jeherve Sure, rebased in 9944ec0.

@jeherve jeherve added [Status] Needs Review This PR is ready for review. and removed [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. labels Mar 17, 2020
@jeherve jeherve added [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review This PR is ready for review. [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. labels Mar 17, 2020
@jeherve jeherve merged commit 656ad9c into master Mar 17, 2020
@jeherve jeherve deleted the fix/milestone-widget-id branch March 17, 2020 16:32
@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 added a commit that referenced this pull request Mar 31, 2020
* 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

Labels

[Feature] Extra Sidebar Widgets [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.

5 participants