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

Fixes to append items via toolbar #18412

Merged
merged 1 commit into from Nov 11, 2019

Conversation

@getdave
Copy link
Contributor

getdave commented Nov 8, 2019

Closes #18299

Updates the Nav Block toolbar "insert" button to append items at the end of the currently selected Nav Block Item.

Previously this was hardcoded to 0 which meant they were always prepended.

How has this been tested?

Manual testing.

Screenshots

Screen Capture on 2019-11-08 at 17-31-26

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. .
@getdave getdave self-assigned this Nov 8, 2019
@getdave getdave added this to 👀 PRs to review in Navigation block via automation Nov 8, 2019
@@ -245,7 +245,7 @@ export default compose( [
hasDescendants: !! getClientIdsOfDescendants( [ clientId ] ).length,
};
} ),
withDispatch( ( dispatch, ownProps ) => {
withDispatch( ( dispatch, ownProps, registry ) => {

This comment has been minimized.

Copy link
@obenland

obenland Nov 8, 2019

Member

I think the only suggestion I'd have would be to maybe consider destructuring registry to just pick select?

This comment has been minimized.

Copy link
@getdave

getdave Nov 11, 2019

Author Contributor

I had some advice from @mcsf that Gutenberg prefers being explicit about where you are accessing the properties of data controls. I took this as avoiding unnecessary destructuring in/around select/dispatch/registry. I could be wrong.

This comment has been minimized.

Copy link
@mcsf

mcsf Nov 11, 2019

Contributor

I had some advice from @mcsf that Gutenberg prefers being explicit about where you are accessing the properties of data controls. I took this as avoiding unnecessary destructuring in/around select/dispatch/registry. I could be wrong.

Thanks for thinking about my advice 😄. In this case I don't really have an opinion, since, in the context of withDispatch, select could only come from one place.

My main point — which was merely stylistic — in the other PR was about not extracting the selectors into their own variable, e.g. const { getFoos } = select( store ), and instead always calling select( store ).getFoos( ...args ).

@getdave

This comment has been minimized.

Copy link
Contributor Author

getdave commented Nov 11, 2019

Waiting on build issues to merge...

@getdave getdave merged commit 6f4dfc1 into master Nov 11, 2019
2 checks passed
2 checks passed
pull-request-automation
Details
Travis CI - Pull Request Build Passed
Details
Navigation block automation moved this from 👀 PRs to review to ✅ Done Nov 11, 2019
@getdave getdave deleted the fix/nav-block-items-prepeneded-by-toolbar-btn branch Nov 11, 2019
Copy link
Contributor

mcsf left a comment

Have you tried calling insertBlock with an undefined index? I had noticed that the argument was considered optional so I tried dropping it, and I think the same effect is achieved.

You can see in the order reducer where index defaults to the substate's length:

const { index = subState.length } = action;

Quick check with @youknowriad: the documentation for insertBlock(s) isn't super explicit about appending when index is missing, but the signature does state "inserted, optionally at a specific index respective a root block list". We can recommend this usage, right?

@@ -245,7 +245,7 @@ export default compose( [
hasDescendants: !! getClientIdsOfDescendants( [ clientId ] ).length,
};
} ),
withDispatch( ( dispatch, ownProps ) => {
withDispatch( ( dispatch, ownProps, registry ) => {

This comment has been minimized.

Copy link
@mcsf

mcsf Nov 11, 2019

Contributor

I had some advice from @mcsf that Gutenberg prefers being explicit about where you are accessing the properties of data controls. I took this as avoiding unnecessary destructuring in/around select/dispatch/registry. I could be wrong.

Thanks for thinking about my advice 😄. In this case I don't really have an opinion, since, in the context of withDispatch, select could only come from one place.

My main point — which was merely stylistic — in the other PR was about not extracting the selectors into their own variable, e.g. const { getFoos } = select( store ), and instead always calling select( store ).getFoos( ...args ).

@youknowriad youknowriad added this to the Gutenberg 6.9 milestone Nov 11, 2019
CreativeDive added a commit to CreativeDive/gutenberg that referenced this pull request Nov 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.