-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Convert navigation link to use hooks and context. #28996
Conversation
Size Change: -116 B (0%) Total Size: 1.38 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, no issues in testing, the only thing I noticed was a minor variable naming thing.
selectedBlockId, | ||
] )?.length, | ||
numberOfDescendants: descendants, | ||
isBeingDragged: isDraggingBlocks(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is mostly used to handle when other blocks are being dragged, so the name change isn't quite right.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I couldn't reuse isDraggingBlocks
without making linter unhappy, and I'm not sure what the best name might be. Is areBlocksBeingDragged
too verbose?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's never nice, but the two options are usually assigning the selectors to a variable first and accessing through that:
const selectors = select( blockEditorStore );
// ...
isDraggingBlocks: selectors.isDraggingBlocks(),
Or prefixing the function with an underscore:
const {
isDraggingBlocks: _isDraggingBlocks,
} = select( blockEditorStore );
return {
isDraggingBlocks: _isDraggingBlocks(),
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😑 we can live with the underscore, I guess.
359eb5b
to
6eb17c5
Compare
Description
Follow-up on this comment.
The Navigation link block edit function was using a messy mix of HOCs and hooks for select/dispatch functionality. This PR cleans up and converts it to use only hooks, and get the inherited styles from the parent Navigation block from context instead of attributes.
How has this been tested?
Tested in browser by adding a Navigation block, some links and some submenus with more links, and checking all is working correctly.
Screenshots
Types of changes
Checklist: