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: Fix attribute defaults #23777

Merged
merged 2 commits into from Jul 8, 2020
Merged

Conversation

noisysocks
Copy link
Member

Fixes https://core.trac.wordpress.org/ticket/50608.

Prior to #22422, the Search block's label and buttonText attributes defaulted to __( 'Search' ). This meant that adding a Search block to a post or page resulted in <!-- wp:search /--> being inserted into the HTML.

In #22422, the defaults were removed so that Search could use a block.json file. The default values were instead implemented using a default block variation.

This, however, meant that Search blocks added prior to #22422 would render with a blank label and button. This is the bug outlined in the ticket above.

This fix is to default label and buttonText to __( 'Search' ) in the block's render_callback. Note that care must be taken here to permit these attributes to be '' which hides the label and/or button.

Before After
Screen Shot 2020-07-08 at 12 23 49 Screen Shot 2020-07-08 at 12 23 23

To test:

  1. Manually insert <!-- wp:search /--> into a post's markup.
  2. Test that it renders with a label and button text on the frontend.

@noisysocks noisysocks added [Type] Bug An existing feature does not function as intended Backport to WP Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta [Block] Search Affects the Search Block - used to display a search field labels Jul 8, 2020
@noisysocks noisysocks requested a review from gziolo July 8, 2020 02:29
@noisysocks
Copy link
Member Author

noisysocks commented Jul 8, 2020

@ellatrix: This should go in 5.5 Beta 2.

@github-actions
Copy link

github-actions bot commented Jul 8, 2020

Size Change: +4 B (0%)

Total Size: 1.14 MB

Filename Size Change
build/block-editor/index.js 115 kB +4 B (0%)
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.14 kB 0 B
build/annotations/index.js 3.67 kB 0 B
build/api-fetch/index.js 3.39 kB 0 B
build/autop/index.js 2.82 kB 0 B
build/blob/index.js 620 B 0 B
build/block-directory/index.js 7.67 kB 0 B
build/block-directory/style-rtl.css 944 B 0 B
build/block-directory/style.css 945 B 0 B
build/block-editor/style-rtl.css 10.8 kB 0 B
build/block-editor/style.css 10.8 kB 0 B
build/block-library/editor-rtl.css 7.54 kB 0 B
build/block-library/editor.css 7.54 kB 0 B
build/block-library/index.js 130 kB 0 B
build/block-library/style-rtl.css 7.75 kB 0 B
build/block-library/style.css 7.76 kB 0 B
build/block-library/theme-rtl.css 728 B 0 B
build/block-library/theme.css 729 B 0 B
build/block-serialization-default-parser/index.js 1.88 kB 0 B
build/block-serialization-spec-parser/index.js 3.1 kB 0 B
build/blocks/index.js 48.2 kB 0 B
build/components/index.js 198 kB 0 B
build/components/style-rtl.css 15.8 kB 0 B
build/components/style.css 15.8 kB 0 B
build/compose/index.js 9.56 kB 0 B
build/core-data/index.js 11.4 kB 0 B
build/data-controls/index.js 1.29 kB 0 B
build/data/index.js 8.46 kB 0 B
build/date/index.js 5.38 kB 0 B
build/deprecated/index.js 772 B 0 B
build/dom-ready/index.js 569 B 0 B
build/dom/index.js 3.23 kB 0 B
build/edit-navigation/index.js 10.8 kB 0 B
build/edit-navigation/style-rtl.css 1.08 kB 0 B
build/edit-navigation/style.css 1.08 kB 0 B
build/edit-post/index.js 304 kB 0 B
build/edit-post/style-rtl.css 5.57 kB 0 B
build/edit-post/style.css 5.57 kB 0 B
build/edit-site/index.js 16.6 kB 0 B
build/edit-site/style-rtl.css 3.03 kB 0 B
build/edit-site/style.css 3.03 kB 0 B
build/edit-widgets/index.js 9.35 kB 0 B
build/edit-widgets/style-rtl.css 2.45 kB 0 B
build/edit-widgets/style.css 2.45 kB 0 B
build/editor/editor-styles-rtl.css 537 B 0 B
build/editor/editor-styles.css 539 B 0 B
build/editor/index.js 45 kB 0 B
build/editor/style-rtl.css 3.78 kB 0 B
build/editor/style.css 3.78 kB 0 B
build/element/index.js 4.65 kB 0 B
build/escape-html/index.js 733 B 0 B
build/format-library/index.js 7.71 kB 0 B
build/format-library/style-rtl.css 547 B 0 B
build/format-library/style.css 548 B 0 B
build/hooks/index.js 2.13 kB 0 B
build/html-entities/index.js 622 B 0 B
build/i18n/index.js 3.56 kB 0 B
build/is-shallow-equal/index.js 709 B 0 B
build/keyboard-shortcuts/index.js 2.52 kB 0 B
build/keycodes/index.js 1.94 kB 0 B
build/list-reusable-blocks/index.js 3.12 kB 0 B
build/list-reusable-blocks/style-rtl.css 476 B 0 B
build/list-reusable-blocks/style.css 476 B 0 B
build/media-utils/index.js 5.32 kB 0 B
build/notices/index.js 1.79 kB 0 B
build/nux/index.js 3.4 kB 0 B
build/nux/style-rtl.css 671 B 0 B
build/nux/style.css 668 B 0 B
build/plugins/index.js 2.56 kB 0 B
build/primitives/index.js 1.4 kB 0 B
build/priority-queue/index.js 789 B 0 B
build/redux-routine/index.js 2.85 kB 0 B
build/rich-text/index.js 13.9 kB 0 B
build/server-side-render/index.js 2.71 kB 0 B
build/shortcode/index.js 1.7 kB 0 B
build/token-list/index.js 1.28 kB 0 B
build/url/index.js 4.06 kB 0 B
build/viewport/index.js 1.85 kB 0 B
build/warning/index.js 1.13 kB 0 B
build/wordcount/index.js 1.17 kB 0 B

compressed-size-action

Copy link
Contributor

@talldan talldan left a comment

Choose a reason for hiding this comment

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

Looks good, I tested and confirmed it fixes the issue. Code change is small but clear.

Is it worth keeping the variation after this?

packages/block-library/src/search/index.php Outdated Show resolved Hide resolved
Prior to d26842b, the Search block's label and buttonText attributes
defaulted to `__( 'Search' )`. This meant that adding a Search block to
a post or page resulted in `<!-- wp:search /-->` being inserted into the
HTML.

In d26842b, the defaults were removed so that Search could use a
block.json file. The default values were instead implemented using a
default block variation.

This, however, meant that Search blocks added prior to d26842b would
render with a blank label and button.

This fix is to default label and buttonText to `__( 'Search' )` in the
render_callback. Care must be taken here to permit these attributes to
be '' in which case the label and/or button are hidden.
@noisysocks noisysocks force-pushed the fix/default-search-attributes branch from ab0133f to 9f451a1 Compare July 8, 2020 05:38
@noisysocks
Copy link
Member Author

Is it worth keeping the variation after this?

I did wonder this. We could remove the variation but then would have to change the frontend to display __( 'Search' ) in the editor when attributes.label === undefined and setAttributes( { label: undefined } ) when label === __( 'Search' ). This feels pretty cumbersome to me.

@talldan talldan merged commit 965a5ad into master Jul 8, 2020
@talldan talldan deleted the fix/default-search-attributes branch July 8, 2020 08:46
@github-actions github-actions bot added this to the Gutenberg 8.6 milestone Jul 8, 2020
youknowriad pushed a commit that referenced this pull request Jul 13, 2020
* Search: Fix attribute defaults

Prior to d26842b, the Search block's label and buttonText attributes
defaulted to `__( 'Search' )`. This meant that adding a Search block to
a post or page resulted in `<!-- wp:search /-->` being inserted into the
HTML.

In d26842b, the defaults were removed so that Search could use a
block.json file. The default values were instead implemented using a
default block variation.

This, however, meant that Search blocks added prior to d26842b would
render with a blank label and button.

This fix is to default label and buttonText to `__( 'Search' )` in the
render_callback. Care must be taken here to permit these attributes to
be '' in which case the label and/or button are hidden.

* Try to trick GitHub into re-running tests
@youknowriad youknowriad removed the Backport to WP Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label Jul 13, 2020
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] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants