Show insertion point after the last block in a container #28418
Merged
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
youknowriad
added
[Type] Bug
|
Size Change: -21 kB (-2%) Total Size: 1.37 MB
ℹ️ View Unchanged
|
|
Thanks! Would probably review it next week. Could you also help to uncomment the tests we commented out before? Just want to make sure we pass the tests 😉. |
I think the test might have been refactored entirely, can't seem to find them. |
|
nevermind, I was looking at the wrong test :) |
|
Seems like this test (https://github.com/WordPress/gutenberg/pull/28418/checks?check_run_id=1760742282) is failing for whatever reason 🤔, could you please take a look :) ? |
|
@kevin940726 It should be fixed now. |
|
In #25849, there's a fix to show the indicator in between two blocks when the former one is selected (See the screenshot in the PR). When trying this in this PR, it seems like it's not working anymore (seems intermittently). Any idea why? Is this intentional? Kapture.2021-01-26.at.10.21.46.mp4Also, perhaps we should start writing some e2e tests for the insertion indicator to avoid regressions in the future. |
|
I think the bug here is not in the current PR. It seems the issue is the fact that the insertion point in the state (reducer) is actually not "reset" properly when you open the inserter a second time. I believe this might be related to the fix @noisysocks or @talldan worked on to fix the position of the insertion after clicking "Browse All". I'm not really sure I understand the logic of that fix properly but it seems we're missing a reset at some point. |
kevin940726
approved these changes
Jan 27, 2021
|
It's great to see this work coming along. I was curious to see what the inserter button looked like in cases where the default block appender is already showing (at the ends of some block lists) and that is a bit funny but I found something else that is probably of greater concern. When using the inserter button that appears at the end of a block list, a newly inserted block gets inserted based on the location of the selected block instead of the inserter button. I made a recording with three uses of the same inserter button and three different places where a block gets inserted. None of them the expected position: insertion-after-selected.mp4 |
|
good catch @stokesman I think it's fixed now. |
|
The failing e2e tests here are a Core regression tracked here https://core.trac.wordpress.org/ticket/50025 |
youknowriad
changed the title
This was referenced Jan 28, 2021
Merged
|
@youknowriad testing #28382 i got this weird behavior with the inserter not being removed. Can you think of anything from this PR that may be the cause, so I can open an informed issue? ghost-inserter.mp4 |
|
@draganescu I don't know really on top of my mind, the interactions here are a bit complex and I didn't test nested menus in popovers. Please do track this. |
This was referenced Feb 10, 2021
7 tasks
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.
follow-up to #27860 (merge in the same changelog entry)
closes #28381
After #27860 the insertion point (in between inserter) only shows up between two blocks due to a complex positioning logic (center it properly between blocks both vertically and horizontally), this had the side effect of removing the possibility to show at the end of the block list (for instance by selecting the last block, opening the inserter and hovering a block type).
This PR solves that by considering a potential nullable next element.
There was also another issue where the hovering a block type in the inserter was showing the wrong insertion point indicator.