Join GitHub today
GitHub is home to over 36 million developers working together to host and review code, manage projects, and build software together.Sign up
Use button block appender on group block #14943
Adds the button appender from #14241 to the group block.
How has this been tested?
Types of changes
New feature (non-breaking change which adds functionality)
requested review from
Apr 12, 2019
I plan to write some tests for this.
Some initial observations:
Oh that's interesting indeed. Here's what I see:
I recall us very recently removing the background color on this one, cc: @kjellr, maybe that wasn't a great idea.
Okay, made a patch — I can push to this branch if you like, but didn't want to commandeer it because the change I'm proposing is a little opinionated, so feel free to try it out first, otherwise let me know and I'll push:
Now it looks like this:
or this in a dark theme:
This actually changes the background color of the generic appender to be way more opaque everywhere it's used. But this is necessary since it can show up on any color background, I feel (another Kjell sanity check would be good here).
It also adds additional margins to the appender when seen in context of the
@jasmussen Yep, I think that's the right move. We'd probably want to annotate those new background fill variables to make it clear why they exist.
I have some incredibly minor adjustments to the color values to recommend. I think these match the placeholder backgrounds just a little more than what you have currently:
Also, I think your patch has the new colors incorrectly swapped. Based on the code comments in
referenced this pull request
Apr 12, 2019
Just following up on the comments:
@aduth - I definitely agree with this. It's not only a problem with the appender, it's also hard to discern which blocks are within a group at times.
I wondered if we could do something with the border on the left of blocks—when any inner block is selected use the left hand border to show the range of the group.
It could probably be a bit subtler.
I wrote a comment to this effect but it appears to have been lost on this page. Somehow Github's comment system is confusing :)
In the comment, I noted that the primary problem this generic appender aims to solve is to make it obvious that you have now inserted a group block. If it's just empty, with an empty appender inside it, it's not at all clear what, if anything, you've done. The big appender button solves that nicely.
However that's also only the setup state for the block. As soon as a group block has content, IMO it doesn't need to have that appender UI anymore. In fact doing so only makes it confusing to the user why this block behaves differently to every other block. Which is to say: when a block has nested content, the way you add more content is to press Enter, or use the sibling inserter.
Another aspect is that if there are too many ways to insert content, in this case too many (+) icons, it'll just overwhelm the user and confuse. So I think we should treat the generic appender similarly to how we treat the placeholder state of an image: it's only there when the block is first created.
Yep, I've definitely noticed that.
@jasmussen Would the idea be that the default/paragraph appender is used instead after the first block is inserted?
Sort of, yes. Basically I imagine the behavior to be the same as it is in any other current nested block, like Media & Text or Columns. I.e. in nested contexts, there isn't actually any trailing appender unless the last block is a not-text-block:
Thanks @jasmussen, I've pushed some changes to the branch. The way it works now is:
Let me know if that seems the right behaviour.
Love it, thank you, this is a vast improvement. Here's a GIF:
This GIF also highlights a small challenge with this approach. But that challenge is not specific to this branch, and therefore something we can look at separately. But still perhaps something to think about. It's this:
Even though there's only a image placeholder in the group, the fact that the empty appender sits below means there's empty space where it feels like there shouldn't be.
This is already the case with the columns block too:
And even Media & Text — the button in the right column is supposed to be vertically centered but is offset by the empty appender:
The solution here is, separate from this branch (and to be clear, I think it's now good to go
A couple of strawman solution ideas:
I like option 1 because it is how editors traditionally work: the way you insert a linebreak is you select the place you want a linebreak after, and press Enter. In this case, you'd select the image and presse Enter, and you'd have your empty paragraph. Perhaps combined with #10051 this would make for a nice separate iteration.
But it's also worth noting that this suggestion has been controversial in the past, because while this is an editor, it is also a "block editor", and users don't always assume that the behavior they're used to from traditional editors will work here. But much of that controversy was mitigated by improvements to the sibling inserter, which will ALSO work here. So I think it may be worth looking at.
Thanks for the help @jasmussen—hopefully this is good to go then pending a code review.
I like that it wasn't particularly complicated to implement in the Group block. There might still be room for improvement code-wise around how this kind of appender can be implemented for block builders, but I think this is ok as a first step.
Just a note that I'll be away on vacation until the end of the month (I probably should have handed this over earlier, sorry about that).
Apr 30, 2019
1 check passed
This was referenced
Apr 30, 2019
referenced this pull request
May 8, 2019
The appender indicator is a much welcome addition. An improvement to it would be to be the ability to drag a block into the enclosing element and have it take the appenders place. As it is you still need to add a random block before you can drag in another block.
Also this highlights the need for a way to multi select and drag several block to another location, apart from the copy/cut/paste option. Include option/drag here for several selected blocks to duplicate to another location. Probably a separate PR.
Thanks @irishetcher. The fact that you can't drop into the area when the appender is present is a bug, and is fixed in #15702. There's also discussion there around allowing people to drop directly onto the appender itself. Hopefully we'll see a PR for that soon.