Try/block toolbar refactoring #9282
Conversation
|
I tested it out - the block settings menu feels really nice on the toolbar and I conceptually like the idea of the responsive toolbar items. On the UX, the toolbar feels quite cluttered when viewed like this, granted this might be an extreme case, but there's also a lot of empty space in the dropdown when the items don't tesselate nicely:
I'm not sure what the alternative is. I hacked around in the dev tools to see what it would look like with the items aligned right and the dropdown background removed, but I think now the dropdown items don't look like they have enough elevation over the toolbar:
You noted there might be bugs, so I won't focus on them, but I'll make a brief mention as a footnote just to make sure they're captured:
|
|
I think I would prefer if the toolbar was not always full-width. It seems like a waste to cover up all that space for blocks with short toolbars and especially blocks where the ellipsis is the only thing in the toolbar. I think the More Tools drop-down should be the minimum width possible when there are only a few controls in it.
There is no reason for that empty space to be there, and it feels a bit awkward. Maybe you should try just having a fade-out and scroll bar instead of an overflow menu? |
|
👏 😍 This is incredible. You get a medal! I think this is an incredibly solid first step, and I personally feel like this validates our ideas both of a responsive toolbar, and of popping off sections into the overflow menu as space becomes a premium. GIF:
What I like:
What we need to work out:
Of those three, I think 1 is the most important to work out. Given that you already have this sort of working, that feels like a point in the "collapse it all into a single dropdown" camp. There are a few kinks here and there to work out as well, but assuming that's just small things we can address as we go. In fact depending on the decisions, I can push some polish directly to this branch if you like. THIS IS AMAZING WORK :ovation: |
|
Thanks for putting this together! Some quick items:
From Google Sites: |
Making the "ResponsiveToolbar work" without being full-width might be tricky because it needs a fixed width to compute the remaining space. This is not a definitive "no" though, it needs testing.
Unfortunately as explained in #9075 (comment) I don't see how to implement separate collapsing because of the way we use Slots to render these toolbars. |
Note that it's fine if the toolbar is technically fullwidth — could we do that, but make the empty space on the right transparent so you can see the text below? |
Yes, that's what I'm going to try |
|
@jasmussen @youknowriad If the bar is technically fullwidth, you should still be able to click in the transparent spot to be able to select the previous block. Otherwise it would be confusing and annoying. I think the toolbar should be the width of all its controls until it reaches full-width, at which point the controls would overflow into either the chevron or ellipsis, depending on what is decided upon. Personally, I prefer the combined ellipsis menu, since it takes up the least amount of space. Here is an incredibly sloppy mockup of what I think a combined ellipsis menu and toolbar overflow would look like:
Notice how the toolbar sections wrap to the next line, and are divided by vertical lines, but no horizontal ones, keeping a relatively clean look. Alternatively, the one-toolbar-per-line idea could work as well, but that would take up more space in some cases. Of course, this wrapping/vertical-line-dividers idea could also be used in a tools-only chevron, if that is the direction taken. |
|
|
||
| const MyResponsiveToolbar = () => ( | ||
| <ResponsiveToolbar> | ||
| <IconButton icon="plus" /> |
| display: none; | ||
| } | ||
| `; | ||
| document.body.appendChild( styleNode ); |
There was a problem hiding this comment.
Good catch it should be
|
|
||
| window.cancelAnimationFrame( this.rafHandle ); | ||
| window[ handler ]( 'resize', this.throttledUpdateHiddenItems ); | ||
| window[ handler ]( 'scroll', this.throttledUpdateHiddenItems, true ); |
There was a problem hiding this comment.
Probably useless, this was copied from the Popover but it's a different use-case there
| const handler = isListening ? 'addEventListener' : 'removeEventListener'; | ||
|
|
||
| window.cancelAnimationFrame( this.rafHandle ); | ||
| window[ handler ]( 'resize', this.throttledUpdateHiddenItems ); |
There was a problem hiding this comment.
Should we use withGlobalEvents instead?
| } | ||
|
|
||
| componentDidUpdate( prevProps ) { | ||
| if ( prevProps.children !== this.props.children ) { |
There was a problem hiding this comment.
When will this ever be false?
| } | ||
| } ); | ||
|
|
||
| if ( countHiddenChildren !== this.state.countHiddenChildren ) { |
There was a problem hiding this comment.
Early return on inverse to avoid tabbing rest of function?
| childRect.left < containerRect.left || | ||
| childRect.right > containerRect.right - OFFSET | ||
| ) { | ||
| countHiddenChildren++; |
There was a problem hiding this comment.
Minor: This would make sense as a reduce, though I suppose childNodes is a NodeList, not Array, so not natively available without either using Lodash or converting to array [ ...childNodes ]:
const total = childNodes.reduce( ( result, child ) => {
// ...
}, 0 );| } | ||
|
|
||
| render() { | ||
| const defaultRenderToggle = ( { onToggle, isOpen } ) => ( |
There was a problem hiding this comment.
Why not a top-level constant variable? To avoid unnecessary garbage collection and React reconciliation.
| </div> | ||
| </Disabled> | ||
|
|
||
| { Children.map( children, ( child, index ) => { |
There was a problem hiding this comment.
Also, maybe it was an earlier approach you considered, but why not just not render the hidden children, rather than all the work to style them as hidden?
There was a problem hiding this comment.
Notice that we're not hiding children (components), we're hiding dom nodes. We can't work on components (children) because one child (a slot) can contain multiple dom nodes. It can even be a slot containing another slot in some cases.
There was a problem hiding this comment.
Why not
{ children }?
The idea is that I'm rendering the children in multiple places, I was wondering if React uses the same "instances" of components it it finds the same instance of element. But I guess it's probably not the case.
|
@matias the idea from #9282 (comment) is not ideal for accessibility. The "More Tools" button has all it needs for accessibility:
Instead, the idea of collapsing groups as vertical dropdowns:
doesn't address a11y and semantics in many ways. To start with, the button that toggles the dropdown has a double function: it works as a toggle to reveal the dropdown but it should also indicate the current setting state (and also set the related option?). There's no good way to communicate semantically this doubled functionality. |
|
Aside: so far, the only thing I see missing is arrows navigation in the "secondary" toolbar (not sure how we want to call it):
When using the keyboard, the main toolbar can be navigated with both the Tab and the Arrow keys. Insead, the secondary toolbar misses arrows navigation. |
youknowriad
force-pushed
the
try/block-toolbar-refactoring
branch
from
b67ffcf to
5b9271c
Compare
|
Pushed some updates here, I think it's better but there are still some issues:
The responsive toolbar works by showing its content on a hidden node and computing whether these nodes are inside the toolbar container or outside which means they should be moved to a dropdown menu, the issue when switching from block to block is that the content if this hidden doesn't seem to update properly consistently and I'm having a hard time understanding why. |
|
Thank you for still working on this. I think the most recent update definitely suggests a fork in the road. We either:
In the case of 1, the problem with this is that it is not an obvious place to look for formatting styles. The problem with 2 is, as I understand it, deeply technical in that it's somewhere between impossible and _very difficult. Correct? What we can't have, though, having multiple dropdowns. We can't put the More menu inside a dropdown menu, it has to be on its own. |
I agree with this and I think it's solvable with some custom props added to the ResponsiveToolbar component but it's not straightforward. If you think that would be enough for this PR to "work", I can go this road:
|
|
I think the iteration with a ellipsis always on the right and an otherwise "shrinking into the toolbar" approach could be an improvement over what we have today. But I'm also cognizant of the benefits of the other approach, and would defer to Matías and Tammie. Is there any way we could make the other approach work? I know I'm asking for the moon here. |
|
I would prefer a solution that keeps the toolbar thin when there are not a lot of controls. I don't like the idea of the toolbar always being full-width no matter how few controls a block has. I have no idea how difficult this is technically, but I would prefer it if it is possible. |
I believe in @youknowriad's ability to deliver moons :) |
|
Hello all. I made a separate issue here #9472 but thought I'd add my two cents here as well in case someone wanted to actually mock up my mockup. I've used GB for my last two projects and feel like the overall writing process could be improved on through rearranging the buttons and toolbars as I've mocked up below:
My suggestions include:
Let me know what you think! |
|
Having UI controls so far on the right is a problem for accessibility and doesn't meet the "proximity of controls" principles. Just think at users who use a screen magnifier with a high zoom level: they would have hard times to understand focus has gone on the far right, outside of their view field. |
|
I understand what you're saying; but I don't see how having icons on the far right affects usability. The UI controls on the far right are to edit the block's overall settings. The toolbar on the left is for editing the block's actual content. In terms of accessibility, I honestly don't have an actual screen reader or magnifier to test GB on....however I did try GB at 250% zoom in my browser and it looked fine. Button groups are only focused on if a user actively clicks, and if they can't see the right of the screen, they obviously can't see the update button etc either meaning they will have to scroll right anyway. As far as I know GB Is maxed out at 900ish pixels, and that makes it pretty good for making sure these icons are always in view whether they are right or left at all. |
|
@youknowriad would the following approach be doable in a somewhat simple way?
|
|
@jasmussen yes, this seems fine, the only downside is that it's not a generic enough solutions for third-party blocks. |
It would be if the third party blocks use the stock alignment segments, right? I think this would be a good starter approach that would let us get part of the way. |
|
We should also allow blocks to opt-in to a collapsed-by-default presentation. Thinking of cases where the alignment is less important than other tools (something like Table block, which has floats left / right so prominent). |
|
@jaclyntan Also, having the Remove Block button on the right of the ellipsis does not seem right. The ellipsis should appear after all other block controls.
Notably, blocks using the full alignment can be the full width of the screen, which puts the ellipsis literally as far away as it can get. On top of that, themes can change the content width (as well as the wide/full widths) in the editor to match the front-end, so the width could be wider or thinner than that. Personally, I think I prefer the up/down movers being on the left side of the block. The toolbar is aligned to the top left of the block, so having the movers near that corner feels nice visually and keeps all the controls close to each other. |
|
@ZebulanStanphill Perhaps inspiration could be taken from ACF and have a tooltip pop up before a block is actually deleted?
Whether the block is at full width or if it is contained doesn't really matter in my opinion. Having the ellipsis on the right when it is full width in my mind is actually a good thing because it means updating block settings and updating the page is overall quicker on large screens. Having the ellipsis on the far left on a huge screen means you have to constantly move the mouse diagonally over the screen...which is a lot of time wasted. The up/down movers on the left also feels awkward when you're rearranging multiple blocks then moving the cursor to the update/publish button. It might look nice visually but it's not really fun to use when you have to constantly change content. |
|
Some of these is already merged and some superseded by #9687 |
This is an experiment that does a lot of things (probably more than necessary) to the block toolbar
For testing purpose, I duplicated the alignment toolbar in the paragraph block to make it easier to test the ResponsiveToolbar behavior.
Also, this is still buggy especially in floated blocks and wide blocks, but it can be fixed, more a POC at the moment.
Related #9074 #9075