Skip to content
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

Closed
wants to merge 7 commits into from

Conversation

bamadesigner
Copy link

@bamadesigner bamadesigner commented Jan 16, 2018

Description

Adding new search widget block. For #1800

Some thoughts/questions for discussion:

  • How much do we want this to work with get_search_form()?
  • I copy/pasted form markup from get_search_form() because I added 3 fields so users can customize search label, placeholder, and submit button value but this markup isn't editable in get_search_form() at the moment.
  • These fields could be an issue with current theme architecture searchform.php, e.g. need to get the search block attributes to the template files.
  • get_search_form() and its markup used here needs an accessibility check.
  • I don't think the form label (taken from get_search_form()) should be instantly classified as "screen-reader-text". That should be up to the theme on whether or not to hide it from display. I changed class to "search-label" in the admin so it would display.
  • @melchoyce mentioned in the issue to have the search label be set to "Search" but that doesnt match get_search_form() that uses "Search for:".
  • The screenshot in the issue lists the default placeholder text as "Enter a search keyword or phrase" but the placeholder in get_search_form() is "Search …".
  • I added a "get_search_form_args" filter. Really just trying to get in the mindset of updating get_search_form() to be used with the block.
  • I just put in some basic styles for the form in the editor to get started

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:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows has proper inline documentation.

@melchoyce
Copy link
Contributor

@melchoyce mentioned in the issue to have the search label be set to "Search" but that doesnt match get_search_form() that uses "Search for:".
The screenshot in the issue lists the default placeholder text as "Enter a search keyword or phrase" but the placeholder in get_search_form() is "Search …".

Ah, then let's go with the default values for these, seems easier 👍

@ghost
Copy link

ghost commented Jan 16, 2018

How much do we want this to work with get_search_form()?

Just make sure the code can be overridden by or handle integration to

a dedicated search engine, like Elasticsearch

as advised in WordPress search, unexpected results due to Gutenberg serialization markup #3739 as solution for a working search.

@melchoyce melchoyce requested review from a team and westonruter February 10, 2018 19:01
Copy link
Member

@westonruter westonruter left a 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.

$defaults = array(
'label' => _x( 'Search for:', 'label' ),
'placeholder' => esc_attr_x( 'Search …', 'placeholder' ),
'submitValue' => esc_attr_x( 'Search', 'submit button' ),
Copy link
Member

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.

* @since 2.7.0
*
* @param string $form The search form HTML output.
*/
Copy link
Member

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' ),
},
Copy link
Member

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.

Copy link
Member

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:

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(
Copy link
Member

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.

$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" />
Copy link
Member

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()

@danielbachhuber
Copy link
Member

Closing this for now, per #1800 (comment)

This seems like Customization territory, not 5.0.

@melchoyce melchoyce reopened this Dec 14, 2018
@gziolo gziolo added Needs Accessibility Feedback Need input from accessibility New Block Suggestion for a new block [Feature] Blocks Overall functionality of blocks labels Jan 21, 2019
@gziolo gziolo requested review from ajitbohra, Soean and gziolo January 21, 2019 09:56
@gziolo
Copy link
Member

gziolo commented Jan 21, 2019

@bamadesigner Can you rebase it again?
The Gutenberg phase 2 with Widgets to blocks has started and there are volunteers to help with reviews of this PR: https://make.wordpress.org/core/2019/01/17/editor-chat-summary-january-16/.

@gziolo gziolo added this to the 5.1 (Gutenberg) milestone Jan 22, 2019
@youknowriad youknowriad mentioned this pull request Jan 22, 2019
22 tasks
@noisysocks noisysocks self-requested a review January 23, 2019 03:05
@noisysocks
Copy link
Member

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 🙂

Copy link
Member

@gziolo gziolo left a 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';
Copy link
Member

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';
Copy link
Member

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 && (
Copy link
Member

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 {
Copy link
Member

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', {
Copy link
Member

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:

/**
* Server-side rendering of the `core/search` block.
*
* @package gutenberg
Copy link
Member

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' ),
},
Copy link
Member

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:

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 &hellip;', 'placeholder', 'gutenberg' ),
Copy link
Member

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.

@melchoyce melchoyce mentioned this pull request Jan 28, 2019
@noisysocks noisysocks mentioned this pull request Jan 30, 2019
@noisysocks
Copy link
Member

I've picked this up over in #13583. Thanks again @bamadesigner for all your fantastic work on this!

@noisysocks noisysocks closed this Jan 30, 2019
@afercia afercia added the [Feature] Widgets Screen The block-based screen that replaced widgets.php. label Jan 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Blocks Overall functionality of blocks [Feature] Widgets Screen The block-based screen that replaced widgets.php. Needs Accessibility Feedback Need input from accessibility New Block Suggestion for a new block
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants