DataViews: improve UX of bundled views for Pages #65295
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
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
As I mentioned in the other PR, I won't have a lot of availability the next days. In the interest of making this part of 6.7, I'd appreciate if anyone pushes the necessary tweaks directly — should we want any. |
oandregal
requested review from
ntsekouras,
jorgefilipecosta,
youknowriad,
jameskoster and
jasmussen
oandregal
added
[Type] Enhancement
This was referenced Sep 12, 2024
|
Size Change: +120 B (+0.01%) Total Size: 1.77 MB
ℹ️ View Unchanged
|
youknowriad
reviewed
Sep 12, 2024
| ], | ||
| }, | ||
| view: DEFAULT_POST_BASE, | ||
| filterDataBy: [ |
There was a problem hiding this comment.
Does this really need to be in a "filters" format? I mean this can be just status: publish and inject it directly into the REST API query. Nothing forces us to use the dataviews format here.
The only reason the dataviews format would be good here is if we consider default views just like custom views, views that are saved in the backend and that the user can edit... But since we don't want the user to be able to edit these, it might not be worth the added complexity of using this format.
Anyway, If you feel otherwise, that's ok. It's not a big deal for me.
There was a problem hiding this comment.
We have to do two things here:
- Applying this to the REST API => your suggestion makes a lot of sense and simplifies things.
- Applying this to fields, so they don't have filters that are already declare here => If we go with the REST API nomenclature here, we'd have to create another mapping when fields are created. For example, it can be
authororauthor_excludein the REST API but the field id is alwaysauthor. I just wanted to avoid creating yet another mapping by leveraging a nomenclature that users are already familiar with.
No strong opinions, can do whatever folks prefer.
There was a problem hiding this comment.
Applying this to fields, so they don't have filters that are already declare here
Can you clarify this point. Is it about hiding the field from the available filters?
There was a problem hiding this comment.
Ok, so If I'm not wrong, we want to hide the "status" from available filters in all the views except "all pages".
First, that we need a dedicated flag on the field object to do that. It shouldn't be the absence of "elements" that triggers that.
Now, it's true that we have two ways of looking at this "default views".
-
if they are equivalent to custom views, your approach in the first PR makes sense assuming that we want to hide specific filters values. But I would argue that if these filters can be any random filters, it's bad to hide the filters because we don't really know what they represent. Maybe that filter say "title that start with a" (random filter). In that case, why I shouldn't be able to add another filter as a user that says "title contain b". So in this case hiding the filters doesn't really make sense.
-
The custom views are not "real views", they are just random WP-Admin pages. In this case, there's no need to have a "filters" key, we can just say
status: "publish"and addenableFilter: falseto the "status" field in all pages but the "all pages".
Does that reasoning make sense?
Anyway, I shared my reasoning but I don't think it's that important for now. I'm happy to approve this PR as no new APIs or anything. That said I'd prefer if we keep "filters" as key if we go with the current approach instead of filterDataBy.
There was a problem hiding this comment.
Does that reasoning make sense?
That's in line with what I shared in the other PR. Essentially, we need to figure out how something like this can work in custom views across permission levels (or same user). Custom views are just a view config right now, so if we don't want this to be part of the view config like in 65264, we'd need to make custom views a different thing that can declare changes to view, fields, and datasets (depending on implementation).
That said I'd prefer if we keep "filters" as key if we go with the current approach instead of filterDataBy.
Done at cb9b210
youknowriad
approved these changes
Sep 12, 2024
jameskoster
approved these changes
Sep 13, 2024
oandregal
force-pushed
the
update/bundled-views
branch
from
cb9b210 to
d6a242c
Compare
oandregal
force-pushed
the
update/bundled-views
branch
from
d6a242c to
0e8259d
Compare
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.
Alternative to #62157 and #65264
Partially addresses #60468
What
Makes bundled views declare how they filter the data without displaying that filter to users.
Why
See #60468
How
Bundled views no longer provide filters for the view — those would be visible to users. Instead, they provide filters for the dataset via the
filterDataByproperty — the name is intentionally different fromfiltersso there's no confusion.How to test
Go to the Pages view and select any of the bundled views (left sidebar). In any of them (but "All") you should see that:
Gravacao.do.ecra.2024-09-12.as.18.58.47.mov
Props
Add props to @Souptik2001 for the original PR: