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

Navigation: Add an experimental label #54946

Open
wants to merge 4 commits into
base: trunk
Choose a base branch
from
+43 −21
Open

Navigation: Add an experimental label #54946

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
fc4cde1
Navigation: Add an experimental label
scruffian Sep 29, 2023
546e4fa
Update packages/block-library/src/navigation/index.js
scruffian Oct 5, 2023
47f2d01
Update packages/block-library/src/navigation/index.js
scruffian Oct 5, 2023
da821b1
Decode once
getdave Oct 13, 2023
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
25 changes: 25 additions & 0 deletions packages/block-library/src/navigation/build-navigation-label.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
/**
* WordPress dependencies
*/
import { __, sprintf } from '@wordpress/i18n';
import { decodeEntities } from '@wordpress/html-entities';

function buildNavigationLabel( title, id, status ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

let's add a docblock to this function?

if ( ! title ) {
/* translators: %s is the index of the menu in the list of menus. */
return sprintf( __( '(no title %s)' ), id );
}

if ( status === 'publish' ) {
return decodeEntities( title );
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: can we do decodeEntities( title ) once before the if ( status === 'publish' ) {. That way we don't have any chance of refactoring loosing the decoding.

}

return sprintf(
// translators: %1s: title of the menu; %2s: status of the menu (draft, pending, etc.).
__( '%1$s (%2$s)' ),
decodeEntities( title ),
status
);
}

export default buildNavigationLabel;
21 changes: 2 additions & 19 deletions packages/block-library/src/navigation/edit/navigation-menu-selector.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,24 +18,7 @@ import { useEntityProp } from '@wordpress/core-data';
*/
import useNavigationMenu from '../use-navigation-menu';
import useNavigationEntities from '../use-navigation-entities';

function buildMenuLabel( title, id, status ) {
if ( ! title ) {
/* translators: %s is the index of the menu in the list of menus. */
return sprintf( __( '(no title %s)' ), id );
}

if ( status === 'publish' ) {
return decodeEntities( title );
}

return sprintf(
// translators: %1s: title of the menu; %2s: status of the menu (draft, pending, etc.).
__( '%1$s (%2$s)' ),
decodeEntities( title ),
status
);
}
import buildNavigationLabel from '../build-navigation-label';

function NavigationMenuSelector( {
currentMenuId,
Expand Down Expand Up @@ -72,7 +55,7 @@ function NavigationMenuSelector( {
const menuChoices = useMemo( () => {
return (
navigationMenus?.map( ( { id, title, status }, index ) => {
const label = buildMenuLabel(
const label = buildNavigationLabel(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The destructuring of the navigationMenu object in here means we need to make more changes to the code to pass the whole object. I'm not sure it's worth it. By only passing the properties we need we keep the function clearer and less susceptible to side effects.

title?.rendered,
index + 1,
status
Expand Down
10 changes: 10 additions & 0 deletions packages/block-library/src/navigation/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@ import metadata from './block.json';
import edit from './edit';
import save from './save';
import deprecated from './deprecated';
import useNavigationMenu from './use-navigation-menu';
import buildNavigationLabel from './build-navigation-label';

const { name } = metadata;

Expand Down Expand Up @@ -50,6 +52,14 @@ export const settings = {
},
],
},
__experimentalLabel( { ref } ) {
const { navigationMenu } = useNavigationMenu( ref );
return buildNavigationLabel(
navigationMenu?.title,
1, // This has to be 1 because in this context there is only one Navigation shown.
navigationMenu?.status
);
Comment on lines +57 to +61
Copy link
Contributor

Choose a reason for hiding this comment

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

Given this function signature is a little opaque (especially the 2nd argument) I wonder whether it's better just to pass the entire navigationMenu and have the function grab the bit it needs.

I feel perhaps the consumer needs to know too much.

What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just passing the entire thing won't be enough, as we still need to know the index. We could ditch the third argument though.

Copy link
Contributor

Choose a reason for hiding this comment

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

In that case maybe make the function accept an object with named arguments? It might be easier to understand.

Not a blocker though. Just a code quality idea.

},
edit,
save,
deprecated,
Expand Down
Loading