Make WordPress Core

Opened 12 years ago

Closed 11 years ago

#20164 closed defect (bug) (fixed)

Attaching media box needs improvements

Reported by: markoheijnen's profile markoheijnen Owned by: lessbloat's profile 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 12 years ago.
First version of the patch
22099.diff (8.4 KB) - added by lessbloat 11 years ago.
20164.2.diff (9.2 KB) - added by lessbloat 11 years ago.
20164.diff (9.4 KB) - added by helenyhou 11 years ago.
20164.spinner.png (3.4 KB) - added by SergeyBiryukov 11 years ago.
20164.3.diff (10.9 KB) - added by helenyhou 11 years ago.
20164.spinner.2.png (20.1 KB) - added by SergeyBiryukov 11 years ago.
20164.spinner.3.png (20.0 KB) - added by SergeyBiryukov 11 years ago.
20164.4.diff (11.1 KB) - added by helenyhou 11 years ago.
20164.5.diff (11.2 KB) - added by lessbloat 11 years ago.

Download all attachments as: .zip

Change History (36)

#1 @DrewAPicture
12 years ago

  • Cc xoodrew@… added

@markoheijnen
12 years ago

First version of the patch

#2 @markoheijnen
12 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.

#3 @ocean90
12 years ago

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

#4 @markoheijnen
12 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

#5 @markoheijnen
12 years ago

  • Milestone changed from Awaiting Review to 3.5

The php code should be addressed in 3.5

#6 @meliko
12 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.

#7 @meliko
12 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

#8 @meliko
12 years ago

  • Cc melissachoyce@… added

#9 @lessbloat
11 years ago

#22099 was marked as a duplicate.

@lessbloat
11 years ago

#10 @lessbloat
11 years 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

#11 follow-up: @ocean90
11 years 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.

#12 in reply to: ↑ 11 @lessbloat
11 years 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.

@lessbloat
11 years ago

#13 @lessbloat
11 years ago

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

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

#14 @ocean90
11 years ago

#16029 was marked as a duplicate.

#15 @ocean90
11 years ago

#21296 was marked as a duplicate.

#16 @helenyhou
11 years 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 11 years ago by helenyhou (previous) (diff)

@helenyhou
11 years ago

#17 follow-up: @SergeyBiryukov
11 years 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";

#18 @SergeyBiryukov
11 years ago

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

@helenyhou
11 years ago

#19 in reply to: ↑ 17 ; follow-up: @helenyhou
11 years 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.

#20 in reply to: ↑ 19 ; follow-up: @SergeyBiryukov
11 years 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).

@helenyhou
11 years ago

#21 in reply to: ↑ 20 @helenyhou
11 years 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.

#22 @SergeyBiryukov
11 years ago

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

#23 @nacin
11 years ago

  • Keywords commit added

#24 @nacin
11 years ago

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

@lessbloat
11 years ago

#25 @lessbloat
11 years ago

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

#26 @nacin
11 years 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.