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

[Search Block]: Add color support to search button #32416

Merged

Conversation

stacimc
Copy link
Contributor

@stacimc stacimc commented Jun 2, 2021

Description

This PR adds color support to the button in the Search block.

Note: Initially I included support for setting colors in theme.json, for example like:

"core/search": {
	"color": {
		"text": "blue",
		"background": "yellow"
	}
}

I decided to remove this, so it is no longer included in this PR. Using __experimentalSkipSelector to support this would cause problems down the road when adding other block supports that don't need skip serialization (eg width/typography). Issue #32417 opened to discuss.

How has this been tested?

Manually.

Test Setup
Note your theme will need to opt into color support in its theme.json.

Test Instructions

  1. Create a post, add a search block.
  2. Select the search block and confirm color controls appear in the sidebar.
  3. Test changing the text and background color.
  4. Save the post and confirm the colors are visually correct on the frontend.

Screenshots

Screen Capture on 2021-06-02 at 15-59-47

Types of changes

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • I've tested my changes with keyboard and screen readers.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR (please manually search all *.native.js files for terms that need renaming or removal).

@stacimc stacimc requested a review from ajitbohra as a code owner June 2, 2021 23:01
@ntsekouras ntsekouras added the [Type] Enhancement A suggestion for improvement. label Jun 3, 2021
@ntsekouras ntsekouras requested a review from carolinan June 3, 2021 09:36
@ntsekouras ntsekouras added the [Block] Search Affects the Search Block - used to display a search field label Jun 3, 2021
const renderButton = () => {
return (
<>
{ buttonUseIcon && (
<Button
icon={ search }
className="wp-block-search__button"
style={ { borderRadius } }
className={ getButtonClassNames() }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we apply classes and styles for the colors both to the wrapper and to the richtext?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here I don't apply classes and styles to the wrapper, just to the button -- but it is applied to both the icon-only button and to the richtext, so that colors display correctly for both the text and the icon-only button.


// Text color.
$has_custom_text_color = ! empty( $attributes['style']['color']['text'] );
if ( ! empty( $attributes['textColor'] ) ) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style choice: can we have both $has_custom_text_color and $has_preset_text_color? Same for background below.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done! 👍

@carolinan
Copy link
Contributor

carolinan commented Jun 4, 2021

How do you style the color of the visible label if the options apply to the button? Would it not be better to use the actual button block with all the style options it already comes with?

@aaronrobertshaw
Copy link
Contributor

How do you style the color of the visible label if the options apply to the button? Would it not be better to use the actual button block with all the style option it already comes with.

There may have been some issues with using the button as an inner block within the search block. @apeatling I believe this was something that you looked into in more detail with your PR adding the icon only/expandable search field?

Regarding the search label color, we could add that using the withColors hook as it stands now however that would not appear within the colors sidebar panel. Not to mention be a little back-to-front.

It would be great if the colors block support was configurable such that we could specify sets of colors. In this case, the default text/background for the block as a whole, then one set for the button.

@apeatling
Copy link
Contributor

I haven't done anything with innerBlocks in my PR with the icon only option. The argument previously was that it was a lot of extra UI for something as simple as a search field and button. Maybe that argument doesn't stand anymore with all the extra button styling options?

@carolinan
Copy link
Contributor

It is probably worth getting button color support in while we consider the inner block approach and also improve the icon usage (allow icons in button block, icon selection for navigation and search block).

@stacimc
Copy link
Contributor Author

stacimc commented Jun 7, 2021

In addition to what others have said, another concern with moving the button to InnerBlocks is managing the border radius controls. Currently border-radius selections apply to both the search input and the button automatically. This is particularly important because we can automatically adjust the border-radius of the wrapper when the button is placed inside the search input to avoid 'fat corners' (#30227), rather than relying on the user to know they must make this adjustment.

@stacimc stacimc force-pushed the add/color-support-to-search-block branch from 3e7b420 to 045407b Compare June 7, 2021 18:09
Copy link
Contributor

@aaronrobertshaw aaronrobertshaw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In terms of editing a search block's button colors this works as advertised for me. It could benefit from whichever solution is created through #33255 which aims to allow greater control in applying styles to inner elements.

While the discussion around the best approach to solving that continues, this PR will need a rebase due to some recent changes to the search block landing.

@stacimc stacimc force-pushed the add/color-support-to-search-block branch from df0a7c9 to 7b53b63 Compare July 21, 2021 19:34
@stacimc
Copy link
Contributor Author

stacimc commented Jul 21, 2021

Thanks @aaronrobertshaw -- agreed, I'll keep an eye on #33255. In the meantime I've rebased and merged in the border support changes 👍

@aaronrobertshaw aaronrobertshaw self-requested a review July 26, 2021 05:26
Copy link
Contributor

@aaronrobertshaw aaronrobertshaw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @stacimc for bringing this PR up-to-date 👍

I've just taken this for a more detailed test drive and found an issue. My apologies for not spotting this the first time around but there is a typo in the search block's gradient support flag. That meant the gradient controls weren't showing for the background color and in a way obscured that we're missing generating gradient CSS classes and styles in the server side rendering.

Other than the gradient issues, this is working nicely. After those and a couple of linting errors in the index.php are fixed up, we'll be ready to merge 🚢 .

packages/block-library/src/search/block.json Outdated Show resolved Hide resolved
packages/block-library/src/search/index.php Outdated Show resolved Hide resolved
packages/block-library/src/search/index.php Show resolved Hide resolved
@stacimc
Copy link
Contributor Author

stacimc commented Jul 26, 2021

That meant the gradient controls weren't showing for the background color and in a way obscured that we're missing generating gradient CSS classes and styles in the server side rendering.

Yikes, that's exactly what happened! Thanks for the catch @aaronrobertshaw.

@aaronrobertshaw aaronrobertshaw self-requested a review July 27, 2021 07:16
Copy link
Contributor

@aaronrobertshaw aaronrobertshaw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding the handling of gradients so quickly @stacimc! 🚀

This is working well for me now. In addition to re-running previous tests, adding both custom and named gradients to the button works in the editor and frontend.

Note: Initially I included support for setting colors in theme.json

Regarding styling the button via the theme.json, one option will be to extend the Elements API. If we add the button HTML element to its whitelist, we should be able to target it easily enough. That will best be suited to its own PR. So with that in mind I think this is good to merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Search Affects the Search Block - used to display a search field [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants