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

Stabilize __experimentalMoverDirection InnerBlocks prop and rename to orientation #23416

Merged

Conversation

talldan
Copy link
Contributor

@talldan talldan commented Jun 24, 2020

Description

As mentioned in #23020 (comment), the __experimentalMoverDirection block list setting is now being use for a lot more than establishing the mover direction. While it's experimental and not stable, it'd be a good opportunity to rename it.

This PR changes it to __experimentalBlockListOrientation. Other suggestions welcome.

Should we also consider stabilizing/documenting this API?

How has this been tested?

Manual testing

  • Check that block movers still appear correctly in horizontal block lists (e.g. buttons).
  • Check that drag and drop still works in horizontal block lists

Types of changes

Non-breaking code quality

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 the [Type] Code Quality Issues or PRs that relate to code quality label Jun 24, 2020
@talldan talldan self-assigned this Jun 24, 2020
@talldan talldan changed the title Rename __experimentalMoverDirection to __experimentalBlockListOrientation Rename __experimentalMoverDirection to __experimentalBlockListOrientation Jun 24, 2020
@github-actions
Copy link

github-actions bot commented Jun 24, 2020

Size Change: -266 B (0%)

Total Size: 1.13 MB

Filename Size Change
build/block-directory/index.js 7.42 kB +29 B (0%)
build/block-editor/index.js 109 kB -120 B (0%)
build/block-library/editor-rtl.css 7.63 kB +202 B (2%)
build/block-library/editor.css 7.63 kB +203 B (2%)
build/block-library/index.js 129 kB -100 B (0%)
build/block-library/style-rtl.css 7.8 kB -238 B (3%)
build/block-library/style.css 7.8 kB -242 B (3%)
ℹ️ 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/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/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.65 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.88 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 304 kB 0 B
build/edit-post/style-rtl.css 5.51 kB 0 B
build/edit-post/style.css 5.51 kB 0 B
build/edit-site/index.js 16.6 kB 0 B
build/edit-site/style-rtl.css 3 kB 0 B
build/edit-site/style.css 3 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.9 kB 0 B
build/editor/style-rtl.css 3.85 kB 0 B
build/editor/style.css 3.86 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.72 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.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 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 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.5 kB 0 B
build/priority-queue/index.js 788 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.67 kB 0 B
build/shortcode/index.js 1.69 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

@@ -45,7 +45,7 @@ function UncontrolledInnerBlocks( props ) {
forwardedRef,
templateInsertUpdatesSelection,
__experimentalCaptureToolbars: captureToolbars,
__experimentalMoverDirection,
__experimentalBlockListOrientation,
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe it could be just "orientation" and make it stable? do we expect any other changes to this API?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think so, sounds good to me to stabilize it.

@talldan talldan force-pushed the rename/experimental-mover-direction-to-block-list-orientation branch from ed7a33d to 288e652 Compare July 1, 2020 07:44
@talldan talldan force-pushed the rename/experimental-mover-direction-to-block-list-orientation branch from af82e63 to 1c73a82 Compare July 1, 2020 07:52
clientIds={ blockClientIds }
__experimentalOrientation={ moverDirection }
/>
<BlockMover clientIds={ blockClientIds } />
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed the not needed prop here.

@@ -195,7 +194,6 @@ function BlockPopover( {
<BlockSelectionButton
clientId={ clientId }
rootClientId={ rootClientId }
moverDirection={ moverDirection }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Made BlockSelectionButton use its own selector instead of requiring a prop.

@talldan
Copy link
Contributor Author

talldan commented Jul 1, 2020

This should be ready for a re-review. It's largely just a re-name apart from the two comments I've mentioned above.

The property is now called orientation.

Is there a good label for this change? I'm not sure if [Type] New API applies.

@talldan talldan changed the title Rename __experimentalMoverDirection to __experimentalBlockListOrientation Rename __experimentalMoverDirection to orientation Jul 1, 2020
@youknowriad youknowriad added [Type] New API New API to be used by plugin developers or package users. and removed [Type] Code Quality Issues or PRs that relate to code quality labels Jul 1, 2020
@youknowriad
Copy link
Contributor

yes, New API sounds good. thanks @talldan

@talldan talldan changed the title Rename __experimentalMoverDirection to orientation Stabilize __experimentalMoverDirection InnerBlocks prop and rename to orientation Jul 2, 2020
@talldan talldan merged commit 4dbd8c7 into master Jul 2, 2020
@talldan talldan deleted the rename/experimental-mover-direction-to-block-list-orientation branch July 2, 2020 01:52
@github-actions github-actions bot added this to the Gutenberg 8.5 milestone Jul 2, 2020
@ellatrix
Copy link
Member

ellatrix commented Jul 2, 2020

Are we expecting more than two orientations? If it's a binary thing, a boolean prop could avoid a string, which I think is a bit cleaner.

@youknowriad
Copy link
Contributor

I think a binary doesn't convey the right message. You can say isHorizontal=true/false but not orientation=true/false
I have a preference for orientation="vertical".

Whether we'll have other values, I'm not sure but potentially something like "grid" could be supported or "both".

@ellatrix
Copy link
Member

ellatrix commented Jul 2, 2020

@youknowriad Makes sense :)

@chrisvanpatten
Copy link
Contributor

I’d loooove grid someday! Thanks for stabilising this API yall!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] New API New API to be used by plugin developers or package users.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants