WordPress.org

Make WordPress Core

Opened 6 months ago

Last modified 25 hours ago

#38796 reviewing defect (bug)

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

Reported by: celloexpressions Owned by:
Milestone: Future Release 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 weeks ago.
38796.1.diff (4.6 KB) - added by celloexpressions 5 days ago.
Add translator comments for placeholders and reuse automatic labels for image controls
38796.2.diff (4.8 KB) - added by westonruter 3 days ago.
Fix reference to undefined variable $label
38796.3.diff (5.0 KB) - added by Christian1012 25 hours ago.

Download all attachments as: .zip

Change History (16)

#1 @westonruter
6 months ago

  • Milestone changed from 4.7 to 4.8

#2 @westonruter
6 months 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 weeks 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 7 weeks ago by westonruter (previous) (diff)

#4 @westonruter
7 days ago

  • Keywords needs-testing added

@celloexpressions thoughts on 38796.0.diff?

@celloexpressions
5 days ago

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

#5 @celloexpressions
5 days 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
3 days ago

Fix reference to undefined variable $label

#6 @westonruter
3 days 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.


3 days ago

#8 @westonruter
3 days 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
3 days ago

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

#10 @celloexpressions
3 days 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
3 days ago

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

#12 @Christian1012
25 hours 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.

Note: See TracTickets for help on using tickets.