Fix blue line indicator not showing at the end #25849
Conversation
kevin940726
added
[Type] Bug
kevin940726
requested review from
ajitbohra,
ellatrix,
nerrad,
ntwb,
talldan and
youknowriad
as code owners
|
Size Change: +139 B (0%) Total Size: 1.18 MB
ℹ️ View Unchanged
|
kevin940726
requested review from
tellthemachines,
noisysocks,
adamziel and
draganescu
packages/block-editor/src/components/block-list-appender/index.js
Outdated
Show resolved
Hide resolved
| // Can't find the element, might be at the end of the block list, or inside an empty block list. | ||
| // We instead try to find the "Appender" and place the indicator above it. | ||
| // `rootClientId` could be null or undefined when there's no parent block, we normalize it to an empty string. | ||
| return appenderMap.get( rootClientId || '' ); |
There was a problem hiding this comment.
Thinking of some different approaches,getBlockDOMNode just does a getElementById call, could it be an option to do something similar for the appender using querySelector? e.g
document.querySelector( `#block-${ rootClientId } .block-list-appender` );There was a problem hiding this comment.
That's actually my first attempt. However, I just don't feel like it's "react"ive enough as we'll be relying on the DOM structure and classnames.
There was a problem hiding this comment.
I think we should go for the simpler solution. We're doing the same for block nodes, and this should be revised together if we'd like to avoid querying the DOM.
There was a problem hiding this comment.
It's not as simple as just a querySelector call though. Since clientId could be null, also could rootClientId. We have to add some more code to be able to select the parent of the appender.
kevin940726
force-pushed
the
update/fix-insertion-blue-line-at-end
branch
from
219c7cf to
c314041
Compare
| // Can't find the element, might be at the end of the block list, or inside an empty block list. | ||
| // We instead try to find the "Appender" and place the indicator above it. | ||
| // `rootClientId` could be null or undefined when there's no parent block, we normalize it to an empty string. | ||
| return appenderNodesMap.get( rootClientId || '' ); |
There was a problem hiding this comment.
The insertion point is added at the root of the whole tree of blocks, so I think rootClientId will always be '', never an id?
There was a problem hiding this comment.
In other words, does the indicator work for nested appenders?
There was a problem hiding this comment.
It could be an id if it's in the Widget Editor though, widget-area is kind of like a special case for this 😜.
Also, <InsertionPoint> only handles getting of the appender nodes. <BlockList> is where setting happens, which can be in <InnerBlocks> or widget-area. What I'm trying to say here is that a <InsertionPoint> could have multiple <BlockList> hence could also have multiple <BlockListAppender>.
There was a problem hiding this comment.
In other words, does the indicator work for nested appenders?
It should, at least for my testing below.
|
From what I am seeing in the Post Editor. Creating a new post, adding a heading and adding a paragraph block. Having the second to last block active when clicking the Inserter and hovering over a block will show a line below the second to last block. IF I have the last block active no blue line will be seen below that. The second to last block is selected. One sees the blue line below. The last block is selected. One can not see the blue line below the text. Widget screen. Current 9.1. Which has the blue line error. This PR. Another test. Having Footer 2 open. Closing footer 1. From what I see the blue line only shows up if the first widget area is open. Closing the first widget area and having in this example Footer 2 open will not show a blue line in the top of footer 2. |
|
@paaljoachim I can't seem to reproduce those 🙀, see my screenshots/GIFs in the PR description above.
The blue line should be at the bottom of the footer 2 though. See #25708 for more info. It's likely that the blue line is at the bottom of the widgets and since the viewport doesn't auto-scroll to it it's not visible. |
|
@kevin940726 Kevin |
This reverts commit e376eab.
|
@ellatrix I got a bit trigger happy :) if you have objections to merging this we can revert! |
|
It's ok, we can adjust it later. |
This PR is based on #25727 to make sure e2e passes. It should be rebased to master once #25727 merged.
Description
Fix #25785, related to #25727. Also fixes #25785 (comment).
Fix the blue line indicator is not showing at the end when using the global inserter to try to insert a block in a post or in a widget area.
This is more complicated than I thought, I couldn't think of any better way to do this, please feel free to give me some suggestions and guidance.
Internally, I use a
AppenderContextto store aMapto keep the Appender's references. Then we can inject the blue line indicator alongside the appender, which usually lives at the end of the block list.How has this been tested?
Run the e2e test, or...
The same steps can also be tested in the widget editor.
Screenshots
Post editor
Widget editor
Types of changes
Bug fix
Checklist: