Fix filtering for downloadable blocks in inserter. #55243
Conversation
tellthemachines
added
[Type] Bug
|
Size Change: +1.02 kB (0%) Total Size: 1.65 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
Oh, I like this idea in principle! Once a block is installed, you likely don't need to see the "Available to install" UI 👍
One potential wrinkle is that when installing a block, it now hides the area as soon as the block is installed, which could feel a bit jumpy. With a single block it looks kinda okay:
2023-10-11.16.11.10.mp4
However, if the list is fairly long or if the entered string to install the block doesn't quite match the target block, it's possible to wind up in a state where the installed block doesn't appear to be visible anywhere in the list, and focus appears to now be nowhere in particular:
2023-10-11.16.20.09.mp4
A side note on the above is that in that example the results aren't very helpful based on the search term, but that's a separate issue! 😅
If so, we could add logic to check the installed blocks against allowedBlockTypes instead.
Given the potential jumpiness during the installation flow, maybe checking against allowedBlockTypes would make the UI feel a bit more stable there?
|
Thanks for testing @andrewserong !
I've fixed this to check if the block has just been installed, in which case we should show it. I also did a bit more digging into the reason for the initial logic and found #25926, which indicates that A possible solution is to try and differentiate between the allow list and the disallow list scenarios (disallow list should be what happens when we hide blocks in preferences) On the other hand, should it be possible to insert custom blocks targeted specifically at blocks such as Navigation? I half remember this being discussed at some point but I'm not sure it was ever worked on. |
|
Oh, interesting problem!
If so, I imagine that'd be the responsibility of the plugin to figure out, rather than for us to handle in the logic here? Given that #25926 landed a long time ago now, I suppose it might be worth seeing if we can honour the intentions behind that PR. I guess what I mean is, if a plugin decides to override core blocks' allow lists, then I think it'd probably be fine for that behaviour to happen within the plugin, rather than need to account for it in this block directory listing... I don't feel strongly about it, though! 🙂
Is one challenge there that the disallow list happens over in the post editor store, rather than with the block directory package? All that said, personally I don't mind the approach of this PR. Would it be worth getting a second opinion from some of the folks who worked on #25926 originally, to get a better sense of desired behaviour nowadays? |
|
Apologies, I meant to add — the latest commit tests very well for me according to the testing instructions, and resolves the issue of the installed state disappearing. The UI for the installed state is working nicely for the rest of the current session in the editor, and after a reload, if you search for a block, you just see that block rather than the listing for the plugin: 
So, all up, this LGTM if we feel confident that it's okay that we no longer do the check for allowed blocks! |
That's why I added @ryelle to the reviewers 😄 though perhaps @StevenDufresne might have thoughts on this too? I dunno, it's definitely not a great experience to try to install/add a custom block to, say, the Navigation block. On one hand there aren't that many blocks that limit the types of blocks that can be added without just inserting one block type automatically, so this not-so-great experience may not be as common as trying to install blocks in the post editor while also having hidden blocks. If that's the case, this change should be OK. I've been looking for how to tell that the allow list comes from a parent block instead of the post editor, but there doesn't seem to be a way of doing so. The only alternative I can think of is checking the block type of the root client id, which should be undefined if we're at root level in the post editor. This would only allow us to fix the issue of installing new blocks at root level though - it still wouldn't work within a Group block. |
Good point... I suppose one of the reasons I don't mind this PR so much is that a user can install the block via the plugin, and it (mostly) gracefully falls back to installing, but not inserting the block where it can't go. For example, if I have a child block of the Navigation block selected when I go to add MathML, the block gets installed, it just doesn't get inserted anywhere: 2023-10-12.14.42.49.mp4If we go with this PR, one tiny improvement there could be to update the snackbar notice if it wasn't able to insert so that it just says |
|
Thanks for the feedback folks! I'll let it sit for a day or two to give others a chance to say something. This isn't for 6.4 so no huge rush 😄 |
|
@tellthemachines I can't add any more context that isn't shared in the two issues linked in the PR (#22240 & #24148). Regarding the issue of allowed lists and inserting blocks, I don't know how likely it is. When this was written we weren't sure how the block directory would be used since there were ideas of only accepting single block plugins (which could lead to separate child block plugins). I would run through the test case referenced in #24148 for safety. Short of that, you are now the expert 🥇 :) |
Thanks for the tip @StevenDufresne ! The test case in that PR is working correctly on this branch, so should be good from that perspective.
Hmm, I can't see any obvious way of doing this given that we're unable to tell whether the block is allowed or not, and I don't think there's a way to tell whether it has been successfully added to the page. I'm inclined to merge this as is and revisit/iterate based on any feedback that comes in afterwards. |
Me, too. It's quite a niche / edge case situation, so a plus +1 from me for merging and updating further down the track if anyone winds up finding it confusing. |
What?
Fixes #55209.
Changes how the
downloadableItemspassed toDownloadableBlocksPanelare calculated so that if blocks are hidden in the post editor a list of downloadable blocks still shows.The problem was we were using
canInsertBlockTypeto check if the downloadable blocks could be used, but that function only checks against a list of blocks that are already installed. A block that's not installed won't be on the list, so it would always return false.The reason this logic magically worked if there were no blocks hidden in the editor is that
allowedBlockTypesis a boolean if there are no disallowed block types, but a list if there are 😅I changed the logic to filter out blocks already installed, because I noticed installed blocks were still appearing in the downloadable blocks results and if a block is installed but hidden, we don't want it appearing there. I may be missing something here though - is there any reason we might want installed blocks to still appear on the list? If so, we could add logic to check the installed blocks against
allowedBlockTypesinstead.Testing Instructions
With MathML block installed and hidden:
before
after