-
Notifications
You must be signed in to change notification settings - Fork 10.7k
Admin list table [COT] #32864
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
Admin list table [COT] #32864
Conversation
|
I'm not sure if I'm testing correctly / if the behavior I'm seeing is expected. I have populated the orders table from the posts table using #32817 to start with. This is what I see: If the new orders table is authoritativeThere's no "Orders" menu in the WooCommerce menu, and I get the following notices: If I try and go to If the posts table is authoritativeThis time the "Orders" menue entry is there and it redirects to Screenshot of how I see that error: |
Yes, I didn't have the new admin navigation feature (ie, via WooCommerce ▸ Settings ▸ Advanced ▸ Features) enabled (actually, I was blissfully unaware of this) but I'm assuming from the error you shared that you do. I guess we'll need to support both navigation systems.
Yes, I'd expect it to be empty because the data store is not complete at this point. That's the reason the new view needs to be tested with the post table store. |
|
✍🏽 I've addressed the error in the last screenshot, but am still working on a few things relating to new nav experience and also an issue relating to when these settings are saved, versus when post types are registered/when menus are composed (will request a fresh review once done). |
If you're happy to, let's park this issue. There are a couple of ways to solve it but I wanted to get advice from Isotope, since they are the owners of the nav feature. Internal link: p1652220313065349-slack-CBKK5KM41 Edit: actually, unless I've just been looking at this for too long, which is possible, I'm not even sure it is specific to this PR. I seem able to replicate the same error with |
3537ff2 to
c9ebffd
Compare
|
@Konamiman — this is updated (rebased on latest trunk) and should be ready for another look. |
Konamiman
left a comment
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.
@barryhughes Sorry, you wrote in your comment "let's park this issue" and I interpreted it as referring to this pr itself 😓
Looks good and works as expected, the only differences I can notice with the posts based list is that the preview buttons don't work and the list isn't sortable, but I understand that's expected at this point. So, approving.
|
Hi @Konamiman, thanks for merging this pull request. Please take a look at these follow-up tasks you may need to perform:
|
|
Yep, sorry, I should have been clearer (+thanks for reviewing). |

Adds a new list table implementation that works with COT, and otherwise strives to emulate the existing post-based admin list table.
true ||to the start of the condition, to make the new admin list table render anyway.Closes #32385.
Testing
WC_Admin_Menus::orders_menu()method) as suggested and set the WP Posts store as default via WooCommerce > Settings > Advanced > Custom Data Stores.admin.php?page=wc-ordersin the URL).WC_Admin_Menus::orders_menu().Notes/questions
Admin\Internalnamespace (sidenote: its location is the reason an automated action applies thefocus: react adminlabel) or might we prefer to place it in some other namespace?