WordPress.org

Make WordPress Core

Opened 2 years ago

Closed 20 months ago

#20164 closed defect (bug) (fixed)

Attaching media box needs improvements

Reported by: markoheijnen Owned by: lessbloat
Milestone: 3.5 Priority: normal
Severity: normal Version:
Component: Media Keywords: has-patch commit
Focuses: Cc:

Description

  • When you want to attach a media file to a post you can also select page when you are a author. You will get a warning you are not allowed to edit a post. There should be a capability check if a user can attach a media to that post type
  • I do wonder if getting post type which are public is the right way. ( I can blame myself: #13229 ). I can find ways where public is false but the post type still can be viewed from the backend.
  • To make it usable for plugin a filter on the post type can be helpful. So a plugin can use it for it's own purpose.

UX feedback needed for the following points:

  • The popup box little bit fades away in the background. Maybe a drop shadow helps with it?
  • Also the radiobuttons alignment of the post types hits the searchbox.

See function find_posts_div() for the code.

related: #19834, #16029

Attachments (10)

20164.1.patch (3.6 KB) - added by markoheijnen 2 years ago.
First version of the patch
22099.diff (8.4 KB) - added by lessbloat 21 months ago.
20164.2.diff (9.2 KB) - added by lessbloat 21 months ago.
20164.diff (9.4 KB) - added by helenyhou 20 months ago.
20164.spinner.png (3.4 KB) - added by SergeyBiryukov 20 months ago.
20164.3.diff (10.9 KB) - added by helenyhou 20 months ago.
20164.spinner.2.png (20.1 KB) - added by SergeyBiryukov 20 months ago.
20164.spinner.3.png (20.0 KB) - added by SergeyBiryukov 20 months ago.
20164.4.diff (11.1 KB) - added by helenyhou 20 months ago.
20164.5.diff (11.2 KB) - added by lessbloat 20 months ago.

Download all attachments as: .zip

Change History (36)

comment:1 DrewAPicture2 years ago

  • Cc xoodrew@… added

markoheijnen2 years ago

First version of the patch

comment:2 markoheijnen2 years ago

  • Keywords has-patch added; needs-patch removed

Added a first patch that solved all the 5 points.

The default post types doesn't have to be public anymore. I change it to show_ui. It seems more appropriate in this case.
For a more advance choice it would be better to have post type support for it so you can select if a post type really wants this behavior.

comment:3 ocean902 years ago

#16029 was an idea for a Google Code-in project.

comment:4 markoheijnen2 years ago

Related ticket: option for all post types: #21296.

With ticket #16029 I would love to see it as that instead of selecting the posttypes as now. With an option to filter which post types.
If it is one then don't show which post type it is

comment:5 markoheijnen2 years ago

  • Milestone changed from Awaiting Review to 3.5

The php code should be addressed in 3.5

comment:6 meliko2 years ago

I've taken a quick stab at mocking up the attach popup using style of the link popup, as per the suggestion in #16029: http://i.imgur.com/DBLsF.png

It currently only shows the two default post types. There was some discussion in the UI chat about making the radio buttons into a select dropdown if a user has more than two or three post types, which I think could be a good solution to the awkwardness that happens when you have too many inline radio buttons.

comment:7 meliko2 years ago

markoheijnen noted in chat that differentiating between post types isn't necessary using the internal linking model, so I've removed those options: http://i.imgur.com/Vjuev.png

I've also done a quick mockup of a search result: http://i.imgur.com/7QcbB.png

comment:8 meliko2 years ago

  • Cc melissachoyce@… added

comment:9 lessbloat21 months ago

#22099 was marked as a duplicate.

lessbloat21 months ago

comment:10 lessbloat21 months ago

I apologize. Didn't see this ticket before I completed a similar patch (on a separate ticket). Here are the changes I made:

  • Made it so that you could click anywhere in the row to select a radio button
  • Added dark overlay background
  • Margin and padding tweaks in various places
  • Brought radio options on same line as search box
  • Added AJAX spinner on search
  • On init it now loads default set of options

Here's what it looks like:

http://f.cl.ly/items/3M0n1q452b3J10292V2f/find-posts-after.jpg

comment:11 follow-up: ocean9021 months ago

I also have worked on patch on #16029. I don't like the radio buttons for each post types, because it's limited. I prefere something like the internal linking dialog, see #16029.

comment:12 in reply to: ↑ 11 lessbloat21 months ago

Replying to ocean90:

I don't like the radio buttons for each post type

We could remove them easy enough, and just return everything.

lessbloat21 months ago

comment:13 lessbloat21 months ago

Removed radios and added "type" column in 20164.2.diff:

http://f.cl.ly/items/0f3z1U0M2G1k0X3V3B3u/find-post-no-radios.jpg

comment:14 ocean9021 months ago

#16029 was marked as a duplicate.

comment:15 ocean9021 months ago

#21296 was marked as a duplicate.

comment:16 helenyhou20 months ago

  • Keywords ux-feedback removed

Seems like we need to exclude non-public post types (e.g. nav menu item) and attachment, as the radio selectors did previously. 20164.diff for that, but I think my transformation of the post types array into something for IN() is kind of lame, although it doesn't seem all that different from how it's done in WP_Query.

Last edited 20 months ago by helenyhou (previous) (diff)

helenyhou20 months ago

comment:17 follow-up: SergeyBiryukov20 months ago

ucfirst( $post->post_type ) is not translatable and doesn't work with UTF-8 strings.

singular_name label should be used instead:

$post_type_object = get_post_type_object( $post->post_type );
$html .= '<td><label for="found-'.$post->ID.'">'.esc_html( $post->post_title ).'</label></td><td>'.esc_html( $post_type_object->labels->singular_name ).'</td><td>'.esc_html( $time ).'</td><td>'.esc_html( $stat ).'</td></tr>'."\n\n";

comment:18 SergeyBiryukov20 months ago

Tested 20164.diff in Firefox 16. The spinner seems a bit off: 20164.spinner.png.

helenyhou20 months ago

comment:19 in reply to: ↑ 17 ; follow-up: helenyhou20 months ago

Replying to SergeyBiryukov:

ucfirst( $post->post_type ) is not translatable and doesn't work with UTF-8 strings.

Did that plus RTL and blue in 20164.3.diff

Replying to SergeyBiryukov:

The spinner seems a bit off

I can't reproduce in Firefox. Is it something with the translation maybe? Happy to fix, just not sure how.

comment:20 in reply to: ↑ 19 ; follow-up: SergeyBiryukov20 months ago

Replying to helenyhou:

I can't reproduce in Firefox. Is it something with the translation maybe?

Hmm, reproduced on English install as well, also in Chrome 23 and Opera 12 (Windows). Especially noticeable in Opera: 20164.spinner.2.png.

#find-posts-input { width: 141px; } appears to fix this: 20164.spinner.3.png.

We should also probably add nowrap to the Type/Date/Status fields (see the same screenshots).

helenyhou20 months ago

comment:21 in reply to: ↑ 20 helenyhou20 months ago

Replying to SergeyBiryukov:

We should also probably add nowrap to the Type/Date/Status fields (see the same screenshots).

Added that via the .no-break class and added a width for that input in 20164.4.diff. I'm not sure about the nowrap though - it can potentially push the table wider than the dialog. Not sure how likely that is to happen, but doesn't seem good.

comment:22 SergeyBiryukov20 months ago

Tested 20164.4.diff in Firefox 16, Chrome 23, IE 8, Opera 12. Looks good to me.

comment:23 nacin20 months ago

  • Keywords commit added

comment:24 nacin20 months ago

  • Owner set to lessbloat
  • Status changed from new to assigned

lessbloat20 months ago

comment:25 lessbloat20 months ago

20164.4.diff tested great. One minor CSS change in 20164.5.diff​ (adding a border-top to .find-box-buttons).

comment:26 nacin20 months ago

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

In 22723:

Fix up the 'Attach' dialog on upload.php.

We are de-emphasising attaching (see [22630]) but this is existing
core functionality and will remain for now. This commit just cleans
it up a bit so as to be less embarrassing.

props lessbloat, helenyhou.
fixes #20164.

Note: See TracTickets for help on using tickets.