New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Trac 32417: Media widget #215

Closed
wants to merge 22 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@westonruter
Contributor

westonruter commented Feb 13, 2017

// Everything else.
$instance['align'] = sanitize_text_field( $new_instance['align'] );
$instance['size'] = sanitize_text_field( $new_instance['size'] );
$instance['link'] = sanitize_text_field( $new_instance['link'] );

This comment has been minimized.

@westonruter

westonruter Feb 22, 2017

Contributor

These sanitizing functions should probably be more specific.

@westonruter

westonruter Feb 22, 2017

Contributor

These sanitizing functions should probably be more specific.

celloexpressions added some commits Feb 23, 2017

Add an appropriate widget icon
Other widgets that contain more specific titles but contain media will
continue to get the relevant more-specific icon with this placement in
the CSS (before image, audio, video).
Remove unused description property
This could be brought back via the attachmen's description
(post_content), but isn't being saved to the widget instance anywhere.
Might need some cleanup in JS as well.
@celloexpressions

See also forthcoming comment on trac ticket with bigger-picture items.

// Create the media frame.
widgetFrame = wp.media( {
button: {
text: translate( 'addToWidget', 'Add to widget' ) // Text of the submit button.

This comment has been minimized.

@celloexpressions

celloexpressions Feb 23, 2017

Collaborator

"Add to widget" doesn't sound right. The widget will only have one attachment associated with it; this implies that you can add multiple.

@celloexpressions

celloexpressions Feb 23, 2017

Collaborator

"Add to widget" doesn't sound right. The widget will only have one attachment associated with it; this implies that you can add multiple.

* @return {string} Translated string.
*/
function translate( key, defaultText ) {
return l10n[ key ] || defaultText;

This comment has been minimized.

@celloexpressions

celloexpressions Feb 23, 2017

Collaborator

This is nice; however, we should consider whether it's worth possible confusion (for future readers and contributors of this code) since this differs from how most core (or at least customize component) JS i18n is currently done. There's a pending proposal for a better core solution as well: https://make.wordpress.org/core/2016/10/10/javascript-internationalization.

@celloexpressions

celloexpressions Feb 23, 2017

Collaborator

This is nice; however, we should consider whether it's worth possible confusion (for future readers and contributors of this code) since this differs from how most core (or at least customize component) JS i18n is currently done. There's a pending proposal for a better core solution as well: https://make.wordpress.org/core/2016/10/10/javascript-internationalization.

'data-id' => $widget_id,
'title' => $attachment->post_title,
'class' => 'image wp-image-' . $attachment->ID,
'style' => 'width: 100%; height: auto;',

This comment has been minimized.

@celloexpressions

celloexpressions Feb 23, 2017

Collaborator

Let's not do this. The core-provided CSS should already cover this, and themes should have their own CSS doing the same thing. Inlining it is far from ideal.

@celloexpressions

celloexpressions Feb 23, 2017

Collaborator

Let's not do this. The core-provided CSS should already cover this, and themes should have their own CSS doing the same thing. Inlining it is far from ideal.

Show outdated Hide outdated src/wp-includes/widgets/class-wp-widget-media.php
* @return string
*/
private function create_link_for( $attachment, $type = '' ) {
$url = '#';

This comment has been minimized.

@celloexpressions

celloexpressions Feb 23, 2017

Collaborator

This function should probably default to providing no link, not linking to #.

@celloexpressions

celloexpressions Feb 23, 2017

Collaborator

This function should probably default to providing no link, not linking to #.

@westonruter

This comment has been minimized.

Show comment
Hide comment
@westonruter
Contributor

westonruter commented Feb 28, 2017

@westonruter westonruter deleted the trac-32417 branch Apr 28, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment