WordPress.org

Make WordPress Core

Opened 16 months ago

Closed 7 months ago

#20164 closed defect (bug) (fixed)

Attaching media box needs improvements

Reported by: markoheijnen Owned by: lessbloat
Priority: normal Milestone: 3.5
Component: Media Version:
Severity: normal Keywords: has-patch commit
Cc: markoheijnen, xoodrew@…, melissachoyce@…

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 16 months ago.
First version of the patch
22099.diff (8.4 KB) - added by lessbloat 9 months ago.
20164.2.diff (9.2 KB) - added by lessbloat 9 months ago.
20164.diff (9.4 KB) - added by helenyhou 7 months ago.
20164.spinner.png (3.4 KB) - added by SergeyBiryukov 7 months ago.
20164.3.diff (10.9 KB) - added by helenyhou 7 months ago.
20164.spinner.2.png (20.1 KB) - added by SergeyBiryukov 7 months ago.
20164.spinner.3.png (20.0 KB) - added by SergeyBiryukov 7 months ago.
20164.4.diff (11.1 KB) - added by helenyhou 7 months ago.
20164.5.diff (11.2 KB) - added by lessbloat 7 months ago.

Download all attachments as: .zip

Change History (36)

comment:1 DrewAPicture16 months ago

  • Cc xoodrew@… added

markoheijnen16 months ago

First version of the patch

comment:2 markoheijnen16 months 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 ocean9016 months ago

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

comment:4 markoheijnen11 months 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 markoheijnen11 months ago

  • Milestone changed from Awaiting Review to 3.5

The php code should be addressed in 3.5

comment:6 meliko11 months 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 meliko11 months 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 meliko11 months ago

  • Cc melissachoyce@… added

comment:9 lessbloat9 months ago

#22099 was marked as a duplicate.

lessbloat9 months ago

comment:10 lessbloat9 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: ocean909 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 lessbloat9 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.

lessbloat9 months ago

comment:13 lessbloat9 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 ocean909 months ago

#16029 was marked as a duplicate.

comment:15 ocean909 months ago

#21296 was marked as a duplicate.

comment:16 helenyhou7 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 7 months ago by helenyhou (previous) (diff)

helenyhou7 months ago

comment:17 follow-up: SergeyBiryukov7 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 SergeyBiryukov7 months ago

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

helenyhou7 months ago

comment:19 in reply to: ↑ 17 ; follow-up: helenyhou7 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: SergeyBiryukov7 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).

helenyhou7 months ago

comment:21 in reply to: ↑ 20 helenyhou7 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 SergeyBiryukov7 months ago

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

comment:23 nacin7 months ago

  • Keywords commit added

comment:24 nacin7 months ago

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

lessbloat7 months ago

comment:25 lessbloat7 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 nacin7 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.