Make WordPress Core

Opened 2 years ago

Closed 2 years ago

Last modified 16 months ago

#57574 closed enhancement (fixed)

Add a Download row action to the Media List Table

Reported by: pbiron's profile pbiron Owned by: audrasjb's profile audrasjb
Milestone: 6.2 Priority: normal
Severity: normal Version:
Component: Media Keywords: has-screenshots needs-user-docs
Focuses: ui, administration Cc:

Description

As a follow-up to #41474, I think it would be useful to add a Download row action to the Media List Table.

Patch and screenshot coming shortly.

Attachments (2)

57574.diff (1.4 KB) - added by pbiron 2 years ago.
download-row-action.png (25.2 KB) - added by pbiron 2 years ago.

Download all attachments as: .zip

Change History (43)

@pbiron
2 years ago

#1 @pbiron
2 years ago

  • Keywords has-patch has-screenshots added

#2 @joedolson
2 years ago

Note: shortens "Copy URL to clipboard" to reduce clutter. This seems like a reasonable change; I'm not sure that "to clipboard" is really very necessary. What else would 'Copy URL' do?

#3 @pbiron
2 years ago

yes. the only reason the Copy row action said that was that all the other places where there is a Copy URL button (e.g., in the Media Modal) said that.

I think it's fine without `to clipboard' :-)

#4 follow-up: @kebbet
2 years ago

Could the same string as in [55156] be used? So the row action link says: Download file.

Then the row actions will be:
Edit | Delete permanantly | View | Copy URL | Download file

This ticket was mentioned in PR #3934 on WordPress/wordpress-develop by @kebbet.


2 years ago
#5

Use Download file as text for row action link.

https://core.trac.wordpress.org/ticket/57574

@kebbet commented on PR #3934:


2 years ago
#6

Before:
https://i0.wp.com/user-images.githubusercontent.com/11491369/215257928-42cfbc12-ef88-4976-accc-ef6833766a92.png

With patch from @pbiron :
https://i0.wp.com/user-images.githubusercontent.com/11491369/215257901-eb77feb4-4286-4d98-ace1-24de83dc5634.png

With this patch:
https://i0.wp.com/user-images.githubusercontent.com/11491369/215257817-bd5c6f1d-c18f-4baf-9a87-bd7bfa031865.png

#7 @Mista-Flo
2 years ago

Hello there,

that's a nice addition to the media library IMO, I have tested the patch and it works fine, I'm able to download the file, and the copy URL update doesn't affect the feature as well, so LGTM, great job :)

#8 @audrasjb
2 years ago

  • Keywords commit added
  • Milestone changed from Awaiting Review to 6.2
  • Owner set to audrasjb
  • Status changed from new to accepted

This looks perfect to me.
I think we can definitely ship it in 6.2.

#9 @audrasjb
2 years ago

  • Keywords needs-user-docs added

#10 in reply to: ↑ 4 @pbiron
2 years ago

Replying to kebbet:

Could the same string as in [55156] be used? So the row action link says: Download file.

Then the row actions will be:
Edit | Delete permanantly | View | Copy URL | Download file

Great catch @kebbet! I didn't realize I used a different string.

@kebbet commented on PR #3934:


2 years ago
#11

Latest commit updates updates the help tab.

https://i0.wp.com/user-images.githubusercontent.com/11491369/215282334-b84d5779-6984-4da2-8cd1-fa58aaba8165.png

#12 @kebbet
2 years ago

  • Keywords needs-copy-review added

The updated help tab texts might need a copy review. Something about the file is being downloaded to the user’s device or computer.

#13 @audrasjb
2 years ago

  • Keywords commit removed

Ah good point, indeed we need to update the help tabs.
Removing commit pending copy review.

#14 @costdev
2 years ago

  • Keywords needs-refresh added

Great idea! I've looked at the PR that updates the strings and they look good to me - but will leave the copy review to those from the Help/About component.

There are some test failures that are related to an issue with an earlier commit to trunk which has now been fixed. The PR needs a refresh on trunk.

#15 @kebbet
2 years ago

  • Keywords needs-refresh removed

I've updated the PR against trunk (tests are passing), and adjusted the copy a bit. Any feedback is welcome!

#16 @amin7
2 years ago

Test Report


This report validates that the indicated patch added the enhancement.
Patch tested:
https://core.trac.wordpress.org/attachment/ticket/57574/57574.diff

Environment


OS: Windows 10 version 22H2
Web Server: Nginx
PHP: 7.4.30
WordPress: 6.2-alpha-54642-src
Browser: Chrome 109.0.5414.87
Theme: Twenty Twenty-Three
Active Plugins: No plugins activated.

Before Patch


Screenshot: https://d.pr/i/dvaXYR

After Patch


Text Changes: 'Copy URL to Clipboard' to 'Copy URL'
A download option is added and the file can download.

Screencast: https://d.pr/v/WmihOz


✅ patch is working as per enhancement request.

Last edited 2 years ago by amin7 (previous) (diff)

#17 @kebbet
2 years ago

Thanks for testing @amin7, could you please test the latest patch found at GitHub linked in this ticket? It also updated the help tab.

#18 @amin7
2 years ago

Hi @kebbet

I checked and the help tab is also updated, here is a screenshot. https://d.pr/i/2JrdeO

Just sharing a thought of mine if I don't bother you, should the text 'Copy URL to clipboard' also be changed to 'Copy URL' in the edit mode? Here is a screencast: https://d.pr/v/ivPJjN

Thanks & regards

#19 @kebbet
2 years ago

First:
Thanks for testing @amin7!

This ticket focus on the action row links, if the button on the edit page that says Copy URL to clipboard should be changed, that's a discussion for another ticket I guess. But the question started me thinking, should the row action for Copy URL to clipboard be left untouched in this ticket, is there really that much visual clutter? Isn't the text there for a reason?
@audrasjb or @pbiron, do you have any opnions on this?

Secondly:
Any feedback on the help tab text is welcome, what should we go for to get this into 6.2?
https://github.com/WordPress/wordpress-develop/pull/3934#pullrequestreview-1274240130

#20 @audrasjb
2 years ago

Hey hey,

This patch can replace "Copy URL to clipboard" with "Copy URL" in the row action links as it's better in the context of this ticket, but in my mind it's better to open a follow-up ticket to address other occurrences, once this once is committed.

Concerning copy review, I'd go with @costdev's proposal:

<strong>Download file</strong> downloads the original media file to your device.

#21 @kebbet
2 years ago

  • Keywords needs-copy-review removed

Thanks for swift reply! PR is updated with @costdev's proposal.

@kebbet commented on PR #3934:


2 years ago
#22

Latest text change in the help tab:
https://i0.wp.com/user-images.githubusercontent.com/11491369/216284520-56f6b4c8-fd2b-4348-b575-3153b9afac2a.png

#23 @pbiron
2 years ago

Everything looks great to me.

@audrasjb Can this get committed before Beta 1 (next tues)?

#24 @audrasjb
2 years ago

  • Keywords commit added

yes, definitely!

#25 @audrasjb
2 years ago

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

In 55198:

Media: Add a Download row action to the Media List Table.

This changeset makes it easier for users to download their uploaded media by providing a Download row action to the Media List Table. It also rephrases the Copy URL row action for better consistency and to give room for the new Download action.

Follow-up to [55156].

Props pbiron, joedolson, kebbet, Mista-Flo, costdev, amin7, mukesh27.
Fixes #57574.

#27 follow-up: @joedolson
2 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

I think wp_get_attachment_url() should probably get escaped, like it is for the copy action.

#28 in reply to: ↑ 27 @pbiron
2 years ago

Replying to joedolson:

I think wp_get_attachment_url() should probably get escaped, like it is for the copy action.

I'm not sure. The copy action seems to be the only one that escapes the URL in the generated link.

@audrasjb what do you think?

#29 @joedolson
2 years ago

Yeah, as I've looked through, it seems pretty random; should probably find the commit that added the copy link and if there was any discussion there. One way or the other, we should be consistent.

#30 @pbiron
2 years ago

The ticket that added the copy action is #54426. The escaping was first done in https://core.trac.wordpress.org/attachment/ticket/54426/54426.3.patch.

#31 @audrasjb
2 years ago

  • Keywords has-patch commit removed

Indeed, it is recommended to escape use esc_url here, as @costdev pointed out in another ticket, see the function description:

eliminates invalid characters and removes dangerous characters

So, even though esc_url() is mainly used for security, this also helps when extenders return a value that has invalid characters. I guess this is one reason why the pattern exists elsewhere in Core.

#32 @audrasjb
2 years ago

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

In 55221:

Media: Properly escape Download row action link in Media List Table.

Props joedolson, pbiron, audrasjb.
Fixes #57574.

#33 @milana_cap
2 years ago

  • Keywords add-to-field-guide added

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


2 years ago

#35 @milana_cap
2 years ago

  • Keywords add-to-field-guide removed

This ticket was mentioned in Slack in #core-test by pbiron. View the logs.


2 years ago

#37 @bobbingwide
2 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

While testing 6.2-beta5 I noticed that the Download link is not present if you filter on Unattached items.

It does appear for unattached items in the other modes.

#38 @pbiron
2 years ago

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

Thanx for finding that @bobbingwide.

The 6.2 release leads have decided that it's too late in the cycle, and the fix will have to wait until 6.2.1.

I've opened #57890 so this doesn't get lost.

This ticket was mentioned in Slack in #docs by 611shabnam. View the logs.


19 months ago

This ticket was mentioned in Slack in #docs by zzap. View the logs.


17 months ago

This ticket was mentioned in Slack in #docs by zzap. View the logs.


16 months ago

Note: See TracTickets for help on using tickets.