-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Page parent selector with ComboboxControl. #25267
Conversation
Size Change: +181 B (0%) Total Size: 1.19 MB
ℹ️ View Unchanged
|
@youknowriad - created follow up for loading indicator - #25798 I noticed when we merged the author changes, we now use the Shall I open a follow up issue to change this for the authors select, and we can discuss the ideal number to use there? |
I'd be totally in favor of this. No need for a (relatively) complex search UI when a site has only, say, 5 users. Less likely to happen for pages but yes, there may be sites with no more than 10 pages (or 20, or whatever the agreed limit is). |
This would require fetching the number of available pages before rendering which I'm not sure how to do now. It doesn't seem like we have a dedicated data action for that. I'd leave it separate for now. I made some updates here, I'd love some testing before shipping. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code wise and when tested this feels and looks good 👍 - I have left a couple small comments though. I haven't approved, only for wanting to know about my second comment postTypeSlug
related.
In aa701ad I discovered an issue unrelated with this PR but made visible where sometimes getEntityRecords don't return the required values. More details here https://wordpress.slack.com/archives/C02RQC26G/p1602586210264400 I'm choosing to deoptimize the |
@rianrietveld retying to test the latest version against WordPress trunk, I do not see the pages loading/searching when I type, or the initial page added to the list when my parent is outside the original selection. I am testing with several hundred pages, several levels deep, ie:
Is it working for you? |
@adamsilverstein It worked great for me. The command above did create scheduled pages though (not published) so I had to force the date. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍 - CI has a failing test (timing problem) that was in other PRs as well. It's not related and can be handled in a separate PR.
I'll give it another test. |
We only see our Homepage as an option in the Parent page combo field now and no search results are returned as of Gutenberg 9.2.0. We have about 1,500 pages on our site. We see the full list of our pages using Gutenberg 9.1.1 (although it takes about 10 seconds for the Parent Page dropdown field to appear when creating a new page. Using Edge (Chromium) and WordPress 5.5.1. |
@youknowriad I can confirm this does not work for me either in wp core trunk - I see the same results as my previous test:
Note that the user selector still works as expected. I'll try to dig in to see what is happening. |
Thanks for testing and for your help on this @adamsilverstein |
@youknowriad - followed up with fixes in #26397 |
Description
Use the combox selector to provide a searchable page selector.
Fixes #9441.
Supersedes #16666.
Based on work in #23237.
How has this been tested?
Screenshots
Note: screenshot does not include loading indicator which I added a bit later. the logic for loading state needs more work. I found that if I passed false to downshift in initialSelectedItem then later supplied the correct value, the correct value was not shown. I will work on this issue soon.
Types of changes
Checklist: