-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
[edit-widgets] Improve the UX of wp_inactive_widgets on the experimental screen #24790
Conversation
Size Change: +29 B (0%) Total Size: 1.16 MB
ℹ️ View Unchanged
|
@@ -370,6 +371,10 @@ public function prepare_item_for_response( $raw_sidebar, $request ) { | |||
$sidebar['status'] = 'inactive'; | |||
} | |||
|
|||
if ( 'wp_inactive_widgets' === $sidebar['id'] && empty( $sidebar['name'] ) ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe make 'wp_inactive_widgets' === $sidebar['id'] && empty( $sidebar['name'] )
into another is
variable $is_inactive_widgets_sidebar
just to make it like $is_registered_sidebar
... not important.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would have to be just $is_inactive_widgets_sidebar = 'wp_inactive_widgets' === $sidebar['id']
, the name may or may not be empty. It would be a tad more readable, but since that line appears only once per method and there is no duplication, the benefits are arguable - I'd say it's a matter of taste.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested and LGTM!
Would be nice to have a test on this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This works well. I agree that we should add a PHPUnit test. Can we please also move the inactive widgets to the bottom of the new screen and have it collapsed by default?
9f13149
to
3362413
Compare
3362413
to
bb75163
Compare
Description
At the moment, there is an empty area at the top of the Block Areas screen which has no title. It is the inactive widgets area. Widgets in that area are not being correctly stored/returned by the API and they are lost after leaving the page. After applying this PR, the interaction in question works properly:
Before:
After:
Related comment: #24290 (review)
How has this been tested?
Types of changes
Bug fix (non-breaking change which fixes an issue)
Checklist: