WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 35 hours ago

Last modified 10 hours ago

#42063 closed enhancement (fixed)

Media: Indicate if image is used as a site option

Reported by: melchoyce Owned by: helen
Milestone: 5.6 Priority: normal
Severity: normal Version:
Component: Media Keywords: has-patch has-screenshots commit
Focuses: ui, administration Cc:

Description

When viewing images in the Media Library's list view, it displays whether or not the image is attached to a site option, like favicon, logo, or header image. However, this isn't indicated in the grid view.

I was thinking we could add this information to the image details, so it's both available in the grid view, and also available in other media selection contexts.

See attached screenshots.

Attachments (6)

header-image.png (3.5 MB) - added by melchoyce 3 years ago.
audio-widget.png (207.2 KB) - added by melchoyce 3 years ago.
We could also do the same thing for images/video/audio used in media widgets
42063.1.patch (3.6 KB) - added by Mista-Flo 7 weeks ago.
Screenshot 2020-08-31 at 14.50.28.png (23.5 KB) - added by Mista-Flo 7 weeks ago.
Current output of List view
Screenshot 2020-08-31 at 14.50.21.png (102.7 KB) - added by Mista-Flo 7 weeks ago.
Output with the 1st patch applied
42063.patch (601 bytes) - added by mukesh27 2 days ago.
Patch that added missing doc

Change History (36)

@melchoyce
3 years ago

We could also do the same thing for images/video/audio used in media widgets

#1 @desrosj
3 years ago

  • Focuses administration added
  • Keywords needs-patch added

This ticket was mentioned in Slack in #core-media by antpb. View the logs.


21 months ago

#3 @mikeschroder
21 months ago

Hi Mel!

Saw this ticket while we were triaging media tickets without replies, so figured I'd reply with a note of support --
I like this as a feature/enhancement, and think it'd be cool for it to be more clear to users what their images are being used for.

This ticket was mentioned in Slack in #core-media by desrosj. View the logs.


21 months ago

#5 @Mista-Flo
7 weeks ago

  • Keywords has-patch has-screenshots added; needs-patch removed

All right, so I was working on this ticket. A bit more complicated than I thought, here's why:

  • List view display cropped images as well
  • If I upload a new image, and crop it, and use it as the logo for example, the media state will be displayed along the cropped image attachment in list view. But in grid view, the cropped images are not displayed, meaning I won't know which image is used as a logo. If I don't crop the original image, I will see the media state (with my patch applied).

So I will need some feedback about this inconsistency.

However I have uploaded a patch that:

  • Create a new get_media_states in wp-admin/includes/template.php, it just basically mimic the same function as get_post_states, meaning I could use it without printing the same HTML output.
  • It will display the media state as "Current state" in the image detail modal, feel free to give feedback about the wording here.

I'll upload two screenshots. One with the current behaviour in the list mode, another with the output of the patch.

@Mista-Flo
7 weeks ago

@Mista-Flo
7 weeks ago

Current output of List view

@Mista-Flo
7 weeks ago

Output with the 1st patch applied

This ticket was mentioned in Slack in #core-media by florian-tiar. View the logs.


7 weeks ago

#7 @justinahinon
7 weeks ago

Hi @Mista-Flo , thanks for your patch. It correctly adds the information about if the image is used somewhere when I open its modal.

#8 follow-up: @justinahinon
7 weeks ago

@desrosj do you know the reason behind (if there is one) not showing cropped images (like those cropped for site icons/logos) in the media library grid view?

#9 @justinahinon
7 weeks ago

  • Keywords needs-design-feedback added

I added the needs-design-feedback keyword so that we can have feedback about how this will be displayed in grid view.

#10 in reply to: ↑ 8 @pbiron
7 weeks ago

Replying to justinahinon:

@desrosj do you know the reason behind (if there is one) not showing cropped images (like those cropped for site icons/logos) in the media library grid view?

Some of the history behind that can be found in #50410 (and the other tickets/commits) referenced there. That ticket also discusses some of the other problems causes by not showing cropped images in grid view.

This ticket was mentioned in Slack in #core-media by antpb. View the logs.


6 weeks ago

#12 @Mista-Flo
5 weeks ago

Note that for reference, the cropped images are now displayed in the grid library, so the current patch will also be able to display the current state for these cropped images.

This ticket was mentioned in Slack in #core-media by florian-tiar. View the logs.


2 weeks ago

#14 @hellofromTonya
2 weeks ago

  • Milestone changed from Future Release to 5.6

Per Media scrub today, this ticket is win for 5.6. Moving it into the milestone. @karmatosed confirmed adding it on the design feedback list.

This ticket was mentioned in Slack in #design by hellofromtonya. View the logs.


7 days ago

This ticket was mentioned in Slack in #core-media by bluetriangle. View the logs.


6 days ago

#17 @hedgefield
6 days ago

  • Keywords needs-design-feedback removed

The work done so far looks good! And do I understand correctly that this info will also be visible in the list view of the media library? That's great. For the grid view, since there is no text there, marking an image with this info is a little harder. It might need some kind of icon overlay or something, at least something for which there is no precedent currently in WordPress, and thus would need design exploration. So I discussed it with Tammie and we agreed that for 5.6 it's fine to ship it without grid view 'support'. The info is available in the list and in the details, we can iterate on that after 5.6 with a good grid option too. Nice work!

#18 @hellofromTonya
6 days ago

  • Keywords commit added

Per Media meeting, with design feedback, this ticket is now ready for commit.

#20 follow-ups: @helen
5 days ago

I moved this over to https://github.com/WordPress/wordpress-develop/pull/611 for tests to run and to get used to the PR workflow :)

I have two thoughts here before committing:

  1. I would like the string Current state: to say Used as: instead. Both are a little misleading but the latter is somewhat less so, as you have both Header Image and Current Header Image (and the same for backgrounds). This is reflected in the GitHub pull request.
  2. I'm not sure (or rather @markjaquith is not sure) that we should be adding the $echo param to _media_states() in this patch because it is not going to be used anywhere in core, and it's an internal function so it doesn't need to appeal to a wider use case. This is not yet reflected in the pull request.

#21 @Mista-Flo
4 days ago

Hi @helen thanks for your review!

I definitely agree with the wording change, "Used as" better reflects the meaning of this row.

For your second point, I'm not sure, I tried to keep consistency between _post_states, but as you said, if it's not used anywhere else in Core, it might be useless to do so.

This ticket was mentioned in Slack in #core-media by garrett-eclipse. View the logs.


2 days ago

#23 in reply to: ↑ 20 @SergeyBiryukov
2 days ago

Replying to helen:

I'm not sure (or rather @markjaquith is not sure) that we should be adding the $echo param to _media_states() in this patch because it is not going to be used anywhere in core, and it's an internal function so it doesn't need to appeal to a wider use case. This is not yet reflected in the pull request.

Just noting that would be consistent with a similar change for _post_states() in [46309].

#24 in reply to: ↑ 20 @garrett-eclipse
2 days ago

  • Keywords needs-refresh added; commit removed

Replying to helen:

  1. I'm not sure (or rather @markjaquith is not sure) that we should be adding the $echo param to _media_states() in this patch because it is not going to be used anywhere in core, and it's an internal function so it doesn't need to appeal to a wider use case. This is not yet reflected in the pull request.

One thing to note is this is due to the implementation here is slightly off, instead of using get_post_states in the template we should be using _media_states( $post, false ) to get them as a string that's already separated properly. Also the span should use media-state and not post-state for a class. So with this change the echo would then be used in core and is there for back-compat. Although these are meant to be internal functions when we looked at _post_states and just changing behaviour to echo by default we found it was used by plugins so that change wouldn't work well. Instead the new param is so we can change the core use without issues. Hope that helps make sense of it there.

#25 @garrett-eclipse
2 days ago

  • Keywords commit added; needs-refresh removed

Revisiting after I attempted to make a refresh...
Attempting to use _media_states( $attachement, false ); I found the media template doesn't support HTML as it encodes it and viewing in the modal the label is bold so we don't so much care to do the same for the list of states. And looking at the class post-state is appropriate as the existing usage of _media_states is in the list table so it's just adopting that styling.
In short... @helen's previous PR there looks great and I would almost suggest keeping the echo as with the post_states method it was applied in multiple locations through follow-up tickets after the initial change so I would expect similar here.

#26 @helen
2 days ago

I'm not strongly opinionated on the $echo addition, just feels weird to add it before core usage actually happens. Will commit.

#27 @helen
2 days ago

  • Owner set to helen
  • Resolution set to fixed
  • Status changed from new to closed

In 49223:

Media: Indicate if item is or was used as a site option in the details modal.

Props Mista-Flo, melchoyce.
Fixes #42063.

#28 @mukesh27
2 days ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

We should update the doc for _media_states() function because 5.6 introduce return value as well

* @since 5.6.0 Added the `$echo` parameter.

Replace to

* @since 5.6.0 Added the `$echo` parameter and a return value.

@mukesh27
2 days ago

Patch that added missing doc

#29 @SergeyBiryukov
35 hours ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 49231:

Docs: Update the @since note for _media_states().

Follow-up to [49223].

Props mukesh27.
Fixes #42063.

Note: See TracTickets for help on using tickets.