Add: Media field changing ui to Dataviews and content preview field to posts and pages #67278
Conversation
jorgefilipecosta
requested review from
oandregal,
jameskoster,
jasmussen and
ntsekouras
|
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. |
jorgefilipecosta
added
[Feature] DataViews
packages/dataviews/src/types.ts
Outdated
| /** | ||
| * True if the field is supposed to be used as a media field. | ||
| */ | ||
| isMediaField?: boolean; |
There was a problem hiding this comment.
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.
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.
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.
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.
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.
There was a problem hiding this comment.
Personally I think it's the right thing for fields to say that they can be used as "media fields". That said, I'm not sure yet what that is. Maybe it's type: "media" maybe it's a flag like here but I don't see as part of the "view".
There was a problem hiding this comment.
Maybe it's even an "interface" for the field. For instance, we could say that "media" fields needs to support a "size" prop in their render function or something...
There was a problem hiding this comment.
I'd rather introduce the type: media field as a way to declare what fields can act as "preview". If we need additional info/props, we can also make that a requirement later.
There was a problem hiding this comment.
I just checked and the current featuredMedia is of type: text, which doesn't make any sense 😓 Tied to making the render more declarative, we may need more granularity: declaring a field is an image, or a video, or an iframe, etc. But perhaps that granularity doesn't need to come with the type but with something else that controls the render specifically.
|
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. |
|
Haven't looked at implementation deeply because this needs a rebase (a strong one 😅, given that mediaField is its own entity now). |
jorgefilipecosta
force-pushed
the
add/media-field-chaning-ui-and-content-preview-field-to-posts-and-pages
branch
from
3c191b6 to
9553303
Compare
|
Hi @jameskoster, @ntsekouras, @youknowriad, @oandregal this PR was redone to take into account the latest changes and all the feedback was applied. |
|
Right now media fields are identified as a flag as discussed with @youknowriad and @ntsekouras If there is any other preference ( e.g.: by field type ) I can do the change I don't have a preference here. |
|
Size Change: +1.21 kB (+0.07%) Total Size: 1.84 MB
ℹ️ View Unchanged
|
ntsekouras
force-pushed
the
add/media-field-chaning-ui-and-content-preview-field-to-posts-and-pages
branch
from
9553303 to
0555cab
Compare
|
Can you see the below too, or it's just me with something wrong when testing? 🤔 Screen.Recording.2024-12-17.at.10.53.41.AM.mov |
| @@ -18,6 +18,7 @@ const featuredImageField: Field< BasePost > = { | |||
| Edit: FeaturedImageEdit, | |||
| render: FeaturedImageView, | |||
| enableSorting: false, | |||
| isMediaField: true, | |||
There was a problem hiding this comment.
I know we have the mediaField nomenclature, but it feels like it's a previewField and maybe we should update name in view object too? 🤔
There was a problem hiding this comment.
Every new code I'm introducing is updated to refer preview. I guess in a side PR we can change the already existing API of mediaField / showMedia.
| @@ -175,6 +176,7 @@ export const registerPostTypeSchema = | |||
| postTypeConfig.supports?.comments && commentStatusField, | |||
| templateField, | |||
| passwordField, | |||
| contentPreviewField, | |||
There was a problem hiding this comment.
I haven't checked all the code yet, but if we want this field we should check for editor (content) support of the post type.
There was a problem hiding this comment.
I added a check for viewable posts with editor support.
| }: { | ||
| field: NormalizedField< any >; |
There was a problem hiding this comment.
Why remove this and add more individual props? If anything we should aim for the opposite if possible IMO.
There was a problem hiding this comment.
Hi @ntsekouras, because the "Preview" item we create with a label sublabel/description etc is not a field, this component receives strings that can come from real field properties.
| field: NormalizedField< any >; | ||
| fieldId: string; | ||
| label: string; | ||
| subLabel?: string; |
There was a problem hiding this comment.
Maybe description or something like that is more fitting? subLabel is just confusing and has a very specific different meaning in English.
There was a problem hiding this comment.
That's a good point it was updated.
| isVisible, | ||
| isFirst, | ||
| isLast, | ||
| canMove = true, | ||
| canHide = true, | ||
| isInteracting = false, |
There was a problem hiding this comment.
Because the action button that opens the menu popover is only visible if the mouse is over or if there is focus, on the item, when we open the menu the focus goes to the menu. In consequence the button that opens the menu would be hidden and the menu it self would be hidden.
To fix this issue we add a class that while the menu is open shows all the actions.
To test the wrong behavior and why the class is needed we temporarily remove the class.
I added a comment decoumenting this.
| onToggleVisibility, | ||
| onMoveUp, | ||
| onMoveDown, | ||
| additionalActions, |
There was a problem hiding this comment.
I'm wondering if it would be better to be able to pass children instead of naming it additionalActions.
There was a problem hiding this comment.
Good point I changed it to children.
| @@ -481,6 +627,7 @@ function FieldControl() { | |||
| { | |||
| field: mediaField, | |||
| isVisibleFlag: 'showMedia', | |||
| ui: mediaFieldUI, | |||
There was a problem hiding this comment.
Related to the other comment, I'm wondering how we can simplify this.
There was a problem hiding this comment.
We are introducing an option for a locked field to have a custom rendering. This option is used when there are multiple media fields, as it allows us to create a menu for selecting between them and to display a description. For all other cases, we will continue using the existing logic. I also want to simplify this logic, but I'm uncertain about how to do that. If we discover a way to simplify it in the future, we can easily implement changes since none of the logic in this file is exposed.
jorgefilipecosta
force-pushed
the
add/media-field-chaning-ui-and-content-preview-field-to-posts-and-pages
branch
from
0555cab to
082b878
Compare
|
Flaky tests detected in bee972e. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/12518145832
|
Hi @ntsekouras, the Menu component was deeply refactored yesterday with things like Menu.TriggerButton, Menu.Popover etc, that change made this UI not work after a rebase. I updated the code. |
|
Thank you for the review @ntsekouras, all the feedback was applied. |
I'd prefer that we introduce the {
id: 'featured_media',
type: 'text',
isPreviewField: true,
}Type |
| @@ -18,6 +18,10 @@ npm install @wordpress/fields --save | |||
|
|
|||
| Author field for BasePost. | |||
|
|
|||
| ### BasePost | |||
|
|
|||
| Undocumented declaration. | |||
There was a problem hiding this comment.
By adding a JSDoc comment to the exported entity this won't be undocumented.
There was a problem hiding this comment.
Hi @oandregal I added as JSDoc but the problem is persisting. BasePostWithEmbeddedAuthor was already undocumented so I don't think this is a blocker.
| isVisibleFlag: 'showMedia', | ||
| ui: previewFieldUI, |
There was a problem hiding this comment.
Is there a way to simplify this implementation? For example, instead of adding a specific UI for a field, what if we made the following changes:
FieldItem:
- add a new property "description" that renders the field in use (e.g.: Featured image).
- add a new property "options" that receives an array with the available (e.g.: [ "Content preview", "Featured image" ])
- add a new property "onChangeOption" that communicates a new option has been selected
In the FieldControl, we do the following for the media field:
<FieldItem
field={ field }
description={ view.mediaField.label }
options=[
{ label:"Content preview", id: content_preview },
{ label: "Featured image", id: feature_image }
],
onChangeOption={ newOption => onChangeView({ mediaField: newOption })
/>I feel the implementation would be simpler if we make the FieldItem (leaf node) a general component that the FieldControl configures depending on the specifics of the field (visible, not visible, can have multiple options, etc.). This way, it's easy to add the same behaviour to other fields (title).
There was a problem hiding this comment.
I applied this suggestion, It had some complexities but the base idea is there.
jorgefilipecosta
force-pushed
the
add/media-field-chaning-ui-and-content-preview-field-to-posts-and-pages
branch
from
4887285 to
1786885
Compare
|
Hi @oandregal I applied your feedback. |
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