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

Off Canvas Navigation Editor: Add Convert To Links Modal #45984

Closed
wants to merge 27 commits into from
Closed
Changes from 1 commit
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
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
Prev Previous commit
Next Next commit
Merge branch 'trunk' into add/navigation-off-canvas-edit
Conflicts:
	packages/block-editor/src/components/off-canvas-editor/block-edit-button.js
  • Loading branch information
Alex Lende committed Dec 1, 2022
commit 8e447213eb6a49fb51cb5bef2b87e804408cd824
Original file line number Diff line number Diff line change
@@ -3,7 +3,7 @@
*/
import { edit } from '@wordpress/icons';
import { __ } from '@wordpress/i18n';
import { useMemo, useState } from '@wordpress/element';
import { forwardRef, useMemo, useState } from '@wordpress/element';
import { Button, Modal } from '@wordpress/components';
import { useSelect, useDispatch } from '@wordpress/data';
import { createBlock as create } from '@wordpress/blocks';
@@ -163,7 +163,10 @@ const ConvertToLinksModal = ( { onClose, clientId, pages } ) => {
);
};

const BlockEditButton = ( { label, clientId } ) => {
export default forwardRef( function BlockEditButton(
Copy link
Contributor

Choose a reason for hiding this comment

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

In general I feel the BlockEditButton is now doing too much and has awareness of too many concepts.

Ideally these components need to stay as close to those in the canonical <ListView> as possible. This will make our lives much easier if we try to normalise them in the future.

To this end I think we should consider

  • moving <ConvertToLinksModal> modal to it's own file
  • extracting all pages logic to a custom hook
  • passing the requisite callbacks down into BlockEditButton as required.

We'll still be coupling concepts like Pages to the block editor package (not good) but at least it will be easier to extract and refactor in future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please take a look at the refactored version and let me know what you think. Moving usePageData and ConvertToLinksModal to their own files and lifting the onClick handler made the button small enough to make it irrelevant.

{ clientId, ...props },
ref
) {
const { selectBlock } = useDispatch( blockEditorStore );
const [ convertModalOpen, setConvertModalOpen ] = useState( false );
const { pages, totalPages } = usePageData();
@@ -195,9 +198,12 @@ const BlockEditButton = ( { label, clientId } ) => {
pages={ pages }
/>
) }
<Button icon={ edit } label={ label } onClick={ onClick } />
<Button
{ ...props }
ref={ ref }
icon={ edit }
onClick={ onClick }
/>
</>
);
};

export default BlockEditButton;
} );
You are viewing a condensed version of this merge commit. You can view the full changes here.