DataViews: add footer to Pages sidebar #57690
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
oandregal
requested review from
ntsekouras,
jorgefilipecosta,
jameskoster and
youknowriad
oandregal
added
[Type] Experimental
|
Size Change: +95 B (0%) Total Size: 1.69 MB
ℹ️ View Unchanged
|
ntsekouras
reviewed
Jan 10, 2024
| title={ __( 'Pages' ) } | ||
| content={ <DataViewsSidebarContent /> } | ||
| footer={ | ||
| <VStack spacing={ 0 }> |
There was a problem hiding this comment.
Maybe it's better to extract this and reuse it?
There was a problem hiding this comment.
Where would it need reusing? Note that the existing pages root is going to be removed when the new pages dataviews is made stable, so there's only one place where we need this.
There was a problem hiding this comment.
It could be a while though until we stabilize the pages view.. Anyway I have no strong opinions, it's just that it adds some bloat in this file, which we could move afterwards..
There was a problem hiding this comment.
Yeah, I see your point. I'm working to make it stable for 6.5, a few weeks from now, so I want to be optimistic. Considering that timeline, it felt unnecessary to make this PR bigger by extracting a new component that was to be removed soon. Could have done it differently, in a less optimistic day :)
|
Ack, sorry I didn't get to this before it was merged. Ideally these 'pages' should be included in the main pages list rather than being grouped separately in the sidebar. 
I appreciate that may be challenging as we're mixing record types and the fields/actions aren't compatible. If that is prohibitive then it's probably best to omit them for now. Placing them in the sidebar as in this PR creates some conceptual overlap between views and records which isn't altogether helpful. |
Oh, no worries, I can explore that. I think I misunderstood your comment. Actually, I was already implementing the second step (adding the "Front Page" to the sidebar, mimicking the existing), and I wanted to raise the same point. The existing page presents three concepts in the sidebar (templates, special pages, and user pages), which I find a bit difficult to reason about – more now with dataviews and the concept of views there. I presume they were there because of findability? This is: making it easier for users to find them, when they don't know they are templates (404) or special pages (front page). Is that right? Perhaps we can explore alternatives for the same effect: add a view in the sidebar for them, or something like that. |
This was referenced Jan 11, 2024
|
@jameskoster I've prepared two PRs for the
Considering why templates were listed there (findability?) and the current dataviews experience (filters, search, preview, etc.) is tailored for items of the same type, I lean towards the second approach. 1 is doable, though I wouldn't recommend pursuing it at this stage and don't find that it provides good findability. I'm fine if we want to go with 3, though I understand that it was there for a reason and would at least try to provide a similar affordance to users. |
|
@oandregal @ntsekouras how do y'all feel about reverting this one (and #57759)? It was initially added because we thought the List layout for data views would be included in 6.5. We were worried about not having the same features as the current pages list, which includes links to 404, search results, blog home, and front page templates. Since the List layout didn't make it into 6.5, having the footer on both the existing pages list and the pages data view seems a bit redundant. Additionally, there's some potential confusion between buttons that open views and buttons that open templates. That trade-off was worth it before, but now I'm not so sure. We could reconsider this for version 6.6. It might be better to find a way to incorporate these templates directly into the data views themselves. What do you think? |
|
Makes sense to me @jameskoster. I created a revert PR here: #59151 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Part of #55083
Follow-up to #57578
What?
Adds a footer to the sidebar of
Pagescontaining the404andSearchtemplates.Why?
To achieve feature parity with the existing
Pagesroot.How?
Creates a new
SidebarNavigationScreenPagesDataViewsthat adds the footer.Testing Instructions