-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Hide inserter pattern/media panel when there are no categories #66246
Conversation
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
Size Change: -525 B (-0.03%) Total Size: 1.77 MB
ℹ️ View Unchanged
|
I think backporting #65611 will be a better UX and fix overall. |
useEffect( () => { | ||
onHasCategories( !! categories.length ); | ||
}, [ categories, onHasCategories ] ); |
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.
The onHasCategories
prop will have different references on each render, which makes it unsafe to pass it to the effect.
I think the only reason we're not getting an infinite loop is that the state is a boolean value.
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.
Since we're close to the release, I took the liberty to fix this. I also changed the function name to just setHasCategories
to better convey what it does. I hope it's okay for you, @talldan 🙇 !
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 for me 💯.
(I'm not a fan of using useEffect
like this but until we have a better option I think this will work for 6.7)
I agree that the “hack” is fine for this release, bit let’s create an issue to follow up on this. |
Should we still open an issue if the code here only targets 6.7? Would a comment perhaps be more discoverable? 🤔 |
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 is a good patch for 6.7. It avoids broken UI when no results are found.
What?
Fixes part of #65833
Pairs with the fix in #66214
Hides the slide-out pattern/media inserter panel when there are no categories.
How?
The code here is a little ropey, since we need a child component to control part of its parent. The
InserterMenu
component becomes wider whenever the additional panel slides out, but only the childBlockPatternsTab
andBlockMediaTab
components know whether there are actually any categories to be displayed (and patterns or media to be displayed in the slide out panel).I understand it's not great, but it's a fix only targeted at wp/6.7. In trunk, patterns are shown all the time because of #65611, which wasn't backported (an alternative could be to backport it). So this code won't live on in the codebase.
Testing Instructions
In trunk: A weird empty state is shown
In this PR: The slide out panel slides back in and the inserter shows 'No results'
Screenshots or screencast
Before
After