Make WordPress Core

Opened 12 years ago

Closed 12 years ago

Last modified 12 years ago

#21092 closed enhancement (fixed)

Media uploader insert button doesn't respect the current post type

Reported by: evansolomon's profile evansolomon Owned by: nacin's profile nacin
Milestone: 3.5 Priority: normal
Severity: normal Version:
Component: Media Keywords: needs-patch needs-codex
Focuses: Cc:

Description

The media uploader's insert button always says "Insert Into Post", even if you're editing another post type. It's especially confusing if you're editing a custom post type that doesn't look anything like a post, like an image slider or product.

The button should ideally say "Insert into [singular post type]" -- for example, "Insert Into Page".

Attachments (6)

21092.diff (2.5 KB) - added by evansolomon 12 years ago.
21092.2.diff (3.6 KB) - added by evansolomon 12 years ago.
Introduces an optional post type label
21092.3.diff (3.6 KB) - added by evansolomon 12 years ago.
In which I'm bested by the demands of localization
21092.4.diff (3.6 KB) - added by evansolomon 12 years ago.
21092.patch (957 bytes) - added by ocean90 12 years ago.
21092.typo.patch (476 bytes) - added by ocean90 12 years ago.

Download all attachments as: .zip

Change History (31)

@evansolomon
12 years ago

#1 follow-up: @evansolomon
12 years ago

20192.diff ignores the "Insert Into Post" strings in _insert_into_post_button() because the function isn't used anywhere.

#2 follow-up: @johnbillion
12 years ago

The approach in your patch works fine in English, but it's not suitable for l10n. We can't simply pass the post type name as a parameter to sprintf() because in some languages the phrase "Insert into [post_type]" will differ depending on the post type.

We'll need to introduce a new post type label, probably called insert_into_post, and use that. See #19257 for an example of a patch which introduces a new post type label.

#3 in reply to: ↑ 2 @evansolomon
12 years ago

Replying to johnbillion:

We'll need to introduce a new post type label, probably called insert_into_post, and use that. See #19257 for an example of a patch which introduces a new post type label.

This seems a little different because we never want to use a default string; the default should incorporate the post type name, or at least that's the point of the ticket. One idea is to check for a specific label but never set a default, then fall back to "Insert into $post_type". I realize that reintroduces the l10n problem, but I don't think there's any other way to improve the defaults.

@evansolomon
12 years ago

Introduces an optional post type label

#4 follow-up: @johnbillion
12 years ago

WordPress defaults to the 'post' label for every other post type label. For l10n reasons, the behaviour of this label shouldn't be any different. Here's an example why:

Language X uses grammatical gender or some other funky language construct, and the entire phrase "Insert into post" can differ depending on the noun. If we default to the 'post' label for insert_into_post then the phrase will always be grammatically correct.

If instead we try to build the phrase using the singular label as the noun, there's a chance the resulting string will be grammatically incorrect and possibly make no sense at all. It's better to default to the correctly localised version of "Insert into post" rather than an incorrectly localised version of "Insert into [custom post type]".

Your patch should include the insert_into_post label for WordPress' built in Post and Page post types (in get_post_type_labels()). It's then up to individual post types to include their own insert_into_post label.

I realise this doesn't address what you are trying to achieve with this ticket (which is, for this button to always display the correct post type name for custom post types) but unfortunately that's how it is.

@evansolomon
12 years ago

In which I'm bested by the demands of localization

#5 in reply to: ↑ 4 @evansolomon
12 years ago

Replying to johnbillion:

Your patch should include the insert_into_post label for WordPress' built in Post and Page post types (in get_post_type_labels()). It's then up to individual post types to include their own insert_into_post label.

I realise this doesn't address what you are trying to achieve with this ticket (which is, for this button to always display the correct post type name for custom post types) but unfortunately that's how it is.

Fair enough, this makes sense. I took another swing at it in 21092.3.diff.

#6 follow-up: @dd32
12 years ago

Purely code review:

isset( $_GET ) && isset( $_GET['post_id'] )

There's no need to check to see if GET is set, You can simply do if ( isset( $_GET['post_id'] ) ) or !empty() if that fits the use-case better

esc_attr__( $parent_type_obj->labels->insert_into_post )

esc_attr__ is a alias for esc_attr( __(...) ) so you should remove the __ from the function call

@evansolomon
12 years ago

#7 in reply to: ↑ 6 @evansolomon
12 years ago

Replying to dd32:

Nice catches, behold 21092.4.diff.

While we're at it, I'm not sure if the else cases for the GET and POST params not existing ever happen. It seemed prudent to add it, but I haven't been able to find a case where it actually happens. Feedback on that would be welcome if anyone has it.

Last edited 12 years ago by evansolomon (previous) (diff)

#8 follow-up: @helenyhou
12 years ago

What about it saying something more like "Insert into content" or something along those lines? It's already sort of confusing when next to the "Use as featured image" link (another issue entirely), so I wonder if it would be more effective to more clearly indicate that it is, at least by default, going into an editor instance.

#9 in reply to: ↑ 8 @azaozz
12 years ago

  • Keywords ux-feedback added

Replying to helenyhou:

What about it saying something more like "Insert into content" or something along those lines? ... I wonder if it would be more effective to more clearly indicate that it is, at least by default, going into an editor instance.

Was thinking the same. Perhaps one of these:

  • "Insert into content"
  • "Send to editor"
  • "Add"
  • "Insert"

The button that opens that popup has "Insert" too.

Last edited 12 years ago by azaozz (previous) (diff)

#10 @SergeyBiryukov
12 years ago

If we go with 21092.4.diff, insert_into_post should fall back to an existing value (singular_name seems appropriate) if it isn't set (see ticket:15576:7).

I like the idea of "Insert into content" / "Send to editor" though, sounds much simpler.

Seems like we already had "Send to editor" at some point earlier, that string still exists in TinyMCE help:
http://core.trac.wordpress.org/browser/tags/3.4/wp-includes/js/tinymce/wp-mce-help.php#L223

#11 follow-up: @helenyhou
12 years ago

"Send to editor" might be a little ambiguous, since there's also image editing in core. Maybe the logical combination of the two, if headed that route - "Insert into editor".

#12 in reply to: ↑ 1 @SergeyBiryukov
12 years ago

Replying to evansolomon:

20192.diff ignores the "Insert Into Post" strings in _insert_into_post_button() because the function isn't used anywhere.

Related: #20427

#13 in reply to: ↑ 11 @evansolomon
12 years ago

Replying to helenyhou:

"Send to editor" might be a little ambiguous, since there's also image editing in core. Maybe the logical combination of the two, if headed that route - "Insert into editor".

I don't think "Insert into editor" clarifies ambiguity in "Send to editor" -- the multiple editors issue still exists. While technically the button does sense an image to an editor, I think we'd be remiss to not talk about the post (or whatever content type) since that's the public-facing thing that someone is making on that page.

#14 @nacin
12 years ago

  • Keywords ux-feedback removed
  • Milestone changed from Awaiting Review to 3.5

This whole workflow is likely going to change with #21390, but this is something we should keep in mind. Closing #16924.

#15 @georgestephanis
12 years ago

Update on ~ (as it's been about four months):

Button still says Insert into post

Currently hasn't been resolved.

#16 @nacin
12 years ago

For 3.5, this would likely be best as a simple filter, like apply_filters( 'insert_into_post', __( 'Insert into post' ), $post );.

@ocean90
12 years ago

#17 @ocean90
12 years ago

21092.patch lets you change all strings, also insertIntoPost.

#18 @nacin
12 years ago

I can go for this. It seems like 21092.patch should side-step filtering $settings, lest something get broken, and focus squarely on labels?

#19 @nacin
12 years ago

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

In 22702:

Add media_view_settings and media_view_strings hooks. fixes #21092 for 3.5. see #21390.

#20 @nacin
12 years ago

In 22735:

Media: For pages, use 'Insert into page' rather than 'Insert into post'.

Custom post types can use the media_view_strings filter. No new post type "labels" for now.

see #22712. see #21092 (and #19696).

#21 @nacin
12 years ago

That first ticket number (#22712) was supposed to be #22514.

#22 @scribu
12 years ago

  • Cc scribu added

#23 @ocean90
12 years ago

  • Keywords needs-patch added; has-patch removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

On post-new.php:

Notice: Undefined variable: post_id in /wp-admin/edit-form-advanced.php on line 29

#24 @nacin
12 years ago

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

In 22742:

Use a variable that is set a few lines up. $post_id is set only in post.php, not post-new.php. fixes #21092.

@ocean90
12 years ago

#25 @DrewAPicture
12 years ago

  • Keywords needs-codex added

New filters [22702]:

  • media_view_settings
  • media_view_strings

http://codex.wordpress.org/Plugin_API/Filter_Reference

3.5 Checklist

Note: See TracTickets for help on using tickets.