Always show the block appender when its parent is selected #36656
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] Enhancement
|
Size Change: -244 B (0%) Total Size: 1.1 MB
ℹ️ View Unchanged
|
|
Just for consistency, I agree: either we always show, or we never show. But I'd like very much for this one to be paired with 36605 so we don't have plusses everywhere. Thanks Riad! |
|
I agree that this one doesn't work well without #36605 |
youknowriad
requested review from
ajitbohra,
mkaz,
nerrad and
ntwb
as code owners
youknowriad
force-pushed
the
update/show-appender-when-parent-selected
branch
from
4e30d97
to
a3eb290
Compare
6 tasks
|
Glad this is finally being addressed. I tried doing something similar in #28529 but ran into some end-to-end test failures. I think this PR will finally resolve #5055. I'm noticing some odd behavior when clicking on the bottom of the editor canvas. To see what I mean, go into a mostly empty post, add a Paragraph, add an Image (just leave it as a placeholder), then click below the Image. The cursor jumps to the Paragraph for some reason. This is not how it behaves in |
|
@ZebulanStanphill That's probably a bug that's already in trunk but hidden. In trunk, there's an appender that is always visible after the last image block while in this PR, there's nothing, so the click is forwarded to the first block maybe? What would be your expectation here? |
|
@youknowriad I would expect the last block to be selected. The current behavior in |
Definitely. However I'm starting to think of that as a mostly failed experiment mostly resulting in empty trailing paragraphs, and difficulty in deselecting blocks that's increasing a bit with the site editor. I'm tempted to think it's worth revisiting/reverting that code, essentialy addressing #5803. |
|
@jasmussen I agree. Now that the root appender only shows when no blocks are selected, I found it confusing that clicking the empty bottom of the canvas didn't take my selection off the blocks, yet clicking the empty left/right regions did. |
youknowriad
force-pushed
the
update/show-appender-when-parent-selected
branch
from
a3eb290
to
8f90864
Compare
|
@ZebulanStanphill @jasmussen this makes sense, I tried it in the last commit and it feels good for me. WDYT |
|
I see this: The fact that I can deselect without creating an empty paragraph feels right. It also works well for groups: There's one single place in the entire codebase where I feel like the old behavior was better, and that's the navigation block for creating submenus. The details are in this PR which sort of restores the behavior, though that would break with this PR. I don't mean that to be a blocker: I think there are other paths we can take. But I do wonder: should there be some way the block itself could opt in to the "show appender even when a child is selected" behavior? It would make sense for submenus. |
It should already possible to do this somehow with the |
|
Sounds good! |
youknowriad
force-pushed
the
update/show-appender-when-parent-selected
branch
from
04ff359
to
7e19676
Compare
|
Oh one thing I noticed, and it's probably related to what you mentioned here:
While I think that could potentially work for the appender when it sits at the bottom of a post, it tripped me up in the initial state here:
Is it crucial for this PR to make this change at the same time? |
It's the same component, so when I change one, it changes the other, that said, we can try to introduce a special case if needed. Maybe check the content of the block editor and if it's empty create a paragraph. The reasoning for me here is that if I put myself in a screen reader mode, I feel like navigating the UI (hitting tab/shift tab) shouldn't actually do any action. That said I'd be fine with this small exception if needed. |
|
It's only the initial state where we see "Add title" and "Type / to insert a block" where it feels like it affects the writing flow. |
|
I added the special case. |
|
Works perfect 👌 |
youknowriad
force-pushed
the
update/show-appender-when-parent-selected
branch
from
1e8c5a6
to
e5f4041
Compare
|
Can I get a ✅ |
jasmussen
approved these changes
Nov 24, 2021
ellatrix
approved these changes
Nov 24, 2021
|
I'm hesitant about the "backport" label here. What do you think? |
I guess this change is mostly invisible on top of the already merged fixed appender change. In that light, whether it lands or not is less important? |
30 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.
closes #5055
closes #5803
Builds on top of #36605
This PR simplifies the behavior the default block appender:
This basically removes the special case we had in place about the last paragraph in a block list being a paragraph.
Let me know what you think @jasmussen