-
Notifications
You must be signed in to change notification settings - Fork 4.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add: Media field changing ui to Dataviews and content preview field to posts and pages #67278
base: trunk
Are you sure you want to change the base?
Conversation
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. |
/** | ||
* True if the field is supposed to be used as a media field. | ||
*/ | ||
isMediaField?: boolean; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure this prop should be part of fields
because it's something used for some specific layouts.
Based on the linked issue's discussions, I got the impression that just rendering the preview would be enough and probably we should try that first.
Having said that, in the future if we want to add more fine control to views that utilize mediaField
, with viewConfigOptions
a view could render an extra control for a user to select what to preview in the mediaField
. It would need some changes on mediaField
form on that layout, but that is for another time in my opinion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @ntsekouras,
I'm not sure this prop should be part of fields because it's something used for some specific layouts.
It is still a property of the field, some fields can be media and be rendered as an image or video.
But I'm not sure if isMediaField is a good path either I thought about using a type for media files "type" = "media". Is there any other alternative you would prefer?
Based on the linked issue's discussions, I got the impression that just rendering the preview would be enough and probably we should try that first.
On #65331 @jameskoster suggested this design as a UI to switch between media fields. I think some users will prefer a content preview while for others featured-image, so offering the option to choose makes sense. But yah not having the option to choose would be simpler.
@jameskoster do you think not having the field preview switcher in a first version and defaulting to content would be ok?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is still a property of the field, some fields can be media and be rendered as an image or video.
I still don't think this should part of the fields API. For what you're describing above, I think the type
prop is a better fit.
We could still render the control with viewConfigOptions
prop.
Having different media fields could be part of the layout.mediaField
prop, which should be updated in order to support multiple, alternative renders. The latter is also related to the registration of fields, because if a layout provides only one field
for media field (let's say featured image
) and the post type doesn't support one, it could be good to have a fallback.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having different media fields could be part of the layout.mediaField prop
So instead of changing fields at all (not even adding a new type), you would prefer an alternative where layout.mediaField could be an array of fields instead of just one id, and the first item of the array is the media field that is active, correct? I think something like that could also work well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something like that yes, but haven't explored anything in code, so I'm not sure of any possible nuances. I'll also cc @oandregal and @youknowriad for thoughts.
Looks like a good start 🙏 I think the first question to ask here is whether this should apply to all layouts, or only grid. I'm inclined to suggest the latter, at least to begin with, especially as updates to the Pages List layout may remove the featured image (see #66570). The content preview doesn't work so well in Table layout due to the lack of space. I noticed the preview shows only the contents of the The field can be titled 'Preview' with options 'Content', 'Featured image'. |
Hi @jameskoster,
There are some pending questions on other comments but yes I agree we can start with the grid only.
It was not because of performance when requesting a page the content does not contain the template, so I showed just that. But we could implement something that requests the template and the content and shows a preview of both. There will be a small performance impact because of additional requests but that should be not noticeable. |
If performance is acceptable then I think showing the full page (with template) would be preferable. It's feasible that some pages will be heavily template driven, and difficult to recognise if only the content is visible. |
Supersedes: #65331
Fixes #63719.
cc: @jameskoster, @jasmussen
This PR implements the following changes:
For some users, most pages don't have a featured image so this PR fixes the issue of having a gray square as a page media as the UI shown most of the time. This users can now switch to show the content preview.
Testing Instructions
Screenshot