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

Add drag handle to block toolbar #24852

Merged
merged 23 commits into from
Sep 3, 2020
Merged

Add drag handle to block toolbar #24852

merged 23 commits into from
Sep 3, 2020

Conversation

talldan
Copy link
Contributor

@talldan talldan commented Aug 27, 2020

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 !important a few times 😄)

Stylistically, I found it hard to get the icon to look centered.

How has this been tested?

  • Try dragging and dropping blocks
  • Also worth testing mobile and 'Top Toolbar', the drag handle shouldn't be visible.

Screenshots

Screenshot 2020-08-27 at 6 33 13 pm

The horizontal mover version does get quite big 😬 :
Screenshot 2020-08-27 at 6 40 27 pm

Types of changes

Bug fix (non-breaking change which fixes an issue)

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR.

@talldan talldan added [Type] Regression Related to a regression in the latest release [Feature] Drag and Drop Drag and drop functionality when working with blocks labels Aug 27, 2020
@talldan talldan self-assigned this Aug 27, 2020
@github-actions
Copy link

github-actions bot commented Aug 27, 2020

Size Change: +3 kB (0%)

Total Size: 1.17 MB

Filename Size Change
build/block-editor/index.js 126 kB +229 B (0%)
build/block-editor/style-rtl.css 11 kB +232 B (2%)
build/block-editor/style.css 11 kB +233 B (2%)
build/block-library/index.js 136 kB +143 B (0%)
build/components/index.js 200 kB +39 B (0%)
build/components/style-rtl.css 15.5 kB +2 B (0%)
build/components/style.css 15.5 kB +2 B (0%)
build/edit-post/index.js 305 kB +600 B (0%)
build/edit-post/style-rtl.css 6.26 kB +645 B (10%) ⚠️
build/edit-post/style.css 6.24 kB +637 B (10%) ⚠️
build/edit-site/index.js 17 kB +51 B (0%)
build/edit-widgets/index.js 12 kB +61 B (0%)
build/editor/index.js 45.4 kB +125 B (0%)
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.14 kB 0 B
build/annotations/index.js 3.67 kB 0 B
build/api-fetch/index.js 3.44 kB 0 B
build/autop/index.js 2.82 kB 0 B
build/blob/index.js 620 B 0 B
build/block-directory/index.js 7.99 kB 0 B
build/block-directory/style-rtl.css 953 B 0 B
build/block-directory/style.css 952 B 0 B
build/block-library/editor-rtl.css 8.55 kB 0 B
build/block-library/editor.css 8.55 kB 0 B
build/block-library/style-rtl.css 7.47 kB 0 B
build/block-library/style.css 7.47 kB 0 B
build/block-library/theme-rtl.css 741 B 0 B
build/block-library/theme.css 742 B 0 B
build/block-serialization-default-parser/index.js 1.88 kB 0 B
build/block-serialization-spec-parser/index.js 3.1 kB 0 B
build/blocks/index.js 47.7 kB 0 B
build/compose/index.js 9.67 kB 0 B
build/core-data/index.js 12.3 kB 0 B
build/data-controls/index.js 1.29 kB 0 B
build/data/index.js 8.55 kB 0 B
build/date/index.js 5.38 kB 0 B
build/deprecated/index.js 772 B 0 B
build/dom-ready/index.js 568 B 0 B
build/dom/index.js 4.48 kB 0 B
build/edit-navigation/index.js 11.6 kB 0 B
build/edit-navigation/style-rtl.css 1.16 kB 0 B
build/edit-navigation/style.css 1.16 kB 0 B
build/edit-site/style-rtl.css 3.06 kB 0 B
build/edit-site/style.css 3.06 kB 0 B
build/edit-widgets/style-rtl.css 2.46 kB 0 B
build/edit-widgets/style.css 2.45 kB 0 B
build/editor/editor-styles-rtl.css 492 B 0 B
build/editor/editor-styles.css 493 B 0 B
build/editor/style-rtl.css 3.81 kB 0 B
build/editor/style.css 3.81 kB 0 B
build/element/index.js 4.65 kB 0 B
build/escape-html/index.js 733 B 0 B
build/format-library/index.js 7.71 kB 0 B
build/format-library/style-rtl.css 547 B 0 B
build/format-library/style.css 548 B 0 B
build/hooks/index.js 2.13 kB 0 B
build/html-entities/index.js 621 B 0 B
build/i18n/index.js 3.57 kB 0 B
build/is-shallow-equal/index.js 710 B 0 B
build/keyboard-shortcuts/index.js 2.52 kB 0 B
build/keycodes/index.js 1.94 kB 0 B
build/list-reusable-blocks/index.js 3.12 kB 0 B
build/list-reusable-blocks/style-rtl.css 476 B 0 B
build/list-reusable-blocks/style.css 476 B 0 B
build/media-utils/index.js 5.32 kB 0 B
build/notices/index.js 1.79 kB 0 B
build/nux/index.js 3.4 kB 0 B
build/nux/style-rtl.css 671 B 0 B
build/nux/style.css 668 B 0 B
build/plugins/index.js 2.56 kB 0 B
build/primitives/index.js 1.41 kB 0 B
build/priority-queue/index.js 789 B 0 B
build/redux-routine/index.js 2.85 kB 0 B
build/rich-text/index.js 13.9 kB 0 B
build/server-side-render/index.js 2.77 kB 0 B
build/shortcode/index.js 1.7 kB 0 B
build/token-list/index.js 1.27 kB 0 B
build/url/index.js 4.06 kB 0 B
build/viewport/index.js 1.85 kB 0 B
build/warning/index.js 1.14 kB 0 B
build/wordcount/index.js 1.17 kB 0 B

compressed-size-action

@ZebulanStanphill ZebulanStanphill added the [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). label Aug 27, 2020
@ZebulanStanphill ZebulanStanphill added the Needs Accessibility Feedback Need input from accessibility label Aug 27, 2020
Comment on lines 49 to 53
<FlexItem>
<BlockIcon icon={ handle } />
</FlexItem>
Copy link
Member

Choose a reason for hiding this comment

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

What's the purpose of moving this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

@talldan talldan added the Needs Design Feedback Needs general design feedback. label Aug 28, 2020
Comment on lines 72 to 73
<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">
Copy link
Member

Choose a reason for hiding this comment

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

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).

Copy link
Contributor

Choose a reason for hiding this comment

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

@jasmussen
Copy link
Contributor

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.

@mtias
Copy link
Member

mtias commented Aug 29, 2020

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.

@afercia
Copy link
Contributor

afercia commented Aug 29, 2020

I will make a mockup monday that I believe can address this.

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?

Screenshot 2020-08-29 at 15 43 25

[Edit] I see this is being explored in #24092

@jasmussen
Copy link
Contributor

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.

Mockup that adds handle added here: #24898 (comment)

Mockup that improves select mode with drag and drop here: #24092 (comment)

@jasmussen
Copy link
Contributor

jasmussen commented Aug 31, 2020

I took the liberty of pushing tweaks to this branch that makes the handle match the mockup. GIF:

spaced

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.

@afercia
Copy link
Contributor

afercia commented Aug 31, 2020

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?

@talldan
Copy link
Contributor Author

talldan commented Sep 1, 2020

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 handle. Initially I tried using that on the toolbar, but found it looks very similar to the text justification icon.

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 icon package as dragHandle, but I haven't changed the draggable chip to use that.

We probably want to ensure there's only one icon in the icons package for this purpose. @jasmussen what do you think?

@talldan
Copy link
Contributor Author

talldan commented Sep 1, 2020

I've pushed a couple of commits that update DraggableChip to use the same drag handle and made it so that the cursor is positioned over the handle on the chip when dragging:
draggable-chip

This didn't quite work when there are multiple blocks:
multiblock

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:
multidragsame

Blocks of different type use the 'stack' icon:
multidragdifferent

edit: rebased and fixed the merge conflict too.

@talldan talldan merged commit 430c938 into master Sep 3, 2020
@talldan talldan deleted the add/drag-handle-to-toolbar branch September 3, 2020 04:02
@github-actions github-actions bot added this to the Gutenberg 9.0 milestone Sep 3, 2020
@afercia
Copy link
Contributor

afercia commented Sep 3, 2020

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:

dragging

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.

@photoMaldives
Copy link

Complete noob to GitHub and Gutenberg, so apologies for the question (I googled without luck, and ended up here).
I was wondering why Gutenberg does not use a more standard drag icon?

image

@mapk mapk added the Needs Figma Update Needs an update to Figma for design purposes label Sep 10, 2020
@ZebulanStanphill
Copy link
Member

@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?

@afercia
Copy link
Contributor

afercia commented Sep 15, 2020

To me, it would be a bit confusing as an icon that resembles the move mouse pointer would be overlaid by the move mouse pointer itself. Open to explore a better icon though, as the 6-dots one isn't that meaningful. More something for design?

@tellthemachines
Copy link
Contributor

I see this PR has the "Backport to WP Core" label; which release is it targeted at?

@talldan
Copy link
Contributor Author

talldan commented Oct 23, 2020

@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.

@tellthemachines
Copy link
Contributor

5.5.2 is happening next week, but I'm not sure who's wrangling the backports for it 😬

@talldan
Copy link
Contributor Author

talldan commented Oct 23, 2020

Is there anything else from Gutenberg going into it?

@tellthemachines
Copy link
Contributor

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 noisysocks added Backport to WP Minor Release Pull request that needs to be backported to a WordPress minor release and removed Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta labels Oct 29, 2020
talldan added a commit that referenced this pull request Oct 29, 2020
* 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 <[email protected]>
Co-authored-by: Matías Ventura <[email protected]>
noisysocks added a commit that referenced this pull request Oct 29, 2020
* 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 <[email protected]>
Co-authored-by: Matías Ventura <[email protected]>

* 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 <[email protected]>
Co-authored-by: Daniel Richards <[email protected]>
Co-authored-by: jasmussen <[email protected]>
Co-authored-by: Matías Ventura <[email protected]>
Co-authored-by: Joen A <[email protected]>
Co-authored-by: Nik Tsekouras <[email protected]>
Co-authored-by: Ari Stathopoulos <[email protected]>
Co-authored-by: Jorge Costa <[email protected]>
Co-authored-by: Riad Benguella <[email protected]>
Co-authored-by: Marcus Kazmierczak <[email protected]>
@noisysocks noisysocks removed the Backport to WP Minor Release Pull request that needs to be backported to a WordPress minor release label Oct 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Drag and Drop Drag and drop functionality when working with blocks [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). Needs Accessibility Feedback Need input from accessibility Needs Design Feedback Needs general design feedback. Needs Figma Update Needs an update to Figma for design purposes [Type] Regression Related to a regression in the latest release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cursor doesn't change to indicate block can be dragged