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

Search: Hide label when inserted in Navigation block #36026

Closed
wants to merge 5 commits into from
Closed

Search: Hide label when inserted in Navigation block #36026

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
8e05f25
Search: Hide label when inserted in Navigation block
Mamaduka Oct 28, 2021
b37f9f5
More contextual defaults
Mamaduka Oct 28, 2021
a895df9
Include attributes with default values in check
Mamaduka Oct 28, 2021
d968075
Use getBlockParentsByBlockName
Mamaduka Oct 29, 2021
78ba3d3
Remove context
Mamaduka Oct 29, 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
40 changes: 40 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 { useDispatch, useSelect } from '@wordpress/data';
import { useEffect, useRef } from '@wordpress/element';
import { Icon, search } from '@wordpress/icons';
import { __ } from '@wordpress/i18n';

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

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

const { isNavigationChild } = useSelect(
( select ) => {
const { getBlockParentsByBlockName } = select( blockEditorStore );

return {
isNavigationChild: !! getBlockParentsByBlockName(
clientId,
'core/navigation'
)?.length,
};
},
[ clientId ]
);
const { __unstableMarkNextChangeAsNotPersistent } = useDispatch(
blockEditorStore
);

const hasChangedDefaults = useRef( false );
useEffect( () => {
if ( hasChangedDefaults.current ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This only works when we first insert a Search block in Navigation. If we insert the block, save and reload the page, and then try changing the settings back to the defaults, they just change back 😬

We should probably also return if any of the settings have been edited.

Another potential edge case is already existing Search-within-Nav patterns: if the Search settings are the defaults, they'll be auto-changed, which might not be the desired outcome if the block has already been edited and saved before. I'm wondering if we should do this only for newly-inserted blocks - that would fix the issue of having edited settings reset after page reload too.

Copy link
Member Author

Choose a reason for hiding this comment

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

I added few more checks in a895df9, to mitigate this. But I agree it's not most elegant solution.

I also tried using the wasBlockJustInserted selector, but it wasn't working for some reason. I will give this another go in a moment.

Copy link
Member Author

Choose a reason for hiding this comment

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

@tellthemachines, thanks for the great feedback.

The wasBlockJustInserted selector doesn't work in the Navigation block context. It always returns false for the Search block. I think it's a bug that needs to be addressed separately.

Unfortunately, I'm not aware of another selector to check if the block is new.

As for the edge cases, I'm not sure. Should we consider it a bug we can live with while the wasBlockJustInserted selector issue is fixed?

I will leave that decision to you and @jasmussen.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, this is tricky because wasBlockJustInserted only works for newly inserted blocks, not transformed blocks, and with how Navigation currently works chances are high that folks will be coming to the Search block via a transform.

To mitigate the issue with resetting the block to defaults after reload, we can store refs to the attribute values and return early in the effect if they change. I'm not sure there's an easy way to handle not changing existing blocks though.

Given Navigation is still experimental, perhaps there are not that many existing Navigations with Search blocks using all the default settings out there 😅

@jasmussen what are your thoughts on this?

Copy link
Contributor

Choose a reason for hiding this comment

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

Apologies I missed the first ping!

I took the latest for a quick spin to just check things out, and I'm seeing the correctly-configured-for-nav search block work well:
search

I understand something happens if/when you reload? Or were you referring to existing navigation block that feature search?

Copy link
Contributor

Choose a reason for hiding this comment

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

I was kindly explained that the problem might be that the action that sets the new defaults might fire every time the block loads, and therefore might reset back to the new defaults even if you want something else. Is that correct?

I wasn't actually able to reproduce this — it seems to continue working for me:
bug

But more high level — while this is a really nice one that makes for a better navigation block experience, I wouldn't consider it crucial that this land in 5.9. If we can make it, nice. Otherwise, with 6.0 won't be that far behind!

Copy link
Member Author

Choose a reason for hiding this comment

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

It's is true that the effect will run on every page reload. But the setAttribute will only fire if showLabel and buttonUseIcon attributes use the same value, so this is quite a specific situation.

@jasmussen to reproduce the issue, set button to use text and then enable label. You'll see that block will reset to the insertion state.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah gotcha, so if you basically reset the navigation block to specifically look like the default state, it will reset on reload?

If we think we can fix the underlying bug in a followup and improve that, I'd be tempted to accept that tradeoff to get the feature landed.

Copy link
Member Author

@Mamaduka Mamaduka Oct 29, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

so if you basically reset the navigation block to specifically look like the default state, it will reset on reload?

That's correct.

Copy link
Contributor

@ntsekouras ntsekouras Nov 1, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

so if you basically reset the navigation block to specifically look like the default state, it will reset on reload?

Yes, but most importantly it will update existing Search blocks.

I made an alternative here: #36125, keeping some of this logic from George.

return;
}

if ( isNavigationChild && showLabel && ! buttonUseIcon ) {
// This side-effect should not create an undo level.
__unstableMarkNextChangeAsNotPersistent();
setAttributes( {
showLabel: false,
buttonUseIcon: true,
buttonPosition: 'button-inside',
} );

hasChangedDefaults.current = true;
}
}, [ isNavigationChild, showLabel, buttonUseIcon ] );

// 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