-
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
Block API: Reimplement align as common extension #4069
Conversation
blocks/hooks/align.js
Outdated
return [ | ||
validAlignments.length > 0 && props.focus && ( | ||
<BlockControls key="controls"> | ||
<BlockAlignmentToolbar |
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.
Given that we support the configuration of the supported aligns via an array, we need some way to pass the validAlignments to the BlockAlignmentToolbar. If we set support for align: [ 'center', 'right' ], left align continues to appear although it does not take effect because class and data-align prop is not set. Given that left align is disabled in that case we should not show it.
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.
Given that we support the configuration of the supported aligns via an array, we need some way to pass the validAlignments to the BlockAlignmentToolbar.
An easy enough fix. See 9a2db6e.
@aduth do you plan to finish this PR or is it no longer relevant? 2 months has passed, there has been tons of changes in the meantime :) |
You make it sound as though it was incomplete 😄 But yes, it has languished and would need an update. I think it's still worth pursuing personally. |
For retrieving argument-based block supports
9a2db6e
to
ebfa989
Compare
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 updated code to work with the latest code refactoring around focus
property. This all works as advertised and is ready to go 👍 Nice one. I like how flexible this solution is.
@@ -471,9 +471,12 @@ export class BlockListBlock extends Component { | |||
const { onMouseLeave, onReplace } = this.props; | |||
|
|||
// Determine whether the block has props to apply to the wrapper. | |||
let wrapperProps; | |||
let wrapperProps = this.props.wrapperProps; |
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.
@aduth, do I assume correctly that we should be able to remove this if statement as soon as we migrate all blocks to use supports
?
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.
My hope is that we can remove getEditWrapperProps
at some point, yes.
// Explicitly defined array set of valid alignments | ||
const blockAlign = getBlockSupport( blockName, 'align' ); | ||
if ( Array.isArray( blockAlign ) ) { | ||
return blockAlign; |
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.
Do we want to allow custom values here? At the moment you can pass anything :)
This pull request seeks to explore a refactoring of block alignment as a common
supports
feature, responsible for managing:alignleft
etc. class nameIncluded is a single ported example to demonstrate the new behavior. See the Audio block changes for an example of how this can help simplify common alignment behavior. This is part of an effort toward eliminating
getEditWrapperProps
altogether.Implementation notes:
A new block API
getBlockSupport
has been added, complementinghasBlockSupport
and intended to be used in cases where a block support is defined as a non-boolean type (mimicking...args
ofadd_theme_support
), used to allow granular control of specific supported alignments.The theme supports feature for wide alignment has been renamed with these changes from
wide-images
towide-align
to better reflect that wide alignment can apply to other block types (audio, etc).Testing instructions:
Verify that there are no regressions in the alignment behavior of the Audio block.
Ensure unit tests pass: