Add drag handle to block toolbar #24852
Conversation
talldan added [Type] Regression 
Size Change: +3 kB (0%) Total Size: 1.17 MB
ℹ️ View Unchanged
|
ZebulanStanphill added the [Focus] Accessibility (a11y) ZebulanStanphill added the Needs Accessibility Feedback 
| <FlexItem> | ||
| <BlockIcon icon={ handle } /> | ||
| </FlexItem> |
There was a problem hiding this comment.
What's the purpose of moving this?
There was a problem hiding this comment.
So that the block icon and drag handle are in the same order on the chip as they are on the toolbar.
It didn't feel right the other way around.
There was a problem hiding this comment.
Fair enough.
Come to think of it... why do we even show the handle icon on the drag chip? It's already pretty apparent that the user is dragging once you start, well, doing it. It seems kind of redundant to me.
| <ToolbarGroup> | ||
| <ToolbarItem | ||
| onFocus={ this.onFocus } | ||
| onBlur={ this.onBlur } | ||
| > | ||
| { ( itemProps ) => ( | ||
| <BlockMoverUpButton | ||
| clientIds={ clientIds } | ||
| { ...itemProps } | ||
| /> | ||
| ) } | ||
| </ToolbarItem> | ||
| <ToolbarItem | ||
| onFocus={ this.onFocus } | ||
| onBlur={ this.onBlur } | ||
| > | ||
| { ( itemProps ) => ( | ||
| <BlockMoverDownButton | ||
| clientIds={ clientIds } | ||
| { ...itemProps } | ||
| /> | ||
| ) } | ||
| </ToolbarItem> | ||
| <ToolbarGroup className="block-editor-block-mover__move-button-container"> |
There was a problem hiding this comment.
Is it semantically okay to nest a ToolbarGroup inside of another ToolbarGroup? One could argue the up/down buttons and the drag handle belong on the same level, though of course this complicates the styling somewhat (unless you're willing to remove all special case styling that the mover buttons currently have, in which case the styling becomes really simple).
There was a problem hiding this comment.
This CSS suggests it was at least intended to work that way: https://github.com/WordPress/gutenberg/blob/master/packages/components/src/toolbar-group/style.scss#L9
Thanks for the PR. The problem with this is that it does not solve the problem with drag and drop in top toolbar mode, and it makes the toolbar wider. I would avoid us doing this, and instead tackle drag and drop with a different approach. I will make a mockup monday that I believe can address this. |
Connecting with issue #24898, if we are adding the handle even provisionally while working on the larger aspect of dragging the block itself, we should ensure we improve upon the grouping of the elements, which regressed in some ways in 5.5 compared to the original design. |
Waiting for Monday, worth considering that ideally the "drag handle" should be available also when the editor is in "navigation mode". See screenshot below: how to drag and drop boxes when the block toolbar is not displayed?
[Edit] I see this is being explored in #24092 |
Mockup that adds handle added here: #24898 (comment) Mockup that improves select mode with drag and drop here: #24092 (comment) |
I took the liberty of pushing tweaks to this branch that makes the handle match the mockup. GIF:
There's a fair bit of grid refactoring of the toolbar that needs to happen as part of #24898, but in the interim this PR visually matches. |
Why the drag handle is an icon with 6 dots and changes to an icon with 2 lines while dragging? I'd keep the 6 dots one also while dragging, unless it changes to communicate a different meaning? |
Yeah, this is something we might want to change. Some explanation—the 2 lines were used on the 'draggable chip' prior to this PR. That SVG is in the icon package as As part of the PR I brought back the 6 dots icon (the same icon we used on the side of the block prior to 5.5) and added it to the We probably want to ensure there's only one icon in the |
I've pushed a couple of commits that update This didn't quite work when there are multiple blocks: So I've made the block icon on the draggable chip match the toolbar's when dragging multiple blocks. Blocks of the same type use the block icon: Blocks of different type use the 'stack' icon: edit: rebased and fixed the merge conflict too. |
Maybe I'm missing something but testing latest plugin 8.9.1 I see che chip "moves" while dragging so that the mouse cursor isn't on top of the drag handle any longer (and the cursor icon changes to the default). GIF to clarify:
Tested on latest Chrome and Firefox. Note: the "circle" that appears around the cursor is added by the tool I use to make GIFs (LICEcap), please ignore it. |
Complete noob to GitHub and Gutenberg, so apologies for the question (I googled without luck, and ended up here).
|
@photoMaldives That's a really good question. I have no clue why we aren't using that kind of icon. My only guess would be that it might be confusing to have an icon that looks like one of the states of a mouse cursor, but I'm not sure if that's actually a problem or not. What do you think, @afercia? |
To me, it would be a bit confusing as an icon that resembles the |
I see this PR has the "Backport to WP Core" label; which release is it targeted at? |
@tellthemachines It should probably have gone into 5.5.1. I think there was an expectation of a 5.5.2 shortly after, but that didn't happen. It could go into an upcoming 5.5.2, though 5.6 is on the horizon anyway. |
5.5.2 is happening next week, but I'm not sure who's wrangling the backports for it 😬 |
Is there anything else from Gutenberg going into it? |
I don't know, just left comments on a bunch of old-ish PRs that had the backport label to try and find out. I hope it wasn't expected that I do the backports, because I only just found out about this 😅 |
noisysocks added Backport to WP Minor Release 
* Add drag handle to block mover component * Switch draggable chip to reflect toolbar layout * Use drag cursor * Hide drag handle on mobile or in top toolbar mode * Adjust handle and structure. * Size the switcher. * Adjust mover. * Update icon for handle. * Update movers buttons. * Fix groups. * Focus for switcher. * Handle focus. * Fix top toolbar. * Popover fix. * Fix spacing issue. * Harmonize spacing. * Try small independen transition for up / down. * Reduce motion. * use dragHandle icon in draggable chip * Make draggable chip use same icon as toolbar * Revert "Make draggable chip use same icon as toolbar" This reverts commit d031006. * Revert offset change and ensure cursor does not overlap chip block info * Update snapshots to reflect chevron icon change Co-authored-by: jasmussen <joen@automattic.com> Co-authored-by: Matías Ventura <mv@matiasventura.com>
* Refactor BlockMover to use React hooks (#24774) * Add drag handle to block toolbar (#24852) * Add drag handle to block mover component * Switch draggable chip to reflect toolbar layout * Use drag cursor * Hide drag handle on mobile or in top toolbar mode * Adjust handle and structure. * Size the switcher. * Adjust mover. * Update icon for handle. * Update movers buttons. * Fix groups. * Focus for switcher. * Handle focus. * Fix top toolbar. * Popover fix. * Fix spacing issue. * Harmonize spacing. * Try small independen transition for up / down. * Reduce motion. * use dragHandle icon in draggable chip * Make draggable chip use same icon as toolbar * Revert "Make draggable chip use same icon as toolbar" This reverts commit d031006. * Revert offset change and ensure cursor does not overlap chip block info * Update snapshots to reflect chevron icon change Co-authored-by: jasmussen <joen@automattic.com> Co-authored-by: Matías Ventura <mv@matiasventura.com> * Fix issue with single block. (#25107) * Remove animation from mover buttons. (#25728) The animation was intended to better convey direction, and were added as an experiment. It doesn't seem successful, so let's remove it again. * add label in drag and drop button (#25606) * Change toolbar drag remove labels (#25614) * Refactor toolabar drag+remove labels * fix tests * fixes #24845 (#24847) * Fix: Post schedule label showing wrong time if site and user timezones did not match (#26212) * URLInput: Use debounce() instead of throttle() (#26529) Wait until the user finishes typing before sending an AJAX request. This ensures that there isn't an AJAX request sent every 200 ms while the user is typing. * Update browserlist dependency (#24756) * Fix composer test failures due to invalid lock (#26472) * Update package-lock.json * Set dev environment to use WordPress 5.5 Co-authored-by: Chris Alexander <cmalex@gmail.com> Co-authored-by: Daniel Richards <daniel.richards@automattic.com> Co-authored-by: jasmussen <joen@automattic.com> Co-authored-by: Matías Ventura <mv@matiasventura.com> Co-authored-by: Joen A <1204802+jasmussen@users.noreply.github.com> Co-authored-by: Nik Tsekouras <ntsekouras@outlook.com> Co-authored-by: Ari Stathopoulos <aristath@gmail.com> Co-authored-by: Jorge Costa <jorge.costa@automattic.com> Co-authored-by: Riad Benguella <benguella@gmail.com> Co-authored-by: Marcus Kazmierczak <marcus@mkaz.com>
noisysocks removed the Backport to WP Minor Release 
Description
Fixes #24506
Not sure this would be a 'final' solution, but I thought I'd propose something as the current situation isn't ideal. Even an interim fix might be desirable.
Code could be tidied up too (especially the CSS where I used
!importanta few times 😄)Stylistically, I found it hard to get the icon to look centered.
How has this been tested?
Screenshots
The horizontal mover version does get quite big 😬 :
Types of changes
Bug fix (non-breaking change which fixes an issue)
Checklist: