-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
[Experimental]: First version of pages list in site editor #54966
Conversation
This pull request has changed or added PHP files. Please confirm whether these changes need to be synced to WordPress Core, and therefore featured in the next release of WordPress. If so, it is recommended to create a new Trac ticket and submit a pull request to the WordPress Core Github repository soon after this pull request is merged. If you're unsure, you can always ask for help in the #core-editor channel in WordPress Slack. Thank you! ❤️ View changed files❔ lib/experimental/editor-settings.php ❔ lib/experiments-page.php |
const [ postStatuses, setPostStatuses ] = useState( EMPTY_ARRAY ); | ||
useEffect( () => { | ||
apiFetch( { | ||
path: '/wp/v2/statuses', |
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.
Don't we have an entity for that?
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.
No. We could add one in a follow up maybe?
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.
Definitely
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.
I'll work on this.
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.
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.
Since this is an experiment, and as discussed directly with Nik. I'm not going to deeply review the APIs here. I think the main goal of this PR should be to ship a starting point for us so we can iterate on this in parallel both in terms of API, design and behavior.
I think what we need is a breakdown of these tasks and logging them into a related overview issue which should help developers that want to engage with these efforts pick up tasks.
Size Change: +16.4 kB (+1%) Total Size: 1.64 MB
ℹ️ View Unchanged
|
packages/edit-site/src/components/sidebar-navigation-screen-pages-list/index.js
Outdated
Show resolved
Hide resolved
Flaky tests detected in 625810f. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/6389407723
|
@jameskoster this PR could be considered just a starting point and many things could be improved in follow ups where more people could work in parallel. Having said that I pushed a commit about:
This works but I couldn't make it to match the design to open a third nested submenu on the left. I've checked radix docs about it a bit, but couldn't fix it. --cc @ciampo .
What we should do is have a loader(maybe the progress bar) inside the table and update the new data when they arrive. I don't think we can avoid jumping because the returned rows based on filtering could be fewer/more than the current ones in view.
Let's add more fields in follow ups that we will also need to start forming the API we'll end up with. |
I don't think that's really possible, because I believe that nested menu always open to the right (in LTR environments), and only open to the left when there isn't space available on the right. So in this case, the third nested submenu has the space to open to the right and it does so. My hunch is that this behaviour can't be changed Also, a heads-up that the experimental |
Are you considering just changing the internal implementation or the whole structure/components etc..? |
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.
This is exciting! I haven't given the code a detailed look, but just added a tiny comment about ListView
naming and whether we can make it more unique to distinguish it from the list view sidebar.
One question in terms of design (and given this is an experiment, it can totally be looked at separately), but now that there's a view of table data that updates in a real-time way, it could be good for the pagination area to be sticky-ied to the bottom of the view, as currently it jumps up based on the number of results:
In any case, as Riad mentioned (#54966 (review)), I'm sure this sort of polishing can be picked up as separate tasks after the initial experiment has been merged 👍
Looking forward to seeing how all this evolves!
); | ||
} | ||
|
||
function ListView( { dataView, className, isLoading = false }, ref ) { |
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.
This could be a bit of a question for design across some of this work, but is it possible for us to come up with a more unique name for the list view of table data, so that we don't confuse it with the ListView
component in the sidebar of the post and site editors? Maybe prefixing with Table
would be enough — TableListView
?
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.
Yeah, we could rename, but this component will not be exposed and will only be used internally by the high level DataViews
component.
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.
this could be named "TableView" maybe to avoid confusion, we'll have things like "GridView", "KanbanView"... later
Probably a mix of both, mostly dictated by some differences between |
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.
So this is clearly unfinished and unpolished but I'm approving so that we can have a starting point that allows us to iterate on in parallel.
{ | ||
header: 'Status', | ||
id: 'status', | ||
cell: ( props ) => |
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.
Created a follow-up at #55196 It seems we should use accessorFn
instead of cell
to render this value.
✅ I updated the PHP Sync Tracking Issue to note this PR does not require a backport. |
What?
This is the first version of pages list in site editor with related issues: #53233, #50430.
The underlying APIs will evolve, but we need to get something in, to enable parallel work. This PR adds a new pages list in the site editor and to test it, you'll need to enable the
New admin views
GB experiment.For now it uses TanStack table, but this is an internal implementation detail and could also change. Part of the code has been extracted/polished a bit from #53906.
where the approach was to create helper components that could be composed into creating different views.
This PR changes the direction and aims to use a high level component(
DataViews
) that will provide the APIs for core and extenders for all views and functionality for any entity.Nothing is exported right now and no public APIs have been introduced.
Testing Instructions
Pages->Manage all pages
.Screen.Recording.2023-10-02.at.2.30.33.PM.mov