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

Fix logic for choosing between customizer and widgets.php #13858

Merged
merged 2 commits into from
Dec 2, 2019
Merged

Fix logic for choosing between customizer and widgets.php #13858

merged 2 commits into from
Dec 2, 2019

Conversation

chickenn00dle
Copy link
Contributor

This commit changes the logic for determining whether we are in the
customizer or widgets.php. Previously, when accessing widgets from
widgets.php, the logic failed to recognize this when the page
initialized with no widgets.

This ensures the correct element is selected, even when there are no
active plugins when the page loads.

Fixes #12302

Changes proposed in this Pull Request:

  • removes an additional check for widget-control-actions class when initializing the widgets_shell variable
  • instead check that widgets_shell is not undefined and empty before resorting to customize-controls ID

Testing instructions:

From #12302 (comment)

  • Go to the widgets page, remove all widgets from the widget area and refresh the page (imp step).
  • Add a widget, hit visibility button, it doesn't work
  • Refresh the page, hit visibility button, it works!

Proposed changelog entry for your changes:

  • Fixes bug in widget visibility where visibility button did not work when widgets.php loaded with no active plugins

@chickenn00dle chickenn00dle added [Feature] Widget Visibility [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Oct 28, 2019
@chickenn00dle chickenn00dle requested a review from a team October 28, 2019 17:36
@chickenn00dle chickenn00dle self-assigned this Oct 28, 2019
@matticbot
Copy link
Contributor

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

@jetpackbot
Copy link

jetpackbot commented Oct 28, 2019 •
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: December 3, 2019.
Scheduled code freeze: November 26, 2019

Generated by 🚫 dangerJS against 5a6c21f

@chickenn00dle chickenn00dle mentioned this pull request Oct 28, 2019
Closed
@jeherve jeherve added this to the 8.0 milestone Oct 28, 2019
@jeherve
Copy link
Member

jeherve commented Nov 19, 2019

Noting that this was originally added to fix an issue on WordPress.com. See 6714-wpcom for more details.

That said, applying your patch on WordPress.com does not seem to break things. 👍

@jeherve jeherve self-assigned this Nov 19, 2019
@jeherve jeherve added the [Type] Bug When a feature is broken and / or not performing as intended label Nov 19, 2019
jeherve
jeherve requested changes Nov 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.

Could you bump the version of that file as it is enqueued here?

jetpack/modules/widget-visibility/widget-conditions.php

Line 34 in 89a9af9

20171227,

That will help ensure that everyone gets the updated version of the file loading on their site after the update.

Thank you!

@jeherve jeherve 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] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Nov 19, 2019
@jeherve jeherve removed this from the 8.0 milestone Nov 25, 2019
@chickenn00dle
Fix logic for choosing customizer or widgets.php
37d8799 
This commit changes the logic for determining whether we are in the
customizer or widgets.php. Previously, when accessing widgets from
widgets.php, the logic failed to recognize this when the page
initialized with no widgets.

This change removes the additional check for .widget-control-actions
when initializing the widgets_shell variable. Instead we
make sure the variable is not undefined, and if it is not undefined, we
make sure it is not empty.

This ensures the correct element is selected, even when there are no
initial active plugins.

See #12302 for details.
@chickenn00dle
Bumps version in widget admin setup
5a6c21f 
Bumps version number in widget_admin_setup function to ensure everyone
gets updated file after plugin update.

See #13858 (review)
@matticbot
Copy link
Contributor

chickenn00dle, Your synced wpcom patch D34621-code has been updated.

@chickenn00dle
Copy link
Contributor Author

Apologies for the delay on this @jeherve!

I've bumped the version number :)

@chickenn00dle chickenn00dle added [Status] Needs Review To request a review from fellow Jetpack developers. 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 Nov 28, 2019
@jeherve jeherve added this to the 8.1 milestone Dec 2, 2019
@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 2, 2019
jeherve
jeherve approved these changes Dec 2, 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.

This tests well. Merging.

@jeherve jeherve merged commit 984e44c into Automattic:master Dec 2, 2019
@matticbot matticbot added [Status] Needs Changelog and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Dec 2, 2019
jeherve added a commit that referenced this pull request Dec 13, 2019
@jeherve
Changelog: add #13858
3777892
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 <zinigor@gmail.com>
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 <zinigor@gmail.com>
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

@jeherve jeherve

@chickenn00dle chickenn00dle

Labels
[Feature] Widget Visibility Touches WP.com Files [Type] Bug When a feature is broken and / or not performing as intended
Projects
None yet
Milestone
8.1
Development

Successfully merging this pull request may close these issues.

Widget visibility: Button doesn't work when admin screen loads with no widgets
4 participants
@chickenn00dle @matticbot @jetpackbot @jeherve