Show all blocks all the times to avoid confusing UX #61873
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Size Change: +1.04 kB (+0.06%) Total Size: 1.74 MB
ℹ️ View Unchanged
|
draganescu
changed the title
draganescu
added
[Type] Enhancement
|
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. |
draganescu
force-pushed
the
try/show-all-blocks
branch
from
42b1a5a to
20806d8
Compare
jeryj
reviewed
May 23, 2024
packages/block-editor/src/components/inserter/hooks/use-insertion-point.js
Outdated
Show resolved
Hide resolved
draganescu
commented
May 23, 2024
packages/block-editor/src/components/inserter/hooks/use-insertion-point.js
Outdated
Show resolved
Hide resolved
This comment was marked as resolved.
This comment was marked as resolved.
draganescu
commented
May 24, 2024
packages/block-editor/src/components/inserter/hooks/use-block-types-state.js
Outdated
Show resolved
Hide resolved
draganescu
force-pushed
the
try/show-all-blocks
branch
from
10ba0a5 to
a2e6e11
Compare
draganescu
commented
May 24, 2024
packages/block-editor/src/components/inserter/hooks/use-insertion-point.js
Outdated
Show resolved
Hide resolved
This comment was marked as outdated.
This comment was marked as outdated.
|
@draganescu Maybe we can modify getInsertionPoint for our needs. It's also passed into the inserter already. It's always returning undefined for me at the moment, but maybe it'll be useful. |
…ee like top level
draganescu
force-pushed
the
try/show-all-blocks
branch
from
45061c2 to
23ae559
Compare
youknowriad
reviewed
May 29, 2024
| // template locked mode | ||
| const rootBlocks = select( blockEditorStore ).getBlocks(); | ||
| while ( availableItems.length === 0 ) { | ||
| for ( const block of rootBlocks ) { |
There was a problem hiding this comment.
This loop feels like it can be very bad perf wise in nested templates and things like that. (not sure how true though).
The other thing that feels odd, is that it will end up with a random client id and the behavior for the user can be confusing: where am I going to insert when I click that thing?
It feels like a better path, would be to try to find the "rootClientId" that we want to insert in before hand and update useBlockTypesState to take a rootClientId as an argument. Now that I'm writing that, it seems it already has a rootClientId, so it's a matter of extracting a "BlockList" component from the current BlockTab component and doing something like:
const { rootClientId, alternativeRootClientId } = useSelect(() => {
return {
rootClientId: getInsertionPoint().rootClientId,
alternativeRootClientId: findAlternativeRootClientId() // we can discuss implementation of this function separately.
}
}, [] );
<BlockList rootClientId={ rootClientID } />
<BlockList rootClientId={ alternativeRootClientId } />
There was a problem hiding this comment.
Yes by all means there a couple of risky loops but they're in place just to make things "work" and to show conceptual limitations we still have.
For example a loop later in the PR will make the action of inserting a block while a post content block is selected in a template locked edit mode and it shows that we can't tell where to insert without:
- bringing knowledge of template locked mode to the block editor package
- relying on something else in a higher level package finding "post content" and its client id and pass that down to lower levels
... both of which are kinda weird.
Separately, the theoretical findAlternativeRootClientId will also have to try a sort of heuristic since we don't have a clear picture of how to find the next best insertable spot.
Maybe the block list should advertise this in a separate precomputed part of the store? As in top down places to insert blocks if the next thing is unavailable ... or something.
There was a problem hiding this comment.
There are two separate discussions here:
- First; I don't think the way this PR is implemented is correct. It's just adding code to make things work as it want but without organizing things properly. This is for instance forcing us to have this loop in two places (find the right place).
- Second: how
findAlternativeRootClientIdshould be implemented. There are options: smart as the PR is trying to do (find the closest parent or something), or more controlled: (use the same wrapper the zoom-out would use for instance)
There was a problem hiding this comment.
I don't think the way this PR is implemented is correct. It's just adding code to make things work as it want but without organizing things properly
Yes, that's why I said: "by all means there a couple of risky loops but they're in place just to make things "work" and to show conceptual limitations we still have." and @jeryj said "We have gotten it to work, but it needs refactoring to be more stable and easier to maintain." 😂
There was a problem hiding this comment.
Yeah, sorry I misread the original reply :)
…oss inserter popover and useInsertionPoint
richtabor
reviewed
May 31, 2024
| return block | ||
| ? sprintf( | ||
| // translators: %s: Block title, e.g: "List", "Buttons", "Group" | ||
| __( '%s Block' ), |
There was a problem hiding this comment.
We don't need "Block" appended here. The block name itself will be enough context. "List", "Columns", "Buttons", etc.
|
UX wise, this feels pretty good. You don't end up hung up with no blocks to insert, like this:
|
|
Odd border when the allowed blocks is not necessary:
It may be fine to omit the border even. None of the other categories have one. |
|
Noticed that "Patterns" renders at the bottom as well:
|
ellatrix
reviewed
May 31, 2024
| ( item ) => item.category && item.category !== 'reusable' | ||
| ), | ||
| ( itemList ) => | ||
| itemList.reduce( ( acc, item ) => { |
There was a problem hiding this comment.
Why loop over this twice and add pipe if it can be done in a single loop?
ellatrix
reviewed
May 31, 2024
| const getRootBlockTitle = () => { | ||
| const blockName = getBlockName( rootClientId ); | ||
| // Find the title within the list of all possible blocks. | ||
| const block = allItems.find( ( item ) => item.name === blockName ); |
There was a problem hiding this comment.
Are we getting the block title here? Why not check the block type instead of looping through all blocks, like we normally do?
ellatrix
reviewed
May 31, 2024
| title={ | ||
| categoryItems.length === 1 | ||
| ? getRootBlockTitle() | ||
| : category.title |
There was a problem hiding this comment.
I don't understand this condition. Why do we fall back to the title of the parent of the selected block when there's 1 category?
There was a problem hiding this comment.
Ah, I got confused by categoryItems, I'm guessing this is just the total amount of items, not the amount of categories.
ellatrix
reviewed
May 31, 2024
| @@ -177,6 +218,30 @@ export function BlockTypesTab( | |||
| ); | |||
| } | |||
| ) } | |||
|
|
|||
| { ( items.length === 0 || | |||
| availableCategories.length === 1 ) && ( | |||
There was a problem hiding this comment.
Would be great to comment on this condition. Why show this when there's 1 category available and not 2?
There was a problem hiding this comment.
Why show this when there's 1 category available and not 2?
Pure convention 🤷🏻
ellatrix
reviewed
May 31, 2024
| ] | ||
| ); | ||
|
|
||
| const onToggleInsertionPoint = useCallback( | ||
| ( show ) => { | ||
| if ( show ) { | ||
| showInsertionPoint( destinationRootClientId, destinationIndex ); | ||
| showInsertionPoint( destinationRootClientId, destinationIndex, { | ||
| block: blockToInsert, |
There was a problem hiding this comment.
I assume we need to pass the block so that we can change the target if necessary. Can't be change destinationRootClientId and destinationIndex so we avoid modifying showInsertionPoint?
ellatrix
reviewed
May 31, 2024
| ); | ||
| if ( ! canInsertBlocks() && normalizedBlocks.length === 1 ) { | ||
| // If it's not possible to insert the block in the current location, try to find a new location up the tree | ||
| const rootBlocks = select.getBlocks(); |
There was a problem hiding this comment.
Why do we jump straight from the selected block to the root block instead of attempting to insert into parent blocks? Or am I missing something?
|
Where does this PR stand at the moment? What's the logic being implemented? Any thoughts on the proposal here? #61873 (comment) |
priethor
added
[Type] Bug
priethor
changed the title
|
Let's close this in favor of #62169 - it's the same idea overall but solves the spread of changes to a simpler location. Testing that PR it achieves the same result. It also improves upon our solution here around inserting when a template locked situation exists, by defaulting to the section root (nice!) as the place to insert into. It also solves the same problem for patterns with overrides. So it's a better continuation of the work here. @priethor heads up that #62169 is now the bug fix for #60991 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Explores #60991
Why?
Too often the inserter is empty, for instance when opening the inserter while a list block list item is selected or a button in a buttons block, the only available block is the list item , or the button respectively.
This is confusing because it's unclear why the other blocks are not available.
How?
Try to show all the blocks all the time and insert blocks in the nearest possible location.
Testing Instructions
Testing Instructions for Keyboard
N/A
Screenshots or screencast
CleanShot.2024-05-23.at.16.28.13.mp4
Known limitations