-
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
Add the initial version of the Block Patterns API and UI as a sidebar plugin #20354
Conversation
Size Change: +2.04 kB (0%) Total Size: 866 kB
ℹ️ 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.
Do you plan to put Block Patterns API behind the feature flag? I think it should be considered because it seems like a common approach when new features are under development.
This PR still needs design review.
const blocks = useMemo( () => parse( content ), [ content ] ); | ||
|
||
return ( | ||
<div |
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.
Unrelated to this PR, we probably should add more capabilities to the Button
component:
<Button
as="div"
className="block-editor-patterns__item"
onClick={ () => onClick( blocks ) }
>
Behind the scenes, we would handle:
role
onKeyDown
tabIndex
@diegohaz - is it how Reakit handles custom buttons? :)
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 Button has certain design choices in it, I don't see why I should use it 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 Button has certain design choices in it, I don't see why I should use it here.
Such a button would have to live in @wordpress/primitives
, I'm talking about accessibility aspects being sorted out. I forgot about visuals :)
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.
Off the top of my head, this is the list of things you should implement on non-native buttons:
role
tabIndex
onKeyDown
for Enter (trigger the button)onKeyDown
for Space (activate the button, like a mouse down,:active
)onKeyUp
for Space (trigger the button just like Enter)aria-disabled
(should add the attribute and call.preventDefault()
and.stopPropagation
on many events. Settingpointer-events: none
may help with that, although it's not completely accurate).
Except for Space, that is handled like Enter (otherwise the code would be overcomplicated), Reakit's Button does all that when it's not rendered as the native button element. But it's much simpler to just use the native button. Why not use the native button 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.
If you don't plan to show this component disabled, I would say that the implementation is okay though.
} | ||
|
||
function BlockPatterns( { patterns } ) { | ||
const getBlockInsertionPoint = useSelect( ( select ) => { |
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.
Hooks are better at so many levels. I recall how much work was required to pass down selector to action to call it when a callback is executed. Here, you get everything out of the box :)
|
||
registerPlugin( 'edit-post', { | ||
render() { | ||
return ( | ||
<> | ||
<BlockPatterns /> |
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.
Cool idea to use Plugins API. Once we have the design sorted out, we can add a new slot in the inserter and everything should still work.
It'd be good to not focus on any block when a pattern is inserted, the jump and arbitrary focus seems weird. |
This feels really good! I mirror @mtias's comments.
I'm wrapping inside a group block the patterns I'm creating for #20345. My expectation was that focus would go to the group block (the wrapper), I think this would be a more expected result. What do you think? |
Pushed some small tweaks:
Let me know how it feels? |
@mtias It should be fixed now, good catch. |
@@ -0,0 +1,5 @@ | |||
{ | |||
"__file": "wp_block", | |||
"title": "Teams", |
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'll need to consider translations here eventually.
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.
Right! since this is potentially just a temporary location, I'm ok just using php for these for now.
lib/pattterns/teams.php
rootClientId, | ||
false | ||
); | ||
createSuccessNotice( |
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 is working well for me.
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 is looking good for a first pass.
Works as expected. I'm not in love with the |
I just noticed I have to click to SnackBar notices to dismiss them — is that intentional? I expected them to go away on their own after a few seconds. |
They do go away on their own. Are they stuck for you? |
I also noticed that the Testimonials block pattern isn't included in a Group block which is probably fine, but when I wanted to delete it, I found I had to select all the blocks that it placed on the page to delete them. I'd like to select a Group block for a particular pattern and delete it altogether knowing I'm targeting all the blocks that got added to my page. Any thoughts on including all blocks of a pattern into one Group block or other container block when adding them to the page? |
Technically speaking, patterns don't need a wrapper, but I can add it here. |
Let's have one without a wrapper so that we can get a sense for the experience it provides. |
Ah, I was impatient I guess. They do go away, but slower than I was expecting. |
To split the patterns library discussion from the implementation of the UI. I'm considering merging this PR as is to continue on the UI iterations separately and allow designers to submit patterns as well in parallel. We should not let the current patterns sit for a long time though :P |
Where do I find block patterns in 7.7 ? |
Just a note for usability. In mobile phone screen Width 420px after clicking a "pattern" the pattern select modal stays open so there is no visual feedback that the block was inserted. Imho it should close on click. |
Refs #17335
This is the MVP of blocks patterns:
Note I shamelessly stole two block patterns from WPHubDesign plugin while waiting for #20345