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.

Sign up for GitHub

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

Try: hide search block label when contained #35688

Closed
wants to merge 4 commits into from
Closed

Try: hide search block label when contained #35688

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
707a2bb
Try: hide search block label when contained
vcanales Oct 15, 2021
db3d43d
code style fix
vcanales Oct 15, 2021
8ee78b6
Add comment for usage of useRef
vcanales Oct 15, 2021
0645bf6
regenerate fixtures
vcanales Oct 15, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion packages/block-library/src/navigation/block.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,10 @@
},
"customOverlayTextColor": {
"type": "string"
},
"searchBlockLabel": {
Copy link
Contributor

Choose a reason for hiding this comment

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

It doesn't feel right to have this as an attribute of the Navigation block, because it's not configurable within Navigation. Block attributes should be related to the block itself, usually as settings that affect the block or its children, but in this case we're trying to configure an attribute that only exists in the child block. The parent block shouldn't really need to know about it.

There's a precedent for this type of situation in Page List, where we check for a block parent directly from its edit function. This seems a more appropriate approach for the search label, and has the advantage that we can make it apply to other parent blocks without having to add attributes and context to each individual block (though I'm not sure if in this case we'd want this behaviour when Search is nested in any other block apart from Navigation)

Copy link
Contributor

Choose a reason for hiding this comment

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

Update: #31670 changed Page List to extrapolate its parent from context, but that works because Page List already uses context from Navigation, which Search doesn't.

"type": "boolean",
"default": false
}
},
"providesContext": {
Expand All @@ -78,7 +82,8 @@
"showSubmenuIcon": "showSubmenuIcon",
"openSubmenusOnClick": "openSubmenusOnClick",
"style": "style",
"orientation": "orientation"
"orientation": "orientation",
"searchBlockLabel": "searchBlockLabel"
},
"supports": {
"align": [
Expand Down
5 changes: 4 additions & 1 deletion packages/block-library/src/search/block.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,5 +53,8 @@
"html": false
},
"editorStyle": "wp-block-search-editor",
"style": "wp-block-search"
"style": "wp-block-search",
"usesContext": [
"searchBlockLabel"
]
}
23 changes: 23 additions & 0 deletions packages/block-library/src/search/edit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import {
__experimentalUseBorderProps as useBorderProps,
__experimentalUnitControl as UnitControl,
__experimentalUseColorProps as useColorProps,
store as blockEditorStore,
} from '@wordpress/block-editor';
import {
ToolbarDropdownMenu,
Expand All @@ -27,6 +28,8 @@ import {
__experimentalUseCustomUnits as useCustomUnits,
} from '@wordpress/components';
import { useInstanceId } from '@wordpress/compose';
import { useSelect } from '@wordpress/data';
import { useRef } from '@wordpress/element';
import { Icon, search } from '@wordpress/icons';
import { __ } from '@wordpress/i18n';

Expand Down Expand Up @@ -54,6 +57,8 @@ const DEFAULT_INNER_PADDING = '4px';

export default function SearchEdit( {
className,
clientId,
context,
attributes,
setAttributes,
toggleSelection,
Expand All @@ -76,6 +81,24 @@ export default function SearchEdit( {
const borderColor = style?.border?.color;
const borderProps = useBorderProps( attributes );

const isNewBlock = useSelect( ( select ) => {
const { wasBlockJustInserted } = select( blockEditorStore );

return wasBlockJustInserted( clientId );
} );

// Save a ref to this value, as it would become outdated
// if any other blocks are added.
const isNewBlockRef = useRef( isNewBlock );
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure I understand why we need to use a ref here. Could you elaborate a bit on the problem?


if (
'undefined' !== typeof context.searchBlockLabel &&
true === isNewBlockRef.current
) {
setAttributes( { showLabel: context.searchBlockLabel } );
isNewBlockRef.current = false;
}

// Check for old deprecated numerical border radius. Done as a separate
// check so that a borderRadius style won't overwrite the longhand
// per-corner styles.
Expand Down
3 changes: 2 additions & 1 deletion test/integration/fixtures/blocks/core__navigation.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,8 @@
"orientation": "horizontal",
"showSubmenuIcon": true,
"openSubmenusOnClick": false,
"overlayMenu": "never"
"overlayMenu": "never",
"searchBlockLabel": false
},
"innerBlocks": [],
"originalContent": ""
Expand Down