Refactor convert links #46930
Merged
Refactor convert links #46930
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
ajlende
added
[Type] Bug
|
Size Change: -38 B (0%) Total Size: 1.33 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 0d8ea2e7bda0852ea34ce02210f3e7269f0df7c3. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/3875440012
|
|
👋 This is WIP and also a draft but it's in RfR. Is it actually ready? Just checking 😄 |
|
@getdave I was doing more refactoring on Friday that didn't get pushed up. But this morning, I'm thinking that it might be a better idea to do this first so there isn't so much in the PR all at once, especially since this is changing some behavior. Looks like I have some tests to fix, but then it will be ready. |
ajlende
requested review from
MaggieCabrera,
scruffian,
getdave and
draganescu
ajlende
force-pushed
the
fix/unify-convert-to-links-behavior
branch
from
0d8ea2e to
9ea0f3c
Compare
scruffian
reviewed
Jan 11, 2023
| } | ||
| } | ||
|
|
||
| function ConvertToLinksModal( { onClick, disabled } ) { |
There was a problem hiding this comment.
What do you think about putting this in a different file?
There was a problem hiding this comment.
It was in a different file convert-links-modal.js. Not sure what the problem was with that.
Edit: maybe since the getBlockContent function was turned into a component, having them all inlined is easier to read.
There was a problem hiding this comment.
I think before there were two ways to do it, so that's why we're removing one.
draganescu
reviewed
Jan 11, 2023
| 'This menu is automatically kept in sync with pages on your site. You can manage the menu yourself by clicking customize below.' | ||
| ); | ||
|
|
||
| function BlockContent( { |
scruffian
reviewed
Jan 12, 2023
| 'postType', | ||
| 'page', | ||
| { | ||
| per_page: -1, |
There was a problem hiding this comment.
For some reason this was set to MAX_PAGE_COUNT. I think that's wrong so I changed it to -1, to remove the limit.
There was a problem hiding this comment.
Why is it wrong? I think it was intentionally added as a safeguard.
There was a problem hiding this comment.
Before this change it was set to -1. The limit is there to prevent the conversion happening if there are more than 100 items, not to prevent the display of more than 100 items.
There was a problem hiding this comment.
I've updated it to MAX_PAGE_COUNT anyway as that is probably sensible.
draganescu
previously requested changes
Jan 25, 2023
scruffian
force-pushed
the
fix/unify-convert-to-links-behavior
branch
from
e07587f to
86abead
Compare
talldan
reviewed
Jan 31, 2023
Comment on lines
+83
to
+98
| if ( blockList.length === 0 ) { | ||
| const parentPageDetails = pages.find( | ||
| ( page ) => page.id === parentPageID | ||
| ); | ||
| return ( | ||
| <div { ...blockProps }> | ||
| <Warning> | ||
| { sprintf( | ||
| // translators: %s: Page title. | ||
| __( '"%s" page has no children.' ), | ||
| parentPageDetails.title.rendered | ||
| ) } | ||
| </Warning> | ||
| </div> | ||
| ); | ||
| } |
There was a problem hiding this comment.
Seems like there's a bug that can happen here. If I don't have a parent page defined, parentPageDetails ends up undefined, and an error can be thrown trying to access parentPageDetails.title.rendered.
There was a problem hiding this comment.
I can't reproduce it now, not sure what was happening. 🤔
There was a problem hiding this comment.
I found a different way to reproduce the issue, though it's a real edge case. I put together a fix - #47603.
Looking a the diff, I realise the issue wasn't introduce here, so sorry for bombarding the PR with comments!
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?
Refactors the convert links functionality between the toolbar and panel and makes them consistent with one another.
Why?
In preparation for #46469, I wanted to do some refactoring so the convert to blocks functionality only needed to be changed in one place.
I chose to use the 100 page limit from the toolbar 'Edit' button and the select block functionality from the side panel
Customizebutton.The limit, according to #30390, is because performance begins to significantly degrade after about that many pages. I think in a follow-up the UX should change to show a disabled button in both scenarios with an explanation for why it is disabled rather than just hiding it.
The select parent block was added in #46352 to improve on the UX, and I think that change should be brought over to the toolbar 'Edit' button and modal for consistency.
How?
Testing Instructions
wp post generate --count=100 --post_type=page --post_title='test')Testing Instructions for Keyboard
Screenshots or screencast