-
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
Reusable Blocks: Add reusable blocks UI #3378
Reusable Blocks: Add reusable blocks UI #3378
Conversation
editor/effects.js
Outdated
const { name, type, attributes } = getReusableBlock( getState(), id ); | ||
const content = serialize( createBlock( type, attributes ) ); | ||
|
||
new wp.api.models.ReusableBlocks( { id, name, content } ).save().then( |
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.
What is the name
property? The WP REST API doesn't support a name
attribute. Shouldn't it be title
of the custom post type?
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.
name
is the name of the reusable block: https://github.com/WordPress/gutenberg/blob/master/lib/class-wp-rest-reusable-blocks-controller.php#L302-L307
I agree that naming that title
is a better thing to call that, though. I'll put it on my list!
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.
Ah, I see, thanks.
Your call. :)
After you checkout the branch, try deleting |
I'm cc'ing in @jasmussen as I think we need to iterate on the design here. Currently we have this: This I am suggesting could be iterated by having some more friendly and informative language. Here are some suggestions: Overall, I think we need to think about the flow of this feature and how it could be discovered. I think where we have this is too hidden: Could we add this perhaps in the menu? Note, I've not added the icon and maybe we also need a tooltip to explain what this is. |
I think using the ellipsis menu for making reusable blocks has potential. I'm slightly concerned about putting more stuff in there as we're already looking at a few options there, with a bunch of transformations to come also. But it does feel like a good place for it. |
@jasmussen I have that concern myself. The current implementation feels like hiding a good feature though. Maybe we need to try it? Do we have a best guess at how many things we are going to have if everything we want gets in today? I'm thinking of a visual stress test. |
I think this may be one of those features whose full potential won't be realized until slightly later on, perhaps post phase-1, where we have more templating features and so on. Given this, perhaps we shouldn't hyper-optimize for the current UI, but instead leave room for us to learn about the feature and then find a better place. To put it differently, there seems to be agreement that it's a slick feature that deserves a better place, but perhaps it's a problem that's best solved in the future? |
Codecov Report
@@ Coverage Diff @@
## master #3378 +/- ##
==========================================
- Coverage 37.7% 37.46% -0.24%
==========================================
Files 279 281 +2
Lines 6737 6780 +43
Branches 1226 1230 +4
==========================================
Hits 2540 2540
- Misses 3536 3575 +39
- Partials 661 665 +4
Continue to review full report at Codecov.
|
I rebased this PR and fixed some bugs that were introduced in the last rebase! 🙂 Since The E2E tests are failing. A |
@jasmussen I still think it needs to move from where it is currently, I am ok if that for now is the ellipsis. |
Thanks for the 👀 @karmatosed! I'll move the controls into the ⋯ menu 🙂 |
For posterity: turns out this was because |
I moved the button into the block settings menu. @jasmussen—are you able to whip up an icon for this? |
Nice work. While the icon definitely could use some love, I think for now let's go with this dashicon https://developer.wordpress.org/resource/dashicons/#controls-repeat Reusable blocks is an important interface that I expect we'll hone and polish many times in the future, but for now let's get the ball rolling! |
Icon changed! |
@@ -0,0 +1,23 @@ | |||
/** |
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.
This component is not used anymore, 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.
You're right! I've removed it.
editor/components/inserter/group.js
Outdated
@@ -48,7 +48,7 @@ export default class InserterGroup extends Component { | |||
role="menuitem" | |||
key={ block.name } | |||
className="editor-inserter__block" | |||
onClick={ selectBlock( block.name ) } | |||
onClick={ selectBlock( block.name, block.attributes ) } |
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.
Maybe we should just pass the block
object as an argument here instead of two arguments.
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.
👌 fixed in 8ced4510795d9e428a2f6d1c2bbb4e2f744da23b.
editor/components/inserter/menu.js
Outdated
return () => { | ||
this.props.onSelect( name ); | ||
this.props.onSelect( name, attributes ); |
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.
Thinking more about this "attributes" argument. Regular blocks already have an attributes
property but instead of containing attributes
values, it contains attributes
definition. I assume this will be called with the attributes
definitions for these regular blocks which is not what's expected I guess.
It may not produce bugs at the moment but I think we should fix this ambiguity somehow.
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.
👌 fixed in 8ced4510795d9e428a2f6d1c2bbb4e2f744da23b.
Also for later: creation of global blocks should be handled with proper capabilities (an author would not get access to create reusable blocks, but an editor would, etc). And let this be filterable if someone wants to allow anyone to create reusable blocks. |
} | ||
|
||
register_block_type( 'core/reusable-block', array( | ||
'attributes' => array( |
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.
Some odd spaces here.
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.
The linter requires that the arrow on line 37 aligns with the arrow on line 43.
return gutenberg_render_block( $block ); | ||
} | ||
|
||
register_block_type( 'core/reusable-block', array( |
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.
All the other terms we can more easily change, like internal variables, actions, and functions. But this we'll have to support once we announce.
I am not super comfortable with the name. A couple ideas:
core/global-block
core/saved-block
-> to match the inserter tab.- or maybe this is an exception and is saved like
global-block
without the core suffix. Not to keen on an exception here, but it might warrant it (also of note, we havecore-embed
as a namespace.
For reference, the CPT is now called wp-block
.
cc @aduth @youknowriad @mcsf @pento for naming thoughts
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'm not a native speaker, so if reusable-block
is clearest I'm fine with sticking with it, just want to make sure we are ok with 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.
Thinking a bit more, it seems the most clear and consistent with the wp-block
post type is to call this core/block
which maps to wp:block
and wp:core/block
. Thoughts?
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 only hesitation with global block is that global is a fairly technical term and that using it in the UI might limit how approachable this feature is to a wider audience.
For technical usages though, I'm happy with using a more precise term. core/block
matching up with wp-block
is very appealing.
Note that if we change this then we ought to change the /gutenberg/v1/reusable-blocks
endpoint too, which would be a (fairly insignificant) breaking change.
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.
Saved block might speak to users more. Further, though I'm not a huge fan, there are precedents out there to justify custom blocks. Note that I can see reusable blocks working, if visual context and copy correctly introduce the concept.
Playing devil's advocate, saved and custom blocks may sound confusing ("Aren't I already making my own block when I add a "Paragraph" and type something in? Isn't it custom? Isn't it being saved?"). Those terms stress a different aspect from reusable, with reusable arguably stressing the user-facing value. (Mind you, a similar case can be made against reusable: "This Paragraph, or this Author block, aren't they reusable?")
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'm going to go ahead and rename the block identifier to core/block
so that we can move ahead with this PR. This makes sense to me since, just as <!-- wp:image -->
inserts an image, <!-- wp:block -->
inserts a wp_block
.
Let's move the discussion about the name of the overall feature to a seperate issue. For what it's worth, I asked my wife (a regular WordPress user) what she thought and she said that custom block makes the most sense 🙂
icon="controls-repeat" | ||
onClick={ isReusableBlock ? convertToStatic : convertToReusable } | ||
> | ||
{ isReusableBlock ? __( 'Detach from Reusable Block' ) : __( 'Make into a Reusable Block' ) } |
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.
Would "Convert to Reusable Block" or "Convert to Global Block" be more concise?
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.
Changed in 619d81f51afbd3bf0fd5a407e2a82a993b7a7051.
lib/blocks.php
Outdated
/** | ||
* Renders a single block into a HTML string. | ||
* | ||
* @since 1.3.0 |
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.
This needs to be updated.
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.
Fixed in b8f96e9b5e7c905d37ba7e392083de1a249cc8a1.
@@ -790,7 +790,7 @@ function gutenberg_editor_scripts_and_styles( $hook ) { | |||
$script = '( function() {'; | |||
$script .= sprintf( 'var editorSettings = %s;', wp_json_encode( $editor_settings ) ); | |||
$script .= <<<JS | |||
window._wpLoadGutenbergEditor = Promise.all( [ wp.api.init(), wp.api.init( { versionString: 'gutenberg/v1' } ) ] ).then( function() { | |||
window._wpLoadGutenbergEditor = Promise.all( [ wp.api.init(), wp.api.init( { versionString: 'gutenberg/v1/' } ) ] ).then( function() { |
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.
curious, what's the reason for this change?
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.
None of the Backbone methods work without the trailing slash, but we didn't catch that while testing #3377 since the unit tests mock the API call and no UI existed for any of this.
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 you think this is still useful with #3778
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.
Yes, since we're not using withAPIData
.
const generatedClassName = hasBlockSupport( blockType, 'className', true ) ? | ||
getBlockDefaultClassname( reusableBlock.type ) : | ||
null; | ||
const className = classnames( generatedClassName, reusableBlockAttributes.className ); |
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.
Clever! Thanks so much @youknowriad 🙏
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.
Most comments aren't critical. This works great and is very cool to use and see 👍
key="save" | ||
isPrimary | ||
isLarge | ||
isIndicatingProgress={ isSaving } |
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.
This prop does not exist on the Button
component, and since Button passes through unrecognized props to the underlying DOM node, it results in a warning while editing:
Warning: React does not recognize the
isIndicatingProgress
prop on a DOM element. If you intentionally want it to appear in the DOM as a custom attribute, spell it as lowercaseisindicatingprogress
instead. If you accidentally passed it from a parent component, remove it from the DOM element.
Should this be the isBusy
prop added in #3682?
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.
Good catch. It definitely should be isBusy
. Fixed in a820d1cc43dddd97d44e6e756707dfc6ab56c80b.
{ __( 'Detach' ) } | ||
</Button>, | ||
] } | ||
{ ( isEditing || isSaving ) && [ |
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.
Should the editing variation be visible while the name is being saved? In my testing, it flips back to the static text immediately upon hitting Save.
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.
Good catch—that's a bug that must have slipped in during a merge. Fixed in a820d1cc43dddd97d44e6e756707dfc6ab56c80b.
} | ||
} | ||
|
||
const ConnectedReusableBlockEdit = connect( |
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 don't have any ideas at the moment and it may have already been mentioned, but we need to work to find a solution where the block can access state without the implicit dependency on editor, either:
- Blocks module having its own separate state
- Merging more of [the non-post-editor-specific-] editor and blocks
Obvious too by the fact that we have equivalent action creators for what we're dispatching here, but unable (unwilling) to import them directly from editor.
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.
Agreed, and I also have no ideas on how to address this right now.
Merging more of [the non-post-editor-specific-] editor and blocks
Could you elaborate on this? I'm not quite caught up on my history here. Did we abandon the effort to merge the two modules (#2795)?
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.
Just an idea: Does it make more sense for this block to be defined in the editor
module instead?
I also even wonder if the blocks
module should hold only the blocks API and define all the blocks in the editor
module
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.
Just an idea: Does it make more sense for this block to be defined in the
editor
module instead?
I think there's a lot of merit to that idea.
I also even wonder if the
blocks
module should hold only the blocks API and define all the blocks in theeditor
module
That one I'm not too convinced of right now. I mean, I could picture using editor/library
, but the current situation of having the library sit next to the API makes sense to me, as the core blocks are the canonical blocks: in that sense, they highly complement the API (both code and docs) by providing numerous well tested use cases.
category: 'reusable-blocks', | ||
isPrivate: true, | ||
|
||
attributes: { |
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 don't think you should need to define this on the client, since it should be bootstrapped from the server definition.
See #2529
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'm hesitant to do this because removing the client-side definition means our test fixtures aren't able to test parsing a <!-- wp:block ref="358b59ee-bab3-4d6f-8445-e8c6971a5605" /-->
.
components/spinner/index.js
Outdated
|
||
export default () => spinner; | ||
function Spinner( { className } ) { |
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.
You're not / no longer taking advantage of these enhancements? Should we omit them respecting our good friend YAGNI?
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 removed these changes.
@@ -350,11 +350,13 @@ class BlockListBlock extends Component { | |||
const { isHovered, isSelected, isMultiSelected, isFirstMultiSelected, focus } = this.props; | |||
const showUI = isSelected && ( ! this.props.isTyping || ( focus && focus.collapsed === false ) ); | |||
const { error } = this.state; | |||
const isReusableBlock = blockType.name === 'core/block'; |
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.
Elsewhere we've limited our awareness to specific block types via abstraction, e.g. getDefaultBlockName
, getUnknownTypeHandlerName
. Should we need to consider that here? Maybe not the case we'd ever expect it to change.
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.
Couldn't hurt! I moved this logic out to isReusableBlock
in 95a28a37adea3afeb44fb2b9c068c9e1fc61fb31.
import { convertBlockToStatic, convertBlockToReusable } from '../../actions'; | ||
|
||
export function ReusableBlockToggle( { block, convertToStatic, convertToReusable } ) { | ||
const isReusableBlock = block.name === 'core/block'; |
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.
Thinking purely from the presentational / container distinction, I might expect this variable to be passed as the prop, determined in a selector or mapStateToProps
, particularly since we're not otherwise using the block
prop.
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 moved it in 95a28a37adea3afeb44fb2b9c068c9e1fc61fb31.
@@ -71,6 +71,10 @@ export class InserterMenu extends Component { | |||
this.switchTab = this.switchTab.bind( this ); | |||
} | |||
|
|||
componentDidMount() { | |||
this.props.fetchReusableBlocks(); |
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.
Noting that this can become quite noisy for AJAX requests: Every time the user opens an inserter.
Personally I'm okay with this, since it ensures that the data is kept fresh, but worth keeping in mind.
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.
For now I think this is OK. If it's a problem then we can limit it to fetch once when the inserter is opened for the first time.
@noisysocks Before we go ahead with exposing this at wide. Can you remind me why we are not using the ID provided by WP for the post object in |
Adds the UI which displays when a reusable block is asked for its `edit` component. This includes a panel which includes UI that indicates whether a reusable block is loading and lets a user change the name of the block.
Adds the necessary PHP code for rendering a reusable block as HTML on the blog's front-end.
Changes BlockListBlock to display selected reusable blocks with a dashed outline.
Add a block settings menu control which allows users to convert a regular block to a reusable block and convert a reusable block to a regular block.
Fix reusable blocks not loading by looking at the correct path in the state tree (state.reusableBlocks.data) and by adding a trailing slash to the /gutenberg/v1/ API versionString.
Patches the Inserter to display all reusable blocks in a seperate group, and to insert a reusable block when one is clicked.
Distinguish between initialAttributes which are passed into createBlock and attributes which define which attributes a block type supports.
Make the Inserter search both static blocks and reusable blocks.
Moves reusbale blocks from the Blocks tab to a new Saved tab in the Inserter.
This way, just as <!-- wp:image --> inserts an image, <!-- wp:block --> inserts a wp_block.
Ensure that the Save button indicates that the block is being saved.
Move block.name === 'core/block' logic to a new isReusableBlock function.
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.
Per Slack, this looks great. The recent change to server-issued IDs is quite welcome.
Great work here everyone and specially @noisysocks. This is a great feature. |
@mtias — could you please elaborate on this so that I can create an issue? Right now, any user with the |
We need to let authors insert them, but probably not edit them by default. Not sure if we should let authors create them, or only editors. Furthermore, this is something that should probably be filterable and part of general capabilities so people can map. Example: |
@mtias ... something more broad. There indeed needs to be a complete capabilities- and author-concept for reusable blocks. A current users with a role below "editor" should only be able to see/create/edit/delete his/her own reusable blocks. Users with/above "editor" role should be able to see/create/edit/delete any reusable block and also edit its author. The concept should also provide a way to configure the visibility of reusable blocks for each user and/or post_type, not sure if via capability or filter. Example for no visibility limit requirement: Simple one-user blog. Example for visibility limit requirement: Multi author website with 15-20 authors and everybody creates a few reusable blocks which would then be visible for everybody, the gui would be a mess in no time... |
Adds the UI necessary for reusable blocks. This includes:
Some of this was already reviewed in #2659.