Make WordPress Core

Opened 8 years ago

Closed 7 years ago

#38796 closed defect (bug) (fixed)

Customize media control button labels should automatically reflect the specified mime type

Reported by: celloexpressions's profile celloexpressions Owned by: westonruter's profile westonruter
Milestone: 4.9 Priority: normal
Severity: normal Version: 4.7
Component: Customize Keywords: good-first-bug has-patch
Focuses: Cc:

Description

[39234] updated the button labels for the header video control to reflect that it's used for videos, rather than media in general. However, this approach only fixes this one control instance, and is something that should be brought to all media controls.

For 4.7, we should add an inline note where [39234] is referencing this ticket and explaining that in the future all media controls will default to labeling the buttons based on the mime type (and as a reminder to remove those overrides). Then, this ticket can transition to a future release/4.8 enhancement for updating the default button labels to use the mime_type where possible, and fall back to the existing defaults with media. We'll need an array of known mime types mapped to the corresponding media type labels and should be able to substitute those into each of the default button label strings in the control constructor.

Attachments (4)

38796.0.diff (3.1 KB) - added by Christian1012 8 years ago.
38796.1.diff (4.6 KB) - added by celloexpressions 7 years ago.
Add translator comments for placeholders and reuse automatic labels for image controls
38796.2.diff (4.8 KB) - added by westonruter 7 years ago.
Fix reference to undefined variable $label
38796.3.diff (5.0 KB) - added by Christian1012 7 years ago.

Download all attachments as: .zip

Change History (20)

#1 @westonruter
8 years ago

  • Milestone changed from 4.7 to 4.8

#2 @westonruter
8 years ago

In 39237:

Customize: Only show video header controls if previewing front page; show explanatory notice when controls are hidden.

Also include todo for the header_video control's button_labels. See #38796.

Props westonruter, joemcgill, celloexpressions.
Fixes #38778.

#3 @Christian1012
8 years ago

  • Keywords has-patch added; needs-patch removed

Added 38796.0.diff.

I don't have tests setup locally for trunk, plus I noticed that tests for WP_Customize_Control are still todos. I could write tests but would need a little help on where to put them. Plus I didn't want to write too many until I got feedback on the first pass.

/**
 * Test WP_Customize_Media_Control::get_default_button_labels() when mime type is 'video'.
 * 
 * @see WP_Customize_Media_Control::get_default_button_labels()
 */
public function test_get_default_button_labels_with_video_mime_type() {
        // Decalare expectations.
        $expected = array(
                'select'       => 'Select Video',
                'change'       => 'Change Video',
                'default'      => 'Default',
                'remove'       => 'Remove',
                'placeholder'  => 'No video selected',
                'frame_title'  => 'Select Video',
                'frame_button' => 'Choose Video',
        );

        // Act.
        // Assumes TestCase container has wp_customize property, instance of WP_Customize_Manager.
        // See /trunk/tests/phpunit/tests/customize/control.php for example that this is based on.
        do_action( 'customize_register', $this->wp_customize );
        $control = new WP_Customize_Media_Control( $this->wp_customize, 'nosetting', array(
                'settings' => array( 'nosetting' ),
                'mime_type' => 'video',
        ) );
        
        // Unit.
        $this->assertEquals( $expected, $control->get_default_button_labels() );

        // Functional.
        $this->assertEquals( $expected, $control->button_labels );
}
Last edited 8 years ago by westonruter (previous) (diff)

#4 @westonruter
7 years ago

  • Keywords needs-testing added

@celloexpressions thoughts on 38796.0.diff?

@celloexpressions
7 years ago

Add translator comments for placeholders and reuse automatic labels for image controls

#5 @celloexpressions
7 years ago

  • Keywords commit added; needs-testing removed
  • Owner set to westonruter
  • Status changed from new to reviewing

38796.1.diff builds on 38796.0.diff to remove the type-specific label overwrites for WP_Customize_Image_Control and add translator comments for placeholders. This should be ready for commit.

@westonruter
7 years ago

Fix reference to undefined variable $label

#6 @westonruter
7 years ago

  • Keywords commit removed

Humm. I wonder actually if this refactoring is going to cause problems for translators. For example, there may be languages with verb/object agreement whereby the verb needs to change based on the object being acted upon. This is the same problem we had with trying to translate strings with post type names embedded in them. See #37895. Need input from i18n.

This ticket was mentioned in Slack in #core-i18n by westonruter. View the logs.


7 years ago

#8 @westonruter
7 years ago

  • Keywords needs-patch added; has-patch removed
  • Milestone changed from 4.8 to 4.8.1

Unfortunately this isn't going to work for translation purposes. Is there an alternative refactor that would settle on a better way to keep all of the 4 sets of strings distinct?

#9 @westonruter
7 years ago

  • Milestone changed from 4.8.1 to 4.8
  • Owner changed from westonruter to celloexpressions

#10 @celloexpressions
7 years ago

  • Owner changed from celloexpressions to westonruter

I'm not going to be available to investigate further in the near future in terms of patching. My thought would be to just write out all of the strings for the four primary media/file types here, rather than trying to split out the one word. That fixes this issue and also leaves room for continued future improvement.

#11 @westonruter
7 years ago

  • Keywords good-first-bug added
  • Milestone changed from 4.8 to Future Release
  • Owner westonruter deleted

#12 @Christian1012
7 years ago

  • Keywords has-patch added; needs-patch removed

My thought would be to just write out all of the strings for the four primary media/file types here, rather than trying to split out the one word. That fixes this issue and also leaves room for continued future improvement.

Added 38796.3.diff.

Supports full translatable strings for the three supported mime types (audio, video, image) and the default (file) without tokens. I believe this is what @celloexpressions and @westonruter are advocating.

#13 @celloexpressions
7 years ago

  • Milestone changed from Future Release to 4.8.1

Looks good, @Christian1012, thanks!

#14 @westonruter
7 years ago

  • Milestone changed from 4.8.1 to 4.9

#15 @netweb
7 years ago

  • Owner set to westonruter

This ticket has a patch, setting the owner to that of the person who added the good-first-bug keyword, this ticket update causes the ticket to be removed from the "Unclaimed" section of the Good First Bug Trac Report 44.

#16 @westonruter
7 years ago

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

In 41550:

Customize: Let media control button labels better automatically reflect the specified MIME type.

Props Christian1012, celloexpressions, westonruter.
Fixes #38796.

Note: See TracTickets for help on using tickets.