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 search block #13583

Merged
merged 15 commits into from Feb 7, 2019

Conversation

@noisysocks
Copy link
Member

noisysocks commented Jan 30, 2019

Continuation of #4501.
Closes #1800.
Implements the design in #1800 (comment).

Adds a Search block allowing users to place a search field anywhere they like.

search

  • Users can customise the label by editing it inline
  • Users can customise the placeholder text using the block inspector
  • Our own custom markup is used instead of get_search_form(), see #4501 (review) for context

Props to @bamadesigner for all her hard work on this in #4501.

To test

  1. Create a new post
  2. Insert the Search block
  3. Publish the post
  4. Type a query into the search field you just added
  5. Press Enter

@noisysocks noisysocks added this to the 5.1 (Gutenberg) milestone Jan 30, 2019

@noisysocks noisysocks requested review from talldan , Soean and gziolo Jan 30, 2019

@noisysocks noisysocks referenced this pull request Jan 30, 2019

Closed

Adding core search widget block #4501

0 of 3 tasks complete

@noisysocks noisysocks force-pushed the add/search-block branch from 7e82e51 to 5778c3c Jan 30, 2019

@gziolo gziolo requested a review from ajitbohra Jan 30, 2019

@Soean
Copy link
Member

Soean left a comment

Just did a short test, works great. Just one improvement: If I add a custom class, it is not added to the form.

@gziolo
Copy link
Member

gziolo left a comment

Let's also update CHANGELOG.md and request minor version update for the next package release.

We discussed that yesterday during Core JS weekly chat: https://make.wordpress.org/core/2019/01/29/javascript-chat-summary-january-29-2019/

@Soean - I realized we didn't do it for RSS block, so it would be good to open a separate PR and fix that.


return (
<Fragment>
<div className="wp-block-search">

This comment has been minimized.

@gziolo

gziolo Jan 30, 2019

Member

Should it be also using form to ensure that CSS is properly applied if someone uses a tag name specific selectors?

This comment has been minimized.

@noisysocks

noisysocks Jan 31, 2019

Author Member

Is that likely? We already have the problem of the <label> being a <div> because of some RichText. Making this a <form> means that we'd have to add a onSubmit={ ( event ) => event.stopPropagation() } which I find a little distracting.

This comment has been minimized.

@gziolo

gziolo Jan 31, 2019

Member

I don't know what is the best way moving forward, I just wanted to raise this so we could discuss and come up with something solid as this is going to be replicated in other places :)

This comment has been minimized.

@noisysocks

noisysocks Jan 31, 2019

Author Member

😄 I prefer to leave it how it is. We ought to encourage edit() and save() having whatever markup is appropriate for the task at hand. Semantically, what we have here is not a form.

@gziolo

This comment has been minimized.

Copy link
Member

gziolo commented Jan 30, 2019

Yes, it is close to being ready in my opinion. If we apply changes by the end of the week we can move it to 5.0 (Gutenberg) milestone 🎉

@talldan
Copy link
Contributor

talldan left a comment

I had a couple of minor nitpicks, but this looks good 😄

Show resolved Hide resolved packages/block-library/src/search/edit.js Outdated
Show resolved Hide resolved packages/block-library/src/search/index.js Outdated
Show resolved Hide resolved packages/block-library/src/search/edit.js Outdated
return (
<Fragment>
<div className="wp-block-search">
<RichText

This comment has been minimized.

@talldan

talldan Jan 30, 2019

Contributor

I think this should have an aria-label—other blocks seem to achieve that by using the placeholder prop. Also perhaps multiline={ false }?

edit: multiline={ false } doesn't seem to prevent multiple lines. Bug?

This comment has been minimized.

@noisysocks

noisysocks Jan 31, 2019

Author Member

Good catch—added the placeholder prop so that there's a label.

#13570 is a ticket that tracks adding multiline={ false } which looks like it was removed a while ago. For now, I think it's fine to support line breaks—I can't think of any reason we ought to forbid them! 🙂

This comment has been minimized.

@talldan

talldan Jan 31, 2019

Contributor

Was sure I could remember multiline={ false } working in the editor, but I must have dreamed it 😴. My feeling is that it's a bit unusual for a label to be multiple lines.

Thanks for the fixes, will re-review in a bit.

This comment has been minimized.

@noisysocks

noisysocks Jan 31, 2019

Author Member

You didn't dream it! 😀 I think that we removed support for multiline={ false } when refactoring how RichText works in the lead up to 5.0.

I did a little audit of line breaks in the Search block's attributes and I'm OK with how it is currently:

  • label: <br>s are allowed in a <label> so we permit it. This is consistent with e.g. captions in the Image block.
  • placeholder: <br>s or \ns are not allowed in a placeholder="" so we forbid it.
  • buttonText: <br>s are allowed in a <button> so we permit it. This is consistent with e.g. the button text in a Button or File block.
@gziolo

This comment has been minimized.

Copy link
Member

gziolo commented Jan 30, 2019

@michelleweber would you mind helping to polish translations proposed?

@afercia

This comment has been minimized.

Copy link
Contributor

afercia commented Jan 30, 2019

Adding the Accessibility label in addition to the "needs..." one so the team can search for all the widget-blocks related issues in an easier way.

@afercia

This comment has been minimized.

Copy link
Contributor

afercia commented Jan 30, 2019

It would be great for this block to not encourage the use of placeholders as replacement for labels, see https://core.trac.wordpress.org/ticket/40331

So instead of a default Enter a search keyword or phrase I'd consider to either:

  • leave it empty by default and keep the placeholder setting in the sidebar (even if the sidebar is not ideal for the reason @gziolo mentioned)
  • or use a default Optional placeholder... which of course is not rendered in the front end, and explore to make the field editable as @gziolo proposed, thus removing the setting from the sidebar

More importantly:
I'm not sure why this block is removing the Search button. The classic Search widget prints out a search button and that's recommended and expected.

On top: the new search block, followed by the classic search widget:

screenshot 2019-01-30 at 14 54 26

For accessibility a search form should always have a button

@michelleweber

This comment has been minimized.

Copy link

michelleweber commented Jan 30, 2019

Sure, you can specify what bits exactly you'd like reviewed?

@melchoyce

This comment has been minimized.

Copy link
Contributor

melchoyce commented Jan 30, 2019

Looking at some major sites (Google, Amazon, Twitter, Amazon) their search fields don't have a visible label — are they doing something else to make it a11y-compliant?

Quick idea to include a button, based on how some of the aforementioned sites do it:

image

@melchoyce melchoyce added this to In Progress in Porting widgets to blocks Jan 30, 2019

@ZebulanStanphill

This comment has been minimized.

Copy link
Contributor

ZebulanStanphill commented Jan 30, 2019

@melchoyce They may be putting aria-labels on their search bars. However, this is less-than-ideal; visible labels should always be preferred.

On that note, I think the search button should have a visible label as well... actually, why not allow the user to edit the button text as well?

@melchoyce

This comment has been minimized.

Copy link
Contributor

melchoyce commented Jan 30, 2019

If the search button had a visible label, what would be ideal? I don't think it makes sense for us to use Search as an input label, and a button label.

@afercia

This comment has been minimized.

Copy link
Contributor

afercia commented Jan 30, 2019

I'd tend to think the visible label should be in the hands of the authors. Gutenberg shouldn't force a search field with no visible, properly associated, <label> element over an aria-label. That would mean forcing a decreased level of accessibility.

If authors opt to not have a visible label, then that's their responsibility.

Ideally, in the same way Gutenberg does for the headings hierarchy and color contrast, a visible label should be the recommended option. Authors should be educated, not forced to follow an opinionated choice.

@ZebulanStanphill

This comment has been minimized.

Copy link
Contributor

ZebulanStanphill commented Jan 30, 2019

@melchoyce I guess "Submit", "Go", or "Enter" would be fine defaults if "Search" isn't used for the button label. Alternatively, the button could be labeled "Search" and the label preceding the search input could be "Search this site" or something like that by default. Whatever the case, both the preceding label and button should be editable.

@afercia
Copy link
Contributor

afercia left a comment

Please see previous comment about the placeholder value.

@gziolo

This comment has been minimized.

Copy link
Member

gziolo commented Feb 1, 2019

Please can you use the github feature for 'Requesting changes'.

Will try. I tend to not use it because I feel like it's a bit intrusive.

It shouldn't feel like that. It gives a clear message that there are issues that are blocking merging PR.

@gziolo gziolo dismissed their stale review Feb 1, 2019

It looks like the issues I raised were addressed

@talldan

This comment has been minimized.

Copy link
Contributor

talldan commented Feb 1, 2019

There are precedents, for example the images alt attribute

I don't believe it's the same thing. The image alt value usage is correct, because if I upload an image of say, a house, and set the alt as "Image of a house", the alt value is correct in describing the image.

In this case, if the placeholder in the editor says "Search for films", it's incorrect because the user cannot search for films.

@afercia

This comment has been minimized.

Copy link
Contributor

afercia commented Feb 1, 2019

Right. The user can't search at all. But also the value "Search for films" is incorrect. Because the user can't search for the terms "Search for films" 🙂

How this could be explained in plain words? Ideally, it should make clear "this is the placeholder text that will be used in the front end". But it's getting a bit complicated 🙂

@noisysocks

This comment has been minimized.

Copy link
Member Author

noisysocks commented Feb 3, 2019

If it is not possible / desirable to save "Find Movies..." as the actual placeholder value, would it be possible to remove the placeholder attribute when there is a value? Not saying it would be correct semantically but at least it would be a bit less confusing.

In f3475f5 I've changed the placeholder input so that its placeholder property is removed when there is a value. The input still has an aria-label of 'Optional placeholder text' which I think is pretty clear.

@mapk

This comment has been minimized.

Copy link
Contributor

mapk commented Feb 5, 2019

Just tested, and it feels and works great from a design view. I'm able to delete the label in my search block which is cool. The placeholder worked as expected. The button editing was smooth as can be. Thanks for pushing this forward!

@gziolo

This comment has been minimized.

Copy link
Member

gziolo commented Feb 6, 2019

@Soean and @afercia - it looks like your issues were resolved, can you review again?

@gziolo

gziolo approved these changes Feb 6, 2019

Copy link
Member

gziolo left a comment

Let's give some time @afercia and @Soean to confirm their feedback was addressed and let's get this in 🎉

@afercia

This comment has been minimized.

Copy link
Contributor

afercia commented Feb 6, 2019

Looks better from an accessibility perspective, thank you. Note: I see a warning:
Warning: Received falsefor a non-boolean attributeplaceholder.

because the placeholder gets actually evaluated to false. Should probably use a ternary that returns undefined when the input has value.

@noisysocks

This comment has been minimized.

Copy link
Member Author

noisysocks commented Feb 6, 2019

Good catch @afercia! Fixed in dffa8d3.

@noisysocks noisysocks dismissed stale reviews from Soean and afercia Feb 6, 2019

Have confirmed that setting a custom class works correctly

@noisysocks

This comment has been minimized.

Copy link
Member Author

noisysocks commented Feb 7, 2019

Will merge this as it has two approvals and the remaining issues identified by @afercia and @Soean have been addressed. As always, we can use follow-up PRs to make further changes.

Thanks all for reviewing! And thanks to @bamadesigner for getting this ball rolling! ✌️❤️

@noisysocks noisysocks merged commit 8181261 into master Feb 7, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@noisysocks noisysocks deleted the add/search-block branch Feb 7, 2019

@noisysocks noisysocks moved this from In Progress to Done in Porting widgets to blocks Feb 7, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment