Skip to content
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

Fix issue with inconsistent nesting appender #16453

Merged
merged 1 commit into from Jul 23, 2019

Conversation

jasmussen
Copy link
Contributor

Fixes #16221.

This PR intends to fix a visual regression that happened as a result of some consistency work done to the trailing appender.

The trailing appender is the one that sits at the end of the document to allow you to easily add new content. It usually sits after any block that isn't a paragraph. The consistency work brought this to nesting containers as well. But the side effect is that if you have a single list block inside a group block, the appender is permanently visible all the time, even if probably it shouldn't be. The net result is that the unselected editing canvas is no longer representative of the end result.

This PR changes that, so the trailing appender is only visible when a parent or adjacent block is selected. It's not the be-all end-all solution, as it might make sense to look into more holistic changes to when the appender is visible. But it does fix the immediate visual regression.

Here's a list block inside a group block, before:

listbefore

Here's after:

list after

Here's a columns block with an image, before:

colsbefore

Here's after:

colsafter

I intentionally left in a ToDo that can either be removed once #14961 is merged, or in a future patch.

Fixes #16221.

This PR intends to fix a visual regression that happened as a result of some consistency work done to the trailing appender.

The trailing appender is the one that sits at the end of the document to allow you to easily add new content. It usually sits after any block that isn't a paragraph. The consistency work brought this to nesting containers as well. But the side effect is that if you have a single list block inside a group block, the appender is permanently visible all the time, even if probably it shouldn't be. The net result is that the unselected editing canvas is no longer representative of the end result.

This PR changes that, so the trailing appender is only visible when a parent or adjacent block is selected. It's not the be-all end-all solution, as it might make sense to look into more holistic changes to when the appender is visible. But it does fix the immediate visual regression.
@jasmussen jasmussen added [Type] Bug An existing feature does not function as intended [Type] Regression Related to a regression in the latest release labels Jul 8, 2019
@jasmussen jasmussen self-assigned this Jul 8, 2019
@youknowriad
Copy link
Contributor

This PR improves things but I feel personally that this appender might not be the best choice for nesting context. I wonder if we should just show the Button Appender instead (if the columns block is selected)

@jasmussen
Copy link
Contributor Author

I think that's a fair suggestions, though I would suggest it's a separate consideration. Whether the generic appender or the text based one, imo you need to be able to see an accurate preview of the end result, when blocks are deselected. The notable exception being when a nesting container is empty, then the appender should be visible even when the block is unselected.

@mapk
Copy link
Contributor

mapk commented Jul 10, 2019

imo you need to be able to see an accurate preview of the end result, when blocks are deselected.

I think this is key. I really like having these appenders disappear when deselected. Thanks for catching this fix!

In regards to @youknowriad's comment above, the simple + icon does begin to feel arbitrarily placed among blocks when several of them exist on screen ( see screenshot below). While this PR helps resolve this, I'm still a bit confused on the rhyme and reason for when these little + icons appear and they don't always give me a good sense for where the content is going to go exactly. I believe the appender button would help this, but that can be explored in another PR. This is a great fix as is.

Screen Shot 2019-07-10 at 3 23 37 PM

@mapk
Copy link
Contributor

mapk commented Jul 17, 2019

I just ran into this problem again, and now can't wait for this to get merged!!!

Below, I'm using a Group block to create a background colored section with a Quote block in the middle. I wanted extra padding above and below the quote, so I added a Spacer block to provide this. But because the appender + icon is consistently showing it's adding extra visual padding below the quote. At first, I wasn't quite sure what was going on, so I adjusted the Spacer block below the quote to account for this, but when I viewed the frontend, my adjustment resulted in less padding below the quote than above. Let's hide these + icons when the block isn't selected. It wasn't until later that I realized it's the appender + icon causing the visual extra padding in the editor.

Screen Shot 2019-07-17 at 12 46 12 PM

@draganescu
Copy link
Contributor

draganescu commented Jul 19, 2019

I am completely in this boat. We have so many artefacts on the screen (like the recently used blocks, the plusses all over the place, extra empty paragraphs) that it is very hard to be sure how things are going to look.

Copy link
Contributor

@draganescu draganescu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested locally and it works great for both the group block and the columns block. One thing I noticed is that the selector for the apender is now more specific and if in a block you have:

.wp-block-navigation-menu .block-editor-block-list__layout .block-list-appender {
	display: none;
}

to make you own custom inserter, it won't work after this is applied.
However @jasmussen I don't know if this is a problem so I am approving this for the upside! :)

@jasmussen
Copy link
Contributor Author

Thanks all for looking. I'm afk for a couple of weeks, so if you'd like this merged feel free to do so in my absence 😉

@draganescu draganescu merged commit 1ca616a into master Jul 23, 2019
@github-actions github-actions bot added this to the Gutenberg 6.2 milestone Jul 23, 2019
@youknowriad youknowriad deleted the try/fix-appender-visibility branch July 23, 2019 10:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Bug An existing feature does not function as intended [Type] Regression Related to a regression in the latest release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Always visible appender is inconsistently visible, causing visual offsets
4 participants