-
Notifications
You must be signed in to change notification settings - Fork 4.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Try: One appender at a time. #30833
Try: One appender at a time. #30833
Conversation
Size Change: +66 B (0%) Total Size: 1.47 MB
ℹ️ View Unchanged
|
display: none; | ||
} | ||
|
||
.block-editor-block-list__block.is-selected .block-list-appender { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If tests prove this is the behavior we want, this should be easy to enforce in JS (not render it at all) unless the block is selected.
Awesome to see this being worked on — its a very pesky and finicky problem to solve, but will greatly improve the flow for adding blocks. However, I'm not sure its working as intended. For example, the empty Column appender is now very confusing; You can only see one Column appender at a time, and if you unselect the Columns block the empty Columns are impossible to see/select. Additionally, there's still the ever-present "default" appender which still seems to appear at all times. This might be a separate problem to solve, though: |
Thank you for testing. Yes, that won't do. I'll take a look at your surfaced use case and see if I can make a fix!
A separate one, yes, but an important one. |
f7c0bbf
to
cdf2a9b
Compare
Alright, I changed the thing the CSS targets. It now only targets the "toggle", that is the black 24x24 square button, never the wide white one. That should accomplish what we're after, I think. |
One scenario I spotted that doesn't seem to work is when you have one template part nested inside another: Here I've selected the Navigation block inside my Menu template part, which is inside both my Header and Footer template parts. As you can see two [ + ] buttons are visible – one for the selected Navigation block, and one for the Menu TP. I'm wondering if we could be more aggressive here, and only reveal the [ + ] when the container is selected? That would ensure only one is visible, and potentially tidy up the UI even more. |
I think I caught that edgecase. It looks like changing the "display" is not a good approach, as the display of this appender is likely needing to change from just block, depending on context. In the TP case, it was inline-flex, which overrode the display none. For now I've tried a transform combined with a visibility, but it might end up needing to go the approach Riad suggests, by doing it programmatically. So I suppose this can serve more as a proof of concept? |
Still seeing the double appender here I'm afraid 🙈 |
118c941
to
4464ead
Compare
I think that last one should do it. But I'm starting to think of this more as a proof of concept, as the CSS isn't that reusable or obvious. |
I feel like #31886 proves that there is an opportunity to rethink how the appender works, but it's one that goes beyond what's in this branch. So closing it in favor of that. |
Description
Especially in the site editor which has many nesting contexts, you will often encounter situations where you see a great deal of black plusses at the same time. This is because any selected nesting container gets a plus appender, and they stay visible even as you drill down the hierarchy.
Perhaps not the best example, it can get much worse, but observe how when I select the header template part in the following, the black plus for inserting both into the template part, and inside the navigation block, are visible at the same time:
This is disruptive because it's not clear where either one inserts. This PR (which is round 2 of #29559) tries a relatively simpler system; only show the appender if the block is selected:
This needs a great deal of edgecase testing, but from what I can tell, this serves as a small improvement that might get us slightly further along with #26404.
Checklist:
*.native.js
files for terms that need renaming or removal).