Skip to content

Widget Visibility - Fix broken dropdowns after updating to 4.7#6600

Merged
dereksmart merged 1 commit intoAutomattic:masterfrom
stoyan0v:fix/widget-visibility/broken-post-type-dropdowns
Mar 8, 2017
Merged

Widget Visibility - Fix broken dropdowns after updating to 4.7#6600
dereksmart merged 1 commit intoAutomattic:masterfrom
stoyan0v:fix/widget-visibility/broken-post-type-dropdowns

Conversation

@stoyan0v
Copy link
Copy Markdown
Contributor

@stoyan0v stoyan0v commented Mar 8, 2017 •
edited
Loading

Fixes #6596 #6595

Changes proposed in this Pull Request:

  • Fix broken dropdowns after updating to 4.7

Since "Post Type" option has been removed in 4.7, there is an js error when you open the widget visibility after updating from 4.6 to 4.7. The majorData variable is empty and it prevents other conditions to be shown. This PR adds a check that the major rule is not post_type and additional check that widget_conditions_data has the property, before we make anything.

visibility-options-error

Testing instructions:

  • Install Jetpack 4.6
  • Add a widget to any sidebar
  • Add a Post Type rule in the beginning
  • Add other rule/rules ( it doesn't matter which one )
  • Update to Jetpack 4.7
  • Check that all other rules except the "Post type" one are displayed ( similar to the following image )
    visibility-options

zinigor
zinigor suggested changes Mar 8, 2017
View reviewed changes
Copy link
Copy Markdown
Contributor

@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.

Thanks for the great effort, I have added two comments, can you please address them?

modules/widget-visibility/widget-conditions/widget-conditions.js Outdated
Copy link
Copy Markdown
Contributor

@zinigor zinigor Mar 8, 2017

Choose a reason for hiding this comment

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

Can you please modify the condition to be Yoda-style 'post_type' ===. Also, let's add a comment before this condition that it's done for backwards compatibility in order to avoid errors when people have "Post Type" selected?

Copy link
Copy Markdown
Contributor Author

@stoyan0v stoyan0v Mar 8, 2017

Choose a reason for hiding this comment

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

I've added a comment and changed the condition to Yoda-style. Thank you!

modules/widget-visibility/widget-conditions/widget-conditions.js Outdated
Copy link
Copy Markdown
Contributor

@zinigor zinigor Mar 8, 2017

Choose a reason for hiding this comment

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

I'd say that the change below is not needed because this can only be false when major is post_type, and we have already cut that flow out with the previous condition. Let me know if I'm not seeing something.

Copy link
Copy Markdown
Contributor Author

@stoyan0v stoyan0v Mar 8, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Do you think it will be better if we remove the first condition and leave this one? It may prevent similar issues in the future if there are such, while the first one will handle post_type only ( the second one seems more "global" ). I wasn't sure which one is better that's why added both.

Copy link
Copy Markdown
Contributor

@zinigor zinigor Mar 8, 2017

Choose a reason for hiding this comment

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

Yeah, makes sense, but I'd rather not have a huge wrapping condition like this, because we don't remove major rules that often. It has only happened once so far afaik.

Copy link
Copy Markdown
Contributor Author

@stoyan0v stoyan0v Mar 8, 2017

Choose a reason for hiding this comment

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

Makes sense, thanks for clarification 😄

@zinigor zinigor added [Feature] Widget Visibility [Pri] High [Status] In Progress Bug When a feature is broken and / or not performing as intended labels Mar 8, 2017
@zinigor zinigor mentioned this pull request Mar 8, 2017
Closed
@stoyan0v stoyan0v force-pushed the fix/widget-visibility/broken-post-type-dropdowns branch from 325da5c to c1a423e Compare March 8, 2017 08:47
@zinigor zinigor added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] In Progress labels Mar 8, 2017
@jeherve jeherve added this to the 4.7.1 milestone Mar 8, 2017
@dereksmart
Copy link
Copy Markdown
Contributor

dereksmart commented Mar 8, 2017

👍 thanks!

@dereksmart dereksmart merged commit 3883bc8 into Automattic:master Mar 8, 2017
@dereksmart dereksmart removed the [Status] Ready to Merge Go ahead, you can push that green button! label Mar 8, 2017
dereksmart pushed a commit that referenced this pull request Mar 8, 2017
@stoyan0v @dereksmart
Widget Visibility - Fix broken dropdowns after updating to 4.7 (#6600)
175c260
@dereksmart
Copy link
Copy Markdown
Contributor

dereksmart commented Mar 8, 2017

branch-4.7 175c260

jeherve added a commit that referenced this pull request Mar 9, 2017
@jeherve
Changelog: add #6600
2b706e6
dereksmart pushed a commit that referenced this pull request Mar 9, 2017
@jeherve @dereksmart
Jetpack 4.7.1: Changelog. (#6612)
f144a81 
* Update stable tag.

* Move 4.7 changelog to changelog.txt

* Changelog: add #6571

* Changelog: add #6600

* Changelog: add #6604

* Changelog: add #6608

* Changelog: remove PR numbers and add release date and link.

* Changelog: add #6615
dereksmart pushed a commit that referenced this pull request Mar 9, 2017
@jeherve @dereksmart
Jetpack 4.7.1: Changelog. (#6612)
c973333 
* Update stable tag.

* Move 4.7 changelog to changelog.txt

* Changelog: add #6571

* Changelog: add #6600

* Changelog: add #6604

* Changelog: add #6608

* Changelog: remove PR numbers and add release date and link.

* Changelog: add #6615
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@zinigor zinigor zinigor approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Bug When a feature is broken and / or not performing as intended [Feature] Widget Visibility [Pri] High

Projects

None yet

Milestone

4.7.1

Development

Successfully merging this pull request may close these issues.

4 participants

@stoyan0v @dereksmart @zinigor @jeherve