Skip to content
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

Data View - Page View: Add Featured Image Control #64496

Draft
wants to merge 29 commits into
base: trunk
Choose a base branch
from

Conversation

gigitux
Copy link
Contributor

@gigitux gigitux commented Aug 14, 2024

What?

Part of #64519

This PR adds the Featured Image Control for the Page view.

Screen.Capture.on.2024-09-18.at.18-09-37.mov

View mode

Grid Table Layout
image image image

Edit

Without Image With Image
image image

Additional Changes

Render component and view props

With this PR, the render component will now receive the view prop. This addition is necessary because the component’s layout needs to adapt based on the view. Similar cases include the Title field, where different layouts are required depending on the view.

[ LAYOUT_TABLE, LAYOUT_GRID ].includes( viewType ) &&
item.status !== 'trash';

Fix error on bulk edit mode

Screen.Capture.on.2024-09-18.at.18-01-00.mp4
Uncaught TypeError: Cannot read properties of null (reading 'status')

Currently, there is an error when the user edits a field in bulk mode. This PR fixes this error.

Attachment type

This PR updates the Attachment type.

Why?

How?

Testing Instructions

Ensure that Quick Edit in DataViews and Quick Edit in DataViews are enabled.

  1. Visit the Site Editor
  2. Click on pages.
  3. Toggle to the table view.
  4. Click on the settings.
  5. Make the feature image field visible.
  6. Click on details panel icon.
  7. Ensure that the Featured Image option is available.
  8. Click on it.
  9. Ensure that the Media Gallery opens and click on an image.
  10. Save.
  11. Ensure that the image is set as the featured image to the post.
  12. Toggle to the grid view.
  13. Ensure that the image is rendered correctly.
  14. Toggle to the list view.
  15. Ensure that the image is rendered correctly.

Testing Instructions for Keyboard

Screenshots or screencast

@youknowriad youknowriad added [Type] Experimental Experimental feature or API. [Feature] Data Views Work surrounding upgrading and evolving views in the site editor and beyond labels Aug 14, 2024
@jameskoster
Copy link
Contributor

To be clear, I think the Featured Image control should remain as it is currently in terms of design and position.

However I also recognise that panel Data Forms should be able to handle image fields, respecting the established conventions of this layout. Here's a mockup and flow that could achieve that:

image

What do you think?

@gigitux
Copy link
Contributor Author

gigitux commented Aug 16, 2024

To be clear, I think the Featured Image control should remain as it is currently in terms of design and position.

However I also recognise that panel Data Forms should be able to handle image fields, respecting the established conventions of this layout. Here's a mockup and flow that could achieve that:

image What do you think?

Thanks for your feedback! It makes sense! I'm going to implement your feedback! 🚀

@gigitux gigitux force-pushed the add/page-image-control branch 2 times, most recently from 2133ef7 to 0a2c977 Compare August 22, 2024 13:44
@gigitux
Copy link
Contributor Author

gigitux commented Aug 22, 2024

Hey @jameskoster, I've implemented your suggestion:

Screen.Capture.on.2024-08-22.at.15-42-51.mp4

Let me know if, from the design perspective, there is something to address.

I’m not marking this PR as ready for review because we still need to implement the default edit control for the image field. Since DataViews is a general-purpose library, we shouldn’t rely on the WordPress Media Gallery for the generic control. Instead, I suggest using the Operating System’s file selection option:

Screen.Capture.on.2024-08-22.at.15-52-20.mp4

However, please note that in this case, we won’t have access to the filename once the file is uploaded.

@jameskoster
Copy link
Contributor

Thanks for the update. The flow seems pretty good, but there are some design details to fix.

Screenshot 2024-08-22 at 16 06 34
  • Border radius is missing
  • Border should use $border-width (not the focus width)
  • Border color should be $gray-300
  • Hover background should be $gray-100
  • Thumbnail should be 24px
  • The thumbnail appearance should match unset color swatches
  • Gap between swatch and text should be 8px (seems you can remove the margin on the span)
  • Padding should be 8px 12px

Also here:

Screenshot 2024-08-22 at 16 09 13
  • Thumbnail should be 24px with $radius-small border-radius. Ideally there should be a semi-transparent black outline so that white images are still visible. You can see an example in data views if you toggle on the preview.
  • Image name container height should be 24px to align with thumbnail
  • Filename should be 12px, with 16px line-height
  • Gap between thumbnail and text should be 8px (again, removing the margin on the span should fix)
  • Padding should be 8px 12px
  • Reset button size should be small (24px) to align with the image name / thumbnail
  • Ideally the filename would be truncated so that you can see the filetype. Perhaps this can be accomplished by using the Text component with the truncate and ellipsizeMode props.

Here's a mockup that hopefully clarifies some of the details:

Screenshot 2024-08-22 at 16 20 23

@gigitux
Copy link
Contributor Author

gigitux commented Aug 23, 2024

With 9a39fe6, I addressed your feedback!

Screen.Capture.on.2024-08-23.at.10-14-17.mp4

The only remaining challenge is that the filename can only be truncated to a fixed number of characters, which may result in it not always ending directly under the remove button.

Comment on lines 485 to 490
event.target.tagName.toLowerCase();
// Prevent opening the media modal when clicking on the button/icon.
if (
element !== 'button' &&
element !== 'svg'
) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The issue is that there's a button within a clickable area, and clicking the button triggers a separate action. I'm not sure if this is the best way to handle it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah we should avoid nested buttons. We had a similar issue in data views list layout... this UI:

Screenshot 2024-08-23 at 11 41 02

Perhaps some of the techniques there can be borrowed?

render={ ( { open } ) => {
return (
<div
role="button"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jameskoster
Copy link
Contributor

Nice, it's getting there :)

Border-radius on the thumbnail and button are still missing.

Truncation of the file name seems to occur slightly before it's necessary. In the screenshot below note that there's more space the title could occypy.

Screenshot 2024-08-23 at 11 45 55

I know the button that appears in the panel is a work in progress, but I wonder if it could have the same appearance as the author field. What do you think?

Screenshot 2024-08-23 at 11 49 09

@gigitux
Copy link
Contributor Author

gigitux commented Aug 23, 2024

Truncation of the file name seems to occur slightly before it's necessary. In the screenshot below note that there's more space the title could occypy.

As I mentioned in the previous comment, this could be complex to achieve given that Text components can only truncate to a fixed number of characters, which may result in it not always ending directly under the remove button.

I know the button that appears in the panel is a work in progress, but I wonder if it could have the same appearance as the author field. What do you think?

Do you mean something like this?

image

@jameskoster
Copy link
Contributor

given that Text components can only truncate to a fixed number of characters

Are you sure? It seems to truncate based on width when truncate is true and numberOfLines is 1?

Do you mean something like this?

Sorry I should have been more explicit. See the mockup below and notice that the Author and Featured Image buttons (outlined in red) share the same characteristics; thumbnail size, gap, spacing, etc.

Screenshot 2024-08-23 at 12 23 12

@gigitux
Copy link
Contributor Author

gigitux commented Aug 23, 2024

Are you sure? It seems to truncate based on width when truncate is true and numberOfLines is 1?

The documentation says that when ellipsizeMode is middle (our case), the limit property is required:

image

´Limit` is the max number of characters to be displayed:
image

So, if we want to truncate in the middle (so, showing the extension), we need to explicit the max number of characters.

Sorry I should have been more explicit. See the mockup below and notice that the Author and Featured Image buttons (outlined in red) share the same characteristics; thumbnail size, gap, spacing, etc.

No worries! Gotcha! Thanks! 🙇

@gigitux
Copy link
Contributor Author

gigitux commented Aug 29, 2024

Thank you, @ntsekouras, for your valuable feedback! I've implemented your suggestion. However, the PR isn't ready for a full review yet, as I plan to explore the solution you proposed here. Currently, the UI/implementation of the generic image field control differs significantly from the control in the edit-site package.

One concern with your suggestion is that the existing mockup isn't adaptable for a generic file upload scenario:

image

The current mockup includes two labels:

  • The image name in the media gallery.
  • The filename.

For a generic file upload handler, only the filename would be available. As you suggested, I will explore of some kind of control config that is passed down, but as I mentioned in the Slack thread, I'm thinking if we we should work on a @wordpress/fields-control package

Sharing my thoughts here as well to help keep this discussion visible!

@gigitux
Copy link
Contributor Author

gigitux commented Sep 9, 2024

I'm going to mark this PR as ready to be reviewed. I'm going to update the description, but overall this PR introduces the Featured Image control following the @jameskoster feedback.

The remaining task is to truncate the filename based on the available width. Currently, truncation is performed using a fixed number of characters. Is it something that, is it needed?

In the future, this code may be moved to the @wordpress/core-fields-control package if we agree to create it.

@gigitux gigitux marked this pull request as ready for review September 9, 2024 13:54
Copy link

github-actions bot commented Sep 9, 2024

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 props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: gigitux <[email protected]>
Co-authored-by: jameskoster <[email protected]>
Co-authored-by: ntsekouras <[email protected]>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

@gigitux gigitux changed the title [WIP] Data View - Post View: Add Featured Image Control Data View - Post View: Add Featured Image Control Sep 9, 2024
@jameskoster
Copy link
Contributor

The remaining task is to truncate the filename based on the available width. Currently, truncation is performed using a fixed number of characters. Is it something that, is it needed?

If this is too problematic we could probably omit the filename for now.

Look at the screenshot in your last comment it seems the radius is missing on the thumbnail and the container. We can use $radius-small for this.

@gigitux gigitux changed the title Data View - Post View: Add Featured Image Control Data View - Page View: Add Featured Image Control Sep 10, 2024
@gigitux
Copy link
Contributor Author

gigitux commented Sep 10, 2024

Thanks for the feedback!

If this is too problematic we could probably omit the filename for now.

It makes sense! Addressed with 05f0d1a.

Look at the screenshot in your last comment it seems the radius is missing on the thumbnail and the container. We can use $radius-small for this.

Good catch! Fixed with c4db3b9.

Screen.Capture.on.2024-09-10.at.14-36-47.mp4

label: __( 'Featured Image' ),
getValue: ( { item } ) => item.featured_media,
type: 'image',
Copy link
Member

@oandregal oandregal Sep 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is this type for? This PR provides custom Edit and render functions and the type is not used anywhere I can see.

I see a few options:

  1. Implement the image type for DataForm (with a demo in storybook), like any other types) we support. The advantage is that we can make type a required prop to fields. Then, we'd override the Edit control here to use WordPress-specifics.
  2. Implement a custom feature-image type that is specific to WordPress, like the slug PR tries to do.
  3. Do not provide any type and just override Edit & render. We'd still need to 1 at some point. It's fine if it's a follow-up.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR has been opened before @wordpress/fields package and the multiple great conversations that we had during these week. I think that it makes sense to follow the second option. I'm going to move this PR as draft, given that it will require some work before another round of review!

Thanks for the help! 🙇

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Data Views Work surrounding upgrading and evolving views in the site editor and beyond [Type] Experimental Experimental feature or API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants