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 | Owned by: | 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)
Change History (20)
#3
@
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 todo
s. 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 );
}
@
7 years ago
Add translator comments for placeholders and reuse automatic labels for image controls
#5
@
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.
#6
@
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
@
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
@
7 years ago
- Milestone changed from 4.8.1 to 4.8
- Owner changed from westonruter to celloexpressions
#10
@
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
@
7 years ago
- Keywords good-first-bug added
- Milestone changed from 4.8 to Future Release
- Owner westonruter deleted
#12
@
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
@
7 years ago
- Milestone changed from Future Release to 4.8.1
Looks good, @Christian1012, thanks!
#15
@
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.
In 39237: