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 block - minor refactor to classic menu conversion code #43081

Merged
merged 1 commit into from
Aug 15, 2022
Merged

Navigation block - minor refactor to classic menu conversion code #43081

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
41 changes: 23 additions & 18 deletions packages/block-library/src/navigation/edit/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -231,10 +231,9 @@ function Navigation( {
const isDraftNavigationMenu = navigationMenu?.status === 'draft';

const {
convert,
convert: convertClassicMenu,
status: classicMenuConversionStatus,
error: classicMenuConversionError,
value: classicMenuConversionResult,
} = useConvertClassicToBlockMenu( clientId );

const isConvertingClassicMenu =
Expand Down Expand Up @@ -328,11 +327,7 @@ function Navigation( {
speak( __( 'Classic menu importing.' ) );
}

if (
classicMenuConversionStatus === CLASSIC_MENU_CONVERSION_SUCCESS &&
classicMenuConversionResult
) {
handleUpdateMenu( classicMenuConversionResult?.id );
if ( classicMenuConversionStatus === CLASSIC_MENU_CONVERSION_SUCCESS ) {
showClassicMenuConversionNotice(
__( 'Classic menu imported successfully.' )
);
Expand All @@ -345,11 +340,7 @@ function Navigation( {
);
speak( __( 'Classic menu import failed.' ) );
}
}, [
classicMenuConversionStatus,
classicMenuConversionResult,
classicMenuConversionError,
] );
}, [ classicMenuConversionStatus, classicMenuConversionError ] );

// Spacer block needs orientation from context. This is a patch until
// https://github.com/WordPress/gutenberg/issues/36197 is addressed.
Expand Down Expand Up @@ -683,9 +674,15 @@ function Navigation( {
handleUpdateMenu( menuId );
setShouldFocusNavigationSelector( true );
} }
onSelectClassicMenu={ ( classicMenu ) => {
convert( classicMenu.id, classicMenu.name );
setShouldFocusNavigationSelector( true );
onSelectClassicMenu={ async ( classicMenu ) => {
const navMenu = await convertClassicMenu(
classicMenu.id,
classicMenu.name
);
if ( navMenu ) {
handleUpdateMenu( navMenu.id );
setShouldFocusNavigationSelector( true );
}
} }
onCreateEmpty={ () => createNavigationMenu( '', [] ) }
/>
Expand All @@ -707,9 +704,17 @@ function Navigation( {
handleUpdateMenu( menuId );
setShouldFocusNavigationSelector( true );
} }
onSelectClassicMenu={ ( classicMenu ) => {
convert( classicMenu.id, classicMenu.name );
setShouldFocusNavigationSelector( true );
onSelectClassicMenu={ async ( classicMenu ) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be worth extracting this to a shared method since it is the same as the above event handler?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for reviewing. The final code is different in #42956, where there will be an extra line in one of the callbacks.

Extracting to another function is still possible, but I'm not sure it's worth it for only a few lines. I personally prefer the improved readability gained by not having to scroll to the definition.

const navMenu = await convertClassicMenu(
classicMenu.id,
classicMenu.name
);
if ( navMenu ) {
handleUpdateMenu( navMenu.id );
setShouldFocusNavigationSelector(
true
);
}
} }
onCreateNew={ resetToEmptyBlock }
/* translators: %s: The name of a menu. */
Expand Down
68 changes: 31 additions & 37 deletions packages/block-library/src/navigation/edit/use-convert-classic-menu-to-block-menu.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@ function useConvertClassicToBlockMenu( clientId ) {
const registry = useRegistry();

const [ status, setStatus ] = useState( CLASSIC_MENU_CONVERSION_IDLE );
const [ value, setValue ] = useState( null );
const [ error, setError ] = useState( null );

async function convertClassicMenuToBlockMenu( menuId, menuName ) {
Expand Down Expand Up @@ -88,47 +87,42 @@ function useConvertClassicToBlockMenu( clientId ) {
return navigationMenu;
}

const convert = useCallback(
( menuId, menuName ) => {
if ( ! menuId || ! menuName ) {
setError( 'Unable to convert menu. Missing menu details.' );
const convert = useCallback( async ( menuId, menuName ) => {
if ( ! menuId || ! menuName ) {
setError( 'Unable to convert menu. Missing menu details.' );
setStatus( CLASSIC_MENU_CONVERSION_ERROR );
return;
}

setStatus( CLASSIC_MENU_CONVERSION_PENDING );
setError( null );

return await convertClassicMenuToBlockMenu( menuId, menuName )
.then( ( navigationMenu ) => {
setStatus( CLASSIC_MENU_CONVERSION_SUCCESS );
return navigationMenu;
} )
.catch( ( err ) => {
setError( err?.message );
setStatus( CLASSIC_MENU_CONVERSION_ERROR );
return;
}

setStatus( CLASSIC_MENU_CONVERSION_PENDING );
setValue( null );
setError( null );

convertClassicMenuToBlockMenu( menuId, menuName )
.then( ( navMenu ) => {
setValue( navMenu );
setStatus( CLASSIC_MENU_CONVERSION_SUCCESS );
} )
.catch( ( err ) => {
setError( err?.message );
setStatus( CLASSIC_MENU_CONVERSION_ERROR );

// Rethrow error for debugging.
throw new Error(
sprintf(
// translators: %s: the name of a menu (e.g. Header navigation).
__( `Unable to create Navigation Menu "%s".` ),
menuName
),
{
cause: err,
}
);
} );
},
[ clientId ]
);

// Rethrow error for debugging.
throw new Error(
sprintf(
// translators: %s: the name of a menu (e.g. Header navigation).
__( `Unable to create Navigation Menu "%s".` ),
menuName
),
{
cause: err,
}
);
} );
}, [] );

return {
convert,
status,
value,
error,
};
}
Expand Down