Skip to content

/ gutenberg

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

Jump to bottom

Extract block draggable scroll behaviour into React hook #23444

Merged
merged 4 commits into from Jun 26, 2020
Merged

Extract block draggable scroll behaviour into React hook #23444

merged 4 commits into from Jun 26, 2020

Conversation

@talldan
Copy link
Contributor

@talldan talldan commented Jun 25, 2020 •
edited

Description

As mentioned in https://github.com/WordPress/gutenberg/pull/23082/files#r444178859, the new scroll behavior when dragging can be pretty neatly encapsulated into a React hook.

This PR moves the code into a hook, pretty much verbatim, with a few minor bits of renaming and updating of comments to meet coding standards.

How has this been tested?

  • Block dragging/scrolling behavior should be the same as in master.

Types of changes

Non-breaking code quality change

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 requested review from ellatrix and youknowriad as code owners Jun 25, 2020
@talldan talldan self-assigned this Jun 25, 2020
@talldan talldan requested a review from gravityrail Jun 25, 2020
@github-actions
Copy link

@github-actions github-actions bot commented Jun 25, 2020 •
edited

Size Change: +61 B (0%)

Total Size: 1.13 MB

Filename Size Change
build/block-editor/index.js 109 kB +61 B (0%)
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.14 kB 0 B
build/annotations/index.js 3.62 kB 0 B
build/api-fetch/index.js 3.4 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.37 kB 0 B
build/block-directory/style-rtl.css 941 B 0 B
build/block-directory/style.css 942 B 0 B
build/block-editor/style-rtl.css 10.7 kB 0 B
build/block-editor/style.css 10.7 kB 0 B
build/block-library/editor-rtl.css 7.59 kB 0 B
build/block-library/editor.css 7.6 kB 0 B
build/block-library/index.js 130 kB 0 B
build/block-library/style-rtl.css 8.04 kB 0 B
build/block-library/style.css 8.04 kB 0 B
build/block-library/theme-rtl.css 730 B 0 B
build/block-library/theme.css 732 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 48.2 kB 0 B
build/components/index.js 198 kB 0 B
build/components/style-rtl.css 15.9 kB 0 B
build/components/style.css 15.9 kB 0 B
build/compose/index.js 9.62 kB 0 B
build/core-data/index.js 11.4 kB 0 B
build/data-controls/index.js 1.29 kB 0 B
build/data/index.js 8.44 kB 0 B
build/date/index.js 5.47 kB 0 B
build/deprecated/index.js 772 B 0 B
build/dom-ready/index.js 569 B 0 B
build/dom/index.js 3.19 kB 0 B
build/edit-navigation/index.js 9.87 kB 0 B
build/edit-navigation/style-rtl.css 1.02 kB 0 B
build/edit-navigation/style.css 1.02 kB 0 B
build/edit-post/index.js 303 kB 0 B
build/edit-post/style-rtl.css 5.51 kB 0 B
build/edit-post/style.css 5.5 kB 0 B
build/edit-site/index.js 16.6 kB 0 B
build/edit-site/style-rtl.css 3.02 kB 0 B
build/edit-site/style.css 3.02 kB 0 B
build/edit-widgets/index.js 9.32 kB 0 B
build/edit-widgets/style-rtl.css 2.42 kB 0 B
build/edit-widgets/style.css 2.42 kB 0 B
build/editor/editor-styles-rtl.css 537 B 0 B
build/editor/editor-styles.css 539 B 0 B
build/editor/index.js 44.8 kB 0 B
build/editor/style-rtl.css 3.83 kB 0 B
build/editor/style.css 3.83 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.73 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 622 B 0 B
build/i18n/index.js 3.56 kB 0 B
build/is-shallow-equal/index.js 710 B 0 B
build/keyboard-shortcuts/index.js 2.51 kB 0 B
build/keycodes/index.js 1.94 kB 0 B
build/list-reusable-blocks/index.js 3.13 kB 0 B
build/list-reusable-blocks/style-rtl.css 450 B 0 B
build/list-reusable-blocks/style.css 451 B 0 B
build/media-utils/index.js 5.29 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 663 B 0 B
build/nux/style.css 660 B 0 B
build/plugins/index.js 2.56 kB 0 B
build/primitives/index.js 1.5 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 14 kB 0 B
build/server-side-render/index.js 2.68 kB 0 B
build/shortcode/index.js 1.7 kB 0 B
build/token-list/index.js 1.28 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
@talldan talldan mentioned this pull request Jun 25, 2020
Closed
5 of 6 tasks complete
@youknowriad
youknowriad reviewed Jun 25, 2020
View changes
packages/block-editor/src/components/block-draggable/index.js Outdated
const scrollParentY = useRef( null );

const scrollEditorInterval = useRef( null );
const blockElementId = `block-${ clientIds[ 0 ] }`;

This comment has been minimized.

Sign in to view
@youknowriad

youknowriad Jun 25, 2020
Contributor

I think right now we have a better way to get the block node from the tree using the "BlockNodes" context which means we can probably avoid this argument entirely.

This comment has been minimized.

Sign in to view
@talldan

talldan Jun 25, 2020
Author Contributor

Oh yep, I recall seeing that. I've changed the code to use that.

I noticed as well that the code in the hook is fairly agnostic to any kind of drag/drop event, so I went with making the hook accept a dragElement and BlockDraggable now grabs the block's element from BlockNodes context to pass into the hook.

This comment has been minimized.

Sign in to view
@talldan

talldan Jun 25, 2020
Author Contributor

I didn't see any issues when testing manually, but this change seems to be causing some issues with end-to-end tests.

I'll work on fixing it tomorrow.
@youknowriad
youknowriad approved these changes Jun 25, 2020
View changes
Copy link
Contributor

@youknowriad youknowriad left a comment

I like this refactoring, I also wonder whether we can remove the "key" and the changes to BlockPopover from the other PR too.
This was referenced Jun 25, 2020
Merged
Merged
@paaljoachim
Copy link

@paaljoachim paaljoachim commented Jun 25, 2020 •
edited

Can the same procedure be used here?
Try: Don't show a clone when dragging.
#23024

Thanks for working on this, Dan!
talldan added 4 commits Jun 25, 2020
@talldan
Extract block draggable scroll behaviour into React hook
3c382b9
@talldan
Use BlockNodes context to get block element
afc1324
@talldan
Update JSDoc
eac3c23
@talldan
Check for block nodes being undefined
Loading status checks…
5c79b43
@talldan talldan force-pushed the refactor/block-drag-scroll-behavior-into-hook branch from fd06641 to 5c79b43 Jun 26, 2020
@talldan
Copy link
Contributor Author

@talldan talldan commented Jun 26, 2020

Tests are now passing, apart from PHP tests which seem to have started failing overnight. I can't see any merged PRs that might have caused it, so it's most likely an environment issue.

Using admin privileges to merge this as it contains no PHP changes.
@talldan talldan merged commit d074d26 into master Jun 26, 2020
6 of 7 checks passed
6 of 7 checks passed
build build
Details
build
Details
build
Details
pull-request-automation
Details
test (gutenberg-editor-gallery)
Details
test (gutenberg-editor-gallery)
Details
Travis CI - Pull Request Build Failed
Details
@talldan talldan deleted the refactor/block-drag-scroll-behavior-into-hook branch Jun 26, 2020
@github-actions github-actions bot added this to the Gutenberg 8.5 milestone Jun 26, 2020
@talldan talldan mentioned this pull request Jun 26, 2020
Merged
5 of 6 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@youknowriad youknowriad

@gravityrail gravityrail

@ellatrix ellatrix

Assignees

@talldan talldan

Labels
[Feature] Drag and Drop [Type] Code Quality
Projects
None yet
Milestone
Gutenberg 8.5
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
@talldan @paaljoachim @youknowriad
You can’t perform that action at this time.