[Experiment POC] DataTable component(pages + templates list) #53906
Conversation
ntsekouras
requested review from
youknowriad,
SaxonF,
nerrad,
ramonjd and
kevin940726
|
Size Change: +16.7 kB (+1%) Total Size: 1.64 MB
ℹ️ View Unchanged
|
|
A starter list of extensibility needs for consideration with any list table abstract (including what you've already mentioned):
Feature set considerations (although I imagine this would evolve over time):
|
ntsekouras
force-pushed
the
try/datatable
branch
from
a8bbe69
to
5b0471b
Compare
|
Thanks for putting up a draft PR for the discussion! @SaxonF has also done something similar in #54006, which I saw you already have a discussion in #53233 as well. I'm not sure where to put my thoughts. I hope this is the right place but we can also move some of the discussions elsewhere or even open a new discussion topic. The API design here is actually very similar to my first immediate approach when I integrated the library. It accepts some required props like One issue arose when I wanted to access the internal state of We can, however, make everything controlled and use our own state, which seems to be the approach in this PR. It works for more complex cases where we need more controls, but also makes it significantly more difficult to implement a client-only simple table. The solution I took is to just hoist the One drawback of this approach is that we're locked into the This is not to say that there should be only one abstraction though. If we ever find ourselves building the same table over and over again, for instance, the tables of templates and template parts are mostly the same, we can always extract it into another component too. The abstraction doesn't have to be all top-level either. We can build helpers or smaller hooks and components to implement different features like pagination and filtering. WDYT? Does any of these make sense 😅? Any concerns or details I'm missing? Happy to discuss more! 🙇 |
|
Thank you for sharing your insights Kai!
Some of them I think it makes sense to be in the respective PRs, but probably it's better to try to keep decision related discussions in the main issue. Your comment is about the technical approach, so it would be nice to share a link/cross comment there, or share afterwards a summary of some discussions we're making, in order to be more concise. 🤷
API is just a first iteration and we would need to explore more to see if/how we can simplify. That goes for whatever solution we go for.
That's what this PR is mostly about. The Table component(can be renamed) is actually a context provider, so any other components like filters(even by third party devs) can have access to the instance.
No, that's not something good and it's not what this PR does. It does that for
I think that's the main reason for these explorations. I'd say though that not every table is the same in it's foundation and is different in representation, styles, etc.. By exploring different lists(pages, media, templates, etc..) we can see what is missing and what can be reused, and what will work best eventually.
This could be simply achieved by passing through all the TanStack options(which we do now), although we don't need to support everything, if we don't want to or can't. By suggesting/guiding using the external library we also add similar maintenance costs IMO. By having a wrapper of that, we don't need to worry about versions of TanStack consumers are using etc.. I think we aim for a table component that can be also used by plugins, to have a coherent user experience. The drawback of my approach though is that I suggest exposing new APIs(components) early on. These will be locked for start though or just moved to
I think this is the same for both approaches. I don't see us replacing TanStack any time soon, after introducing it in our codebase. I mean, I love we're discussing this and would appreciate more feedback! |
ntsekouras
force-pushed
the
try/datatable
branch
from
5b0471b
to
2f1a5f9
Compare
This approach has a hidden gotcha: The <DataTableContext.Provider value={ { table } }>
const useDataTableContext = () => useContext( DataTableContext ).table;
True, then we just have to decide whether we want to make it a hook or a component (or both?). I guess the reason we want to use a component here is to also pass the context to the plugin's code underneath? If that's the case then I'm starting to see the point!
I was thinking more along the lines of reexporting I can now understand your approach more! I was confused by the |
ntsekouras
changed the title
|
I have very little context about this whole project (been AFK for 2 months), but I'm going to leave some considerations: Where should the table component liveEspecially at a point where the component is very experimental, I'd avoid adding it to the I would also recommend having this component locked until we reach API stability. When/if we are happy with the component and we think it has reach stability and is enough of general purpose component, then we should move it to the About re-exporting
|
ntsekouras
force-pushed
the
try/datatable
branch
from
2f1a5f9
to
5e04c0f
Compare
|
Thanks for the feedback all!
Exactly! This should be part of the evaluation of the suggested implementation. Personally I don't believe it will be a problem, but we'll have to see how impactful it can be.
Yes. Although as you said we could even expose both if we need/want to.
I think discussions are already a bit fruitful, by:
I think the next steps could be:
So any help for all these would be really helpful. Feel free to explore and push directly here. In the end, we can extract code and create smaller PRs to land what we need for now. |
ntsekouras
force-pushed
the
try/datatable
branch
from
087430e
to
2941187
Compare
|
I implemented the logic for the |
|
Thanks Nik. There are still some ongoing design explorations around pagination. Perhaps for now it makes sense to copy the format on the Pattern management view? This one: 
|
ntsekouras
force-pushed
the
try/datatable
branch
2 times, most recently
from
88bc334
to
61d3483
Compare
|
Flaky tests detected in c5ecb01a66835f6d9b65c9de029057eb3071c7c9. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/6272197022
|
ntsekouras
force-pushed
the
try/datatable
branch
2 times, most recently
from
7e9060b
to
dc91a20
Compare
ntsekouras
force-pushed
the
try/datatable
branch
2 times, most recently
from
e435f44
to
c5ecb01
Compare
ntsekouras
force-pushed
the
try/datatable
branch
from
c5ecb01
to
13f2dc9
Compare
What?
This is just an explorative PR about the new WP admin lists I had started some time ago, before the related PR that explores the new Media Library and I'm creating it now, as I was AFK. In general there is overlapping work and we should coordinate how we move forwards. That's also the reason I didn't focus much to add more filter controls.
Related issues: #53233, #50430
What I had in mind mostly is a DataTable component that could be used in core interfaces(pages/templates lists, etc..), but also be able to used by extenders for their lists, that would result in a more coherent UX. After discussions in the PR and the issue, the new components will live in
edit sitepackage and will be extracted tocomponentspackage when are more stable.I mostly experimented with the pages lists as it requires a controlled data table, that means we feed the table with the final data, manually handling filtering, sorting, pagination etc... In general I've implemented this by using a DataTableProvider, so we can compose components an have access to the table's instance.
An example of
uncontrolleddata table would be something like this:Extensibility
Whatever solution we end up with should have extensibility in mind. That means having a way to add columns and custom filters. For columns we could just have a filter and you can test that in dev tools with something like this:
Noting though, that if you do it in the dev tools, just trigger manually a rerender of the table by searching something for example.
What about custom filters? Probably we would need something similar to update the query for fetching the pages and is not implemented right now.
Notes
Testing Instructions
Manage all templatesandManage all pagesand play around a bit.Any feedback would be appreciated 😄