-
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
Add notice of disabled blocks to block inserter when no results are found #17338
Conversation
I just tested this and it works great, @brentswisher! Thanks for helping encourage positive feedback for WP users! I am curious to as to how this might tie in with @melchoyce's work on the Block Directory. Mel, I know you're designing for a state when no blocks are found, and I'm not sure if there are any conflicts here. |
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.
Agreed, nice UI improvement. I left some minor code comments. Overall, it looks great.
} | ||
<br /> | ||
<Button | ||
isButton |
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.
There is no „isButton” prop in „Button” component.
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.
🤦♂ haha, I suppose not, removed. (I was trying to get it styled like a button and tried that before I added isDefault and forgot to remove it)
<Button | ||
isButton | ||
isDefault | ||
className="editor-inserter__block-manager-button block-editor-inserter__block-manager-button" |
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.
There should be only one class name here, the obe starting with „block-editor”. The other one exists only for backward compatibility in other places.
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.
Fixed, thanks! I knew one was older but wasn't sure if I was supposed to still include it or not.
@@ -438,12 +469,15 @@ export default compose( | |||
} | |||
} | |||
const destinationRootBlockName = getBlockName( destinationRootClientId ); | |||
const hiddenBlockTypes = getPreference( 'hiddenBlockTypes' ); | |||
const numberOfHiddenBlocks = isArray( hiddenBlockTypes ) && hiddenBlockTypes.length; |
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 const will return „false” when „hiddenBlockTypes” is not an array. Is there any case where it would have other type?
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.
I am not overly familiar with getPreference
and this is how it was done in the block manager, so I did it here as well. In looking into it further though, hiddenBlockTypes
is defaulted to []
and it may not be necessary.
My thought/assumption at the time of adding it was if the block-editor is used in another context in the future, that preference may not exist, and the isArray() would prevent hiddenBlockTypes.length from erroring in that case, but that doesn't look like it is accurate.
Pinging @jorgefilipecosta as it looks like he added the message to the block manager and may have some insight as to a reason it is needed, otherwise I can remove it here.
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.
Hi @brentswisher, normally when we have a list of blocks, boolean false means no blocks and true means all blocks, that the case we followed for allowed blocks.
Andrew original block manager code used as size lodash function https://github.com/WordPress/gutenberg//blob/4698c304a19d81c7f27f8c49e9cf9c4babd7309f/packages/edit-post/src/editor.js#L59
The tests for using hiddenBlockTypes don't account for booleans so I'm not sure if in this case we are following the all/none convention but writing the code I preferred to play on the safe side, also as referred it may avoid a case where the preference because of some reason does not exists.
I guess I should have also just used size and it would cleaner, here I think we can also use size.
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.
Great, thanks for the feedback, I've updated this to use size and it works well. @gziolo I think that should address all of the issues if you wouldn't mind taking a second look?
I wonder about the functionality of disabling a block to begin with. Maybe disabling a block should hide it from the default view, but still show it when searching. Perhaps this defeats the purpose, but worth a discussion. |
While this PR offers a nice little button to open the Block Manager, it would be great to include the graphical element from this PR over here: #17326 Let's get these two PRs talking to one another. I think this would be a good first step. Then we can look at expanding this to include Mel's more advanced features of actually showing the blocks that are disabled in the Inserter. |
29e10d2
to
617dd60
Compare
@mapk @melchoyce - I'm thinking it might be best to get this merged and then try to iterate? I personally won't have a lot of time the next few weeks to spend with Gutenberg and I would like to get a notification of some kind into Wordpress 5.3. Then when I have some more time to commit to it I can revisit the more advanced approach? |
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.
Alrighty, this is good. It's approved from a design perspective. Let's get it merged.
@@ -429,6 +456,9 @@ export default compose( | |||
const { | |||
getChildBlockNames, | |||
} = select( 'core/blocks' ); | |||
const { | |||
getPreference, | |||
} = select( 'core/edit-post' ); |
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 inserter is a generic component from the block-editor
package. It can't use a selector from the core/edit-post package. I wonder if the hiddenBlocks are available in the getSettings
selector of the block-editor
store instead.
@@ -457,6 +490,10 @@ export default compose( | |||
__experimentalFetchReusableBlocks: fetchReusableBlocks, | |||
} = dispatch( 'core/editor' ); | |||
|
|||
const { | |||
openModal, | |||
} = dispatch( 'core/edit-post' ); |
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.
Same here, we're not allowed to use actions from the core/edit-post
module in this package.
Closing as this is stale. |
Fixes #17235.
(Also potentially fixes part of #15121)
Description
If blocks are disabled when a search in the inserter comes up empty, it will now display a message with the number of disabled blocks and include a button that will open the block manager. If no blocks are disabled, the existing messaging will be used.
How has this been tested?
Open the block manager and disable some blocks.
Open the inserter, type until no matching blocks are found, see new messaging.
Open Block manager and re-enable all blocks
Open the inserter, type until no matching blocks are found, see existing messaging.
Screenshots
Types of changes
New feature (non-breaking change which adds functionality)
Checklist: