DataViews: Update view fields rendering config #63148
Closed
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
youknowriad
added
[Type] Code Quality
|
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. |
|
Size Change: +501 B (+0.03%) Total Size: 1.76 MB
ℹ️ View Unchanged
|
oandregal
reviewed
Jul 5, 2024
oandregal
reviewed
Jul 5, 2024
packages/edit-site/src/components/sidebar-dataviews/default-views.js
Outdated
Show resolved
Hide resolved
oandregal
reviewed
Jul 5, 2024
| @@ -106,7 +110,9 @@ function ViewTypeMenu( { | |||
| return onChangeView( { | |||
| ...view, | |||
| type: e.target.value, | |||
| layout: {}, | |||
| fields: | |||
| defaultFields?.[ e.target.value ] ?? | |||
There was a problem hiding this comment.
This ties view.fields to view.type. However, when considering bundled or custom views, I see that it could be useful to have a few different table views that use different fields (same view.type but different view.fields), for example.
This is why I see this as a consumer decision, not dataview's. I'd remove the defaultFields from DataViews API and keep the onChange callback we have in consumers to augment the information based on the type change.
There was a problem hiding this comment.
I can see both ways but the current approach allows a clear cut (resets fields on each change) when you switch layouts which is a better default and if someone wants to keep or reuse fields, it's possible.
A small advantage too is that we don't have to repeat the same code in all consumers
I'm open to change but I have a small preference as is. Would you be ok with trying this for some time ?
There was a problem hiding this comment.
Sure, not a blocker for me. I see how it can be a nice devexp touch.
The way I understand this prop is as a mechanism to augment or provide the "reset state" for the fields prop — any other view prop has a "natural" reset state (empty). Perhaps the main concern I have is that the name (defaultFields) doesn't really convey that concept.
Throwing some names in case some stick as self-explanatory:
onViewTypeReset: this is the clearest I could come up with. It should also allow for providing a function besides the fields object. There's the question of whether other view props should be reset-configurable as well. I don't see a strong need for this, though perhapsview.perPageor futureview.densitycould benefit from this as well. Ok to start withfieldsonly.- Other options:
viewTypeResetFieldConfig,viewTypeResetFields,fieldsToResetOnViewTypeChange,fieldsPerViewType.
There was a problem hiding this comment.
I agree that there's potential for adding similar reset behaviors to other properties. At this point, the prop becomes defaultViews or something like that.
There was a problem hiding this comment.
Taking into account the conversations in the other threads, I think we should start this exploration with a better name than defaultFields: fieldsRenderConfig would work for me.
There was a problem hiding this comment.
but it's actually the default "fields" property of view. view.fields
oandregal
reviewed
Jul 5, 2024
oandregal
reviewed
Jul 5, 2024
oandregal
reviewed
Jul 5, 2024
@jameskoster I would love an opinion on this. |
|
Primarily, the thing to avoid is this: 
The template description is unnecessarily tall and difficult to read due to it being squished by the label. Hiding the label is one way to go about it. But we should consider that in some cases multiple fields could end up with similar values. With hidden labels it can be tricky to interpret one from the other. E.g. alt text, caption, and description in media items: 
In this case there should still be a way for users to determine the field label. To be honest I think there could be room for both options 🙊
|
|
@jameskoster Fair, do you see the "columns" and "rows" as something that can mix and match or do you see us rendering first columns and then rows (or the opposite like the screenshot). |
|
Mixing and matching can make it more difficult to visually scan. This is not ideal: 
So for now I'd lean towards grouping. However, I don't know that we can categorically say that columns should always appear before rows or vice versa. Same width badges to be honest. Could the order be configurable? If not I'd lean towards: Badges, Rows, Columns. |
|
Ok, so in the latest commit, I restored the support for "column" fields. The problem though is that the rendering config is "lost" when you hide a field, so if you toggle "description" off and on, it becomes a regular "row" field. I think this can be solved by separating "visibleFields" from the "fields" definition, something we wanted to avoid with @oandregal but we can explore it as a follow-up WDYT? |
|
That's a bit clunky, but fine to address in a follow up, assuming you're confident a solution can be found. |
As I see it, this means the |
Yes, but one of the goals of "defaultFields" was to also replace the visible fields per layout and not just define their formats. 🤔. |
|
@jameskoster I applied @oandregal's suggestion to fix the issue. I'm still not entirely satisfied with what we have but I think we can run with this. I'd appreciate some smoke testing. |
|
I'll also try to explore an alternative proposal that represent a smaller departure from what we have in a separate PR (I'm second guessing myself :P ) |
|
Going to close this in favor of #63236 A refactoring like this one can be done later once we gain more clarity. |
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.
Related to #55083
Follow-up to #63127
What?
This PR does two things:
layoutproperty from theviewobject. In favor of adefaultFieldsprop that can be passed optionally to the DataViews component. The defaultFields will tell which fields are visible by default on each layout (when switching layouts)view.fieldsconfig instead of being separate. So the view fields are defined like[{ render: 'media', field: 'preview' }, "author", "description" ]. The idea is that we'll be using this later to combine fields as well.It's a significant change to the API, so it's a good idea to test most dataviews:
Note One known regression here is that I removed the "column/row" distinction in the grid layout. Personally, I think it was wrong to have this in the first place. I'd argue that we could potentially hide the "field labels" in this layout like we do for "list".
Note2 I believe badge fields are only supported in grid views right now, I think this can be easily extended to all layouts if we want.
Testing Instructions
1- Test all layouts.
2- Test toggling field visibility for all layouts.
3- Test custom views.