-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Surface alignment toolbar as block control #987
Conversation
@@ -8,10 +8,11 @@ import { Fill } from 'react-slot-fill'; | |||
*/ | |||
import { Toolbar } from 'components'; | |||
|
|||
export default function BlockControls( { controls } ) { | |||
export default function BlockControls( { controls, children } ) { | |||
return ( | |||
<Fill name="Formatting.Toolbar"> | |||
<Toolbar controls={ controls } /> |
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 probably should make controls
optional and avoid including this if no controls are provided
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.
Technically it is optional, and <Toolbar />
will not render itself when omitted:
https://github.com/WordPress/gutenberg/blob/97ee1a3/components/toolbar/index.js#L13-L15
Example: Included Text and Quote block changes render <BlockControls />
without controls
prop.
return ( | ||
<blockquote className={ `blocks-quote blocks-quote-style-${ style }` }> | ||
return [ | ||
focus && ( |
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 was thinking about the focus check, I guess we could avoid it by hiding the controls using CSS instead of removing them completely from the DOM.
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've grown more okay with it. Seems reasonable to expect that the render logic should be responsible for deciding if controls should be shown depending on whether the block currently has focus.
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.
LGTM 👍
I have some notes that could be resolved separately though.
- Should we keep the alignments when splitting a text block?
- Should we keep the alignments when transforming blocks (text 2 quote, quote 2 text)
Oh I just noticed the alignment is not working on quotes (working only on the cite) |
Good catch. I had styles on the block's Editables reversed (intended for text, not cite). Fixed in 17c066b. |
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.
Works great.
17c066b
to
cb815ad
Compare
This pull request effectively reverts behavior introduced in #533 . It serves as a first step toward considering text blocks as individual paragraphs.
Implementation notes:
These changes identified a few pain points:
data-
attributes for this (both in styling and sourcing attributes), but did not include this as part of the changes here because this seems better addressed by a formal styling proposal or attributes extraction enhancements (Block parsing and serialization/deserialization #391).In changes, alignment controls were removed from the list block. I'm not really certain I understand the purpose of list alignment, but could restore the behavior if it's considered desirable.
Testing instructions:
Ensure tests pass:
Verify that alignment behavior continues to work without regression, except that it applies to an entire block now. Serializing should encode the alignment attribute in block comments, and should be preserved when loading an existing post.