-
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
Adding core search widget block #4501
Conversation
Ah, then let's go with the default values for these, seems easier 👍 |
Just make sure the code can be overridden by or handle integration to
as advised in WordPress search, unexpected results due to Gutenberg serialization markup #3739 as solution for a working search. |
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 a difficult question on whether to try to re-use get_search_form()
. I think the most important thing is that the preview be faithful to the frontend. This is currently a challenge in Gutenberg for widgets because they have separate implementations between editing (JS) and viewing (PHP). A theme can override the PHP rendering to be something completely different already, which would break the WYSIWYG block editing preview. But this would be less likely than the theme defining a searchform.php
. Since the block here is not going to be used in normal theme template contexts but rather in new Gutenberg contexts, I'm inclined to think that the search block you're defining here should break away from trying to re-use searchform.php
to instead use a new Gutenberg block template, one that leverages all of the new customizability you are introducing. After all, the Latest Posts widget block is not re-using the Recent Posts widget template.
In the Gutenberg world, theme templates are going to need to be rethought anyway in favor of a block templating system, I think. (Especially to eliminate duplicating templates across PHP and JS.) So I don't think we should try too hard here to re-use existing WP theme templates.
blocks/library/search/index.php
Outdated
$defaults = array( | ||
'label' => _x( 'Search for:', 'label' ), | ||
'placeholder' => esc_attr_x( 'Search …', 'placeholder' ), | ||
'submitValue' => esc_attr_x( 'Search', 'submit button' ), |
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.
These strings should have the gutenberg
text domain.
blocks/library/search/index.php
Outdated
* @since 2.7.0 | ||
* | ||
* @param string $form The search form HTML output. | ||
*/ |
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.
Since this is copied from core, this phpdoc can be reduced to:
/** This filter is documented in wp-includes/general-template.php */
submitValue: { | ||
type: 'string', | ||
default: _x( 'Search', 'submit button' ), | ||
}, |
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 strings here are duplicated here in JS and in PHP. It would be ideal if they were all consolidated in the one register_block_type()
PHP call and then exported to JS.
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, attributes should be defined on the server using PHP. It will solve the issues with code duplication. See Latest Posts block for reference:
gutenberg/packages/block-library/src/latest-posts/index.php
Lines 88 to 130 in e8f0f89
function register_block_core_latest_posts() { | |
register_block_type( | |
'core/latest-posts', | |
array( | |
'attributes' => array( | |
'categories' => array( | |
'type' => 'string', | |
), | |
'className' => array( | |
'type' => 'string', | |
), | |
'postsToShow' => array( | |
'type' => 'number', | |
'default' => 5, | |
), | |
'displayPostDate' => array( | |
'type' => 'boolean', | |
'default' => false, | |
), | |
'postLayout' => array( | |
'type' => 'string', | |
'default' => 'list', | |
), | |
'columns' => array( | |
'type' => 'number', | |
'default' => 3, | |
), | |
'align' => array( | |
'type' => 'string', | |
), | |
'order' => array( | |
'type' => 'string', | |
'default' => 'desc', | |
), | |
'orderBy' => array( | |
'type' => 'string', | |
'default' => 'date', | |
), | |
), | |
'render_callback' => 'render_block_core_latest_posts', | |
) | |
); | |
} |
$form = ob_get_clean(); | ||
} else { | ||
|
||
$defaults = 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.
These defaults are duplicated. It would be better of the defaults could be pulled from the PHP-registered block's attributes, per my note above.
blocks/library/search/index.php
Outdated
$form = '<form role="search" method="get" class="search-form" action="' . esc_url( home_url( '/' ) ) . '"> | ||
<label> | ||
<span class="screen-reader-text">' . $args['label'] . '</span> | ||
<input type="search" class="search-field" placeholder="' . $args['placeholder'] . '" value="' . get_search_query() . '" name="s" /> |
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 attribute values here and below should be wrapped in esc_attr()
Closing this for now, per #1800 (comment) This seems like Customization territory, not 5.0. |
@bamadesigner Can you rebase it again? |
Spoke to @bamadesigner who says she's too swamped right now to continue with this. I'll be taking over and will try and get it in for Gutenberg 5.1 🙂 |
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 PR needs to be rebased. Blocks were moved to packages/block-library/src/
so it might require some manual work.
I left my feedback which I hope will help to get this PR up to date and some notes how the comment from @westonruter can be addressed.
/** | ||
* Internal dependencies | ||
*/ | ||
import './editor.scss'; |
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 no longer import SCSS files in JavaScript. There is editor.scss
file where this import should be added instead:
https://github.com/WordPress/gutenberg/blob/master/packages/block-library/src/editor.scss
import './editor.scss'; | ||
import { registerBlockType } from '../../api'; | ||
import InspectorControls from '../../inspector-controls'; | ||
import TextControl from '../../inspector-controls/text-control'; |
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 three imports should be converted to WordPress dependencies.
</label> | ||
<input type="submit" className="search-submit" value={ submitValue } /> | ||
</form>, | ||
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.
focus
was removed and we no longer need it.
render
method might return Fragment
component rather than array which needs to assign key to every child element. form
doesn't have key
provided so it will print warnings on the console.
import InspectorControls from '../../inspector-controls'; | ||
import TextControl from '../../inspector-controls/text-control'; | ||
|
||
class SearchBlock extends Component { |
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.
SearchBlock
component should be moved to its own edit.js
file so it could be overridden by the mobile app when necessary.
}, | ||
}; | ||
|
||
registerBlockType( 'core/search', { |
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 no longer register block types. Instead, we export name
and settings
. See RSS block implementation for reference:
https://github.com/WordPress/gutenberg/blob/e8f0f89c5a954d4d67c3bfac1cfa9ef0edd082ed/packages/block-library/src/rss/index.js
and then it's imported and added in the file which contain registerCoreBlocks
method:
rss, |
/** | ||
* Server-side rendering of the `core/search` block. | ||
* | ||
* @package gutenberg |
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 file is going to be copied to WordPress so it should be WordPress
. See another block for reference:
https://github.com/WordPress/gutenberg/blob/master/packages/block-library/src/categories/index.php#L5
submitValue: { | ||
type: 'string', | ||
default: _x( 'Search', 'submit button' ), | ||
}, |
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, attributes should be defined on the server using PHP. It will solve the issues with code duplication. See Latest Posts block for reference:
gutenberg/packages/block-library/src/latest-posts/index.php
Lines 88 to 130 in e8f0f89
function register_block_core_latest_posts() { | |
register_block_type( | |
'core/latest-posts', | |
array( | |
'attributes' => array( | |
'categories' => array( | |
'type' => 'string', | |
), | |
'className' => array( | |
'type' => 'string', | |
), | |
'postsToShow' => array( | |
'type' => 'number', | |
'default' => 5, | |
), | |
'displayPostDate' => array( | |
'type' => 'boolean', | |
'default' => false, | |
), | |
'postLayout' => array( | |
'type' => 'string', | |
'default' => 'list', | |
), | |
'columns' => array( | |
'type' => 'number', | |
'default' => 3, | |
), | |
'align' => array( | |
'type' => 'string', | |
), | |
'order' => array( | |
'type' => 'string', | |
'default' => 'desc', | |
), | |
'orderBy' => array( | |
'type' => 'string', | |
'default' => 'date', | |
), | |
), | |
'render_callback' => 'render_block_core_latest_posts', | |
) | |
); | |
} |
|
||
$defaults = array( | ||
'label' => _x( 'Search for:', 'label', 'gutenberg' ), | ||
'placeholder' => esc_attr_x( 'Search …', 'placeholder', 'gutenberg' ), |
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.
Translations shouldn't use the domain param as this file is going to be copied to core.
I've picked this up over in #13583. Thanks again @bamadesigner for all your fantastic work on this! |
Description
Adding new search widget block. For #1800
Some thoughts/questions for discussion:
How Has This Been Tested?
Tested in local environment with Twenty Seventeen theme. Had to remove/rename searchform.php template to test front-end search for markup.
Screenshots (jpeg or gifs if applicable):
Types of changes
Checklist: