Off Canvas Navigation Editor: Add Convert To Links Modal #45984
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
It was easier to nuke the commit history than to manually sync a bunch of commits, so this is all the commits in the branch squashed
|
|
|
Size Change: -1.47 kB (0%) Total Size: 1.32 MB
ℹ️ View Unchanged
|
It was easier to nuke the commit history than to manually sync a bunch of commits, so this is all the commits in the branch squashed
scruffian
force-pushed
the
add/navigation-off-canvas-edit
branch
from
47d7381
to
5bd0cfc
Compare
|
Thanks for working on this. When I convert to links, none of the links have pencil icons next to them, so I can't edit them, unlike in trunk. |
added 2 commits
November 23, 2022 14:01
…ordPress/gutenberg into add/navigation-off-canvas-edit
cbravobernal
added
[Type] Feature
getdave
reviewed
Nov 25, 2022
packages/edit-site/src/components/sidebar-edit-mode/navigation-item-editor/index.js
Outdated
Show resolved
Hide resolved
packages/block-editor/src/components/off-canvas-editor/block-edit-button.js
Show resolved
Hide resolved
packages/block-editor/src/components/off-canvas-editor/block-edit-button.js
Outdated
Show resolved
Hide resolved
added 2 commits
November 29, 2022 15:26
4 tasks
Conflicts: packages/block-editor/src/components/off-canvas-editor/block-edit-button.js
ajlende
approved these changes
Dec 1, 2022
There was a problem hiding this comment.
I merged in trunk because rebasing was difficult and it's easier to undo a merge commit if something didn't go as expected. That being said, this is testing well for me.
I think since this PR was opened, there were changes to showing only a submenu for the page list block, and that case isn't handled by this change.
I think we can add a follow-up to fix that and get this one merged.
getdave
requested changes
Dec 1, 2022
| const { getSettings } = select( blockEditorStore ); | ||
|
|
||
| return { | ||
| pages: getSettings().__experimentalFetchPageEntities( { |
There was a problem hiding this comment.
We'll need to test whether __experimentalFetchPageEntities is defined before attempting to utilise it.
There was a problem hiding this comment.
For example in the Post Editor this occurs
getdave
requested changes
Dec 1, 2022
There was a problem hiding this comment.
Thank you for continuing to iterate on this 🙏
My main concerns are that
- it doesn't work in the Post Editor
- we're tightly coupling components making future refactoring much harder
Let's try and address those before we merge this. Sorry to be a blocker here but I think it's important.
Much appreciated 🙇
| storedSettings, | ||
| blockPatterns, | ||
| blockPatternCategories, | ||
| fetchPagesEntities, |
There was a problem hiding this comment.
Do you think there's a chance this might cause a perf problem because fetchPagesEntities is always a new function reference?
I'm hoping the fact that useSelect has [] as deps means it will only run once and thus fetchPagesEntities will remain a consistent reference but if that's not true then it will cause problems as the useMemo will be invalidated for the entire editor settings.
| const { getSettings } = select( blockEditorStore ); | ||
|
|
||
| return { | ||
| pages: getSettings().__experimentalFetchPageEntities( { |
There was a problem hiding this comment.
For example in the Post Editor this occurs
| </Modal> | ||
| ); | ||
| }; | ||
|
|
||
| export default forwardRef( function BlockEditButton( |
There was a problem hiding this comment.
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
BlockEditButtonas 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.
There was a problem hiding this comment.
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.
| // Performance of Navigation Links is not good past this value. | ||
| const MAX_PAGE_COUNT = 100; | ||
|
|
||
| const usePageData = () => { |
There was a problem hiding this comment.
Let's move this to its own file.
| }; | ||
|
|
||
| // copied from convert-to-links-modal.js | ||
| const convertSelectedBlockToNavigationLinks = |
There was a problem hiding this comment.
Let's move this to it's own file. All the more reason to do so in order to keep in sync with convert-to-links-modal.js
| }, [ pages ] ); | ||
| }; | ||
|
|
||
| // copied from convert-to-links-modal.js |
There was a problem hiding this comment.
Please can we have the full file path? 🙏
| replaceBlock( clientId, navigationLinks ); | ||
| }; | ||
|
|
||
| const ConvertToLinksModal = ( { onClose, clientId, pages } ) => { |
There was a problem hiding this comment.
If this is copied from somewhere can we have a comment to specific exactly where? 🙏
Comment on lines
185
to
189
| if ( allowConvertToLinks ) { | ||
| setConvertModalOpen( ! convertModalOpen ); | ||
| } else { | ||
| selectBlock( clientId ); | ||
| } |
There was a problem hiding this comment.
I don't think this component should know anything about this concept. Lets lift this up and pass down as a prop. Perhaps if the onClick prop is defined then call that, else default to selectBlock?
Comment on lines
194
to
200
| { convertModalOpen && ( | ||
| <ConvertToLinksModal | ||
| onClose={ () => setConvertModalOpen( false ) } | ||
| clientId={ clientId } | ||
| pages={ pages } | ||
| /> | ||
| ) } |
There was a problem hiding this comment.
This could be lifted up. I don't think it needs to "live" with the edit button.
|
@georgeh I think we should consider a simpler approach. I've outlined the reasons why I think that one might be preferable in the PR description. Let us know what you think? 🙇 |
added 2 commits
December 7, 2022 15:07
|
I think that now #46352 is merged we can close this. Feel free to reopen if you think we still need it. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Screen.Recording.2022-11-22.at.7.26.33.PM.mov
Why?
When editing navigation, editing a page list will prompt converting to links. This is revisiting the work from #45575 and addresses #45442
How?
Testing Instructions
Screenshots or screencast