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

Annotations: Merge assigned block className with incoming prop #21184

Merged
merged 1 commit into from
Mar 26, 2020

Conversation

aduth
Copy link
Member

@aduth aduth commented Mar 26, 2020

Fixes #21172
Related: #19514

This pull request seeks to resolve an issue in the implementation of annotations whereby it would replace the className assigned to the original component. This interfered with the changes introduced as of #19514 in that the .is-drop-target class name was no longer being assigned.

The changes here resolve this by merging any incoming className value to the assigned className.

Testing Instructions:

Repeat Steps to Reproduce from #21172, verifying the drop target appears when dragging a block.

As noted in #21172 (comment), this is most easily accomplished by enabling the "Gutenberg Test Plugin, Plugins API" plugin, assuming the default development environment is used.

Future Tasks:

There should be automated testing for this behavior. However, the implementation of annotations using withSelect is not easily tested without significant refactoring of the implementation itself (migrating to use useSelect hook and/or extracting common function for class name assignment). Since this may or may not need to be cherry-picked for WordPress 5.4, it would be best to save refactoring for future effort.

cc @herregroen

@aduth aduth added [Type] Bug An existing feature does not function as intended [Type] Regression Related to a regression in the latest release [Feature] Annotations Adding annotation functionality labels Mar 26, 2020
@aduth aduth requested a review from ellatrix March 26, 2020 19:38
@aduth aduth requested a review from atimmer as a code owner March 26, 2020 19:38
@github-actions
Copy link

Size Change: +18 B (0%)

Total Size: 856 kB

Filename Size Change
build/annotations/index.js 3.44 kB +18 B (0%)
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 998 B 0 B
build/api-fetch/index.js 3.39 kB 0 B
build/autop/index.js 2.58 kB 0 B
build/blob/index.js 620 B 0 B
build/block-directory/index.js 6.02 kB 0 B
build/block-directory/style-rtl.css 760 B 0 B
build/block-directory/style.css 760 B 0 B
build/block-editor/index.js 101 kB 0 B
build/block-editor/style-rtl.css 10.9 kB 0 B
build/block-editor/style.css 10.9 kB 0 B
build/block-library/editor-rtl.css 7.22 kB 0 B
build/block-library/editor.css 7.23 kB 0 B
build/block-library/index.js 110 kB 0 B
build/block-library/style-rtl.css 7.43 kB 0 B
build/block-library/style.css 7.44 kB 0 B
build/block-library/theme-rtl.css 669 B 0 B
build/block-library/theme.css 671 B 0 B
build/block-serialization-default-parser/index.js 1.65 kB 0 B
build/block-serialization-spec-parser/index.js 3.1 kB 0 B
build/blocks/index.js 57.5 kB 0 B
build/components/index.js 190 kB 0 B
build/components/style-rtl.css 15.8 kB 0 B
build/components/style.css 15.7 kB 0 B
build/compose/index.js 6.21 kB 0 B
build/core-data/index.js 10.6 kB 0 B
build/data-controls/index.js 1.04 kB 0 B
build/data/index.js 8.25 kB 0 B
build/date/index.js 5.37 kB 0 B
build/deprecated/index.js 771 B 0 B
build/dom-ready/index.js 568 B 0 B
build/dom/index.js 3.06 kB 0 B
build/edit-post/index.js 91.2 kB 0 B
build/edit-post/style-rtl.css 8.43 kB 0 B
build/edit-post/style.css 8.43 kB 0 B
build/edit-site/index.js 6.73 kB 0 B
build/edit-site/style-rtl.css 2.91 kB 0 B
build/edit-site/style.css 2.9 kB 0 B
build/edit-widgets/index.js 4.43 kB 0 B
build/edit-widgets/style-rtl.css 2.57 kB 0 B
build/edit-widgets/style.css 2.57 kB 0 B
build/editor/editor-styles-rtl.css 428 B 0 B
build/editor/editor-styles.css 431 B 0 B
build/editor/index.js 42.8 kB 0 B
build/editor/style-rtl.css 3.38 kB 0 B
build/editor/style.css 3.38 kB 0 B
build/element/index.js 4.44 kB 0 B
build/escape-html/index.js 733 B 0 B
build/format-library/index.js 6.95 kB 0 B
build/format-library/style-rtl.css 502 B 0 B
build/format-library/style.css 502 B 0 B
build/hooks/index.js 1.93 kB 0 B
build/html-entities/index.js 622 B 0 B
build/i18n/index.js 3.49 kB 0 B
build/is-shallow-equal/index.js 710 B 0 B
build/keyboard-shortcuts/index.js 2.3 kB 0 B
build/keycodes/index.js 1.69 kB 0 B
build/list-reusable-blocks/index.js 2.99 kB 0 B
build/list-reusable-blocks/style-rtl.css 226 B 0 B
build/list-reusable-blocks/style.css 226 B 0 B
build/media-utils/index.js 4.84 kB 0 B
build/notices/index.js 1.57 kB 0 B
build/nux/index.js 3.01 kB 0 B
build/nux/style-rtl.css 616 B 0 B
build/nux/style.css 613 B 0 B
build/plugins/index.js 2.54 kB 0 B
build/primitives/index.js 1.5 kB 0 B
build/priority-queue/index.js 781 B 0 B
build/redux-routine/index.js 2.84 kB 0 B
build/rich-text/index.js 14.5 kB 0 B
build/server-side-render/index.js 2.55 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.01 kB 0 B
build/viewport/index.js 1.61 kB 0 B
build/warning/index.js 1.14 kB 0 B
build/wordcount/index.js 1.18 kB 0 B

compressed-size-action

Copy link
Member

@ellatrix ellatrix left a comment

Choose a reason for hiding this comment

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

Should we add a test? I'm fine either way.

@aduth
Copy link
Member Author

aduth commented Mar 26, 2020

@ellatrix From the original comment:

There should be automated testing for this behavior. However, the implementation of annotations using withSelect is not easily tested without significant refactoring of the implementation itself (migrating to use useSelect hook and/or extracting common function for class name assignment). Since this may or may not need to be cherry-picked for WordPress 5.4, it would be best to save refactoring for future effort.

@aduth aduth merged commit 27bfa8c into master Mar 26, 2020
@aduth aduth deleted the fix/21172-annotations-drop-target branch March 26, 2020 21:19
@github-actions github-actions bot added this to the Gutenberg 7.9 milestone Mar 26, 2020
@jorgefilipecosta jorgefilipecosta added the Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label Mar 27, 2020
@jorgefilipecosta jorgefilipecosta removed the Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label Apr 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Annotations Adding annotation functionality [Type] Bug An existing feature does not function as intended [Type] Regression Related to a regression in the latest release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The annotations package breaks drop targets when dragging blocks
3 participants