Add: Default readonly views to dataviews. #55740
Conversation
|
Size Change: -207 B (0%) Total Size: 1.7 MB
ℹ️ View Unchanged
|
| // (entity request and useEffect to update the view). | ||
| export const DEFAULT_STATUSES = 'draft,future,pending,private,publish'; // All statuses but 'trash'. | ||
|
|
||
| const DEFAULT_PAGE_BASE = { |
There was a problem hiding this comment.
I don't think we need a different file for this config. It's specific to pages list, so it could live there and shouldn't be part of the dataviews component.
| @@ -40,8 +40,26 @@ const defaultConfigPerViewType = { | |||
| }, | |||
| }; | |||
|
|
|||
| const PATH_TO_DATAVIEW_TYPE = { | |||
| '/pages': 'page', | |||
There was a problem hiding this comment.
I think we don't need this here. We know we are in /pages path and we could just filter the default views based on the url param for the active one.
| @@ -111,13 +129,13 @@ export default function PagePages() { | |||
| id: 'featured-image', | |||
| header: __( 'Featured Image' ), | |||
| getValue: ( { item } ) => item.featured_media, | |||
| render: ( { item, view: currentView } ) => | |||
| render: ( { item, view: iterationView } ) => | |||
There was a problem hiding this comment.
You renamed this because of the new currentView declaration above, right? I think we should leave this as was and rename the other one activeView. The url param feels better as activeView, no?
| const { | ||
| params: { path }, | ||
| } = useLocation(); | ||
| let icon; |
There was a problem hiding this comment.
This seems like a good helper function here. We're going to need to get the icon per type in other places too. Example is the view options(top right) to change per type.
Nit: Do you think just a mapper object would be simpler?
const icons = { list: page, grid: columns};
const icon = icons[dataview.view.type]
|
|
||
| return ( | ||
| <ItemGroup> | ||
| { DEFAULT_VIEWS[ viewType ].map( ( dataview ) => { |
There was a problem hiding this comment.
Related to my other comment, I think we could pass the views here instead of keeping DEFAULT_VIEWS in a separate file for all the views we're going to have. I'd love some input from others for this.
There was a problem hiding this comment.
I agree with you that they are specific to "pages" and we don't need that file in the "data views" folder but these default views are useful in both the sidebar and the frame, so we should have it in one of these two folders. I guess.
There was a problem hiding this comment.
They could be in page-pages, exported and passed as param in <NavigatorScreen path="/pages">(sidebar/index.js). If we gather all default views in a different file than the handling of them feels a bit detached to me. No strong opinions here.
There was a problem hiding this comment.
A different thing here from the above discussion, is that if we try to go to single page from the list(ex. click the title), path is not defined and breaks the editor.
|
Thanks for the PR @jorgefilipecosta! This looks good in general and I'm happy that we're removing the provider 😄 . |
|
Nice. Would it be trivial to add a Trash view? If so that might be helpful – currently there's no way to view trashed pages. A count on the Drafts (and Trash if added) menu items could be useful too, similar to categories on the Patterns screen. |
jorgefilipecosta
force-pushed
the
add/default-readonly-views-to-dataviews
branch
from
8aba6b5
to
f29d484
Compare
|
Thank you for the review @ntsekouras, I think all the feedback was applied, let me know if there is anything pending or we could merge. |
| @@ -28,7 +28,7 @@ const DEFAULT_PAGE_BASE = { | |||
| }; | |||
|
|
|||
| const DEFAULT_VIEWS = { | |||
| page: [ | |||
| '/pages': [ | |||
There was a problem hiding this comment.
Personally I feel this key is useless because that whole file is within page-pages.
There was a problem hiding this comment.
The key was removed 👍
|
Hey, @jameskoster, I've added a trash view. However, I haven't implemented a count feature yet, as it wasn't considered during the initial development. |
| @@ -45,13 +45,13 @@ export default function DataViewsSidebarContent() { | |||
| const { | |||
| params: { path, activeView = 'all' }, | |||
| } = useLocation(); | |||
| if( ! path || ! DEFAULT_VIEWS[ path ]) { | |||
| if( ! path || path != '/pages') { | |||
There was a problem hiding this comment.
I think this check is useless no?
There was a problem hiding this comment.
No for some reason without the check the app crashed as soon as we clicked on a page.
There was a problem hiding this comment.
I think that's probably a subtle issue elsewhere.
jorgefilipecosta
force-pushed
the
add/default-readonly-views-to-dataviews
branch
from
0a664f0
to
9d036f7
Compare
|
Thank you, it's working well. Last request; could we use the We'll need to add an action to restore pages from the trash, and to delete permanently, but that's totally fine to do separately. No worries about the counts, it's a minor detail. |
|
What is the expected behavior around persisting view options for the readonly dataviews? For instance if I go to all pages and change the number of rows per page, I would expect that to persist on refresh, but that's not currently how it works. |
Right now, for these views, nothing is persisted, it's very similar to the existing WP-Admin pages (top level). I think we need to define what we want here precisely.
We didn't want to go down that rabbit whole at the moment until we have a clear idea and since for the next 6.5 release, we want to focus on the template lists and pattern lists, it's has less priority. |
Just noting that view ("Screen") options are persisted in wp-admin, while filters are not. But I agree, this is a detail we need to nail down in the design. |
@jameskoster noting that trash is an existing state in the filter that you can select: 
However, trash pages are not listed when the status is 
|
|
So it is! Still, a default readonly Trash view seems useful to me, don't you think? Edit: @oandregal I just checked and I do not see Trash as a status option in the filter on trunk: 
Note that I am also seeing order statuses from Woo which obviously do not work. Should I create an issue for that? |
Absolutely. Thought I'd share how the "All status" filter works, to validate the current behavior or change it if it needs be.
mmm, I'm intrigued about this. Would you be able to share your wp-admin page for pages? I wonder if you still see the "Trash" link there. Perhaps in a separate issue to not hijack this one.
Absolutely, we need to figure out how this would work in sites with custom statuses. Having a dedicated issue would help. Jorge already mentioned in other places, but the issue here seems to be that statuses are unbound: they affect all post types – see, for example, register_post_status. I can see how Woo may be registering some new statuses that show up there in your setup. The issue is that we don't know for which post type a status is registered, and we cannot simply remove the ones that are not the default from core: we'd also be removing post types that plugins intentionally added for pages, for example (editorial flow). The way forward may be what Jorge already suggested: allow providing statuses that are bound to a Custom Post Type. |
This PR applies the following changes:
Screenshots
Testing