Skip to content
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

Add filter to allow Tiled Galleries on the new Core media gallery widget #8025

Merged
merged 2 commits into from Oct 20, 2017

Conversation

@dereksmart
Copy link
Member

commented Oct 19, 2017

Fixes #7830

Thank you @westonruter for this patch, which, once merged, allows us to seamlessly integrate TG functionality into the new Core widget.

To Test:

  • Activate Tiled Galleries
  • On a site running WP trunk, add a core media gallery widget
  • Choose a type of gallery
  • Make sure it saves the value correctly and displays the right type on the front end
  • Make sure there is no conflicts when not running trunk
@westonruter

This comment has been minimized.

Copy link
Contributor

commented Oct 19, 2017

Core patch committed to trunk in r41951 for 4.9-beta.

@jeherve jeherve added this to the 5.5 milestone Oct 20, 2017

$schema['type'] = array(
'type' => 'string',
'enum' => array( 'default', 'rectangular', 'square', 'circle', 'columns', 'slideshow' ),
'description' => __( 'Type of gallery.', 'jetpack' ),

This comment has been minimized.

Copy link
@eliorivero

eliorivero Oct 20, 2017

Contributor

Needs moar esc_html__

This comment has been minimized.

Copy link
@westonruter

westonruter Oct 20, 2017

Contributor

Why? This should be handled when being output depending on the context. If this is being sent back in the REST API as part of JSON, you wouldn't want HTML entities in here.

This comment has been minimized.

Copy link
@eliorivero

eliorivero Oct 20, 2017

Contributor

If this is a string that would be displayed in front end sometime, and because of that, __ was added, then it needs esc_html__ since it's safer: it would protect against malicious HTML potentially inserted in a translation file.

This comment has been minimized.

Copy link
@oskosk

oskosk Oct 20, 2017

Contributor

In this case looks like esc_html is used when this is rendered as an option's text here.

This comment has been minimized.

Copy link
@westonruter

westonruter Oct 20, 2017

Contributor

It's safest to practice late-escaping.

This comment has been minimized.

Copy link
@eliorivero

eliorivero Oct 21, 2017

Contributor

Thanks oh I see, thanks for pointing that @oskosk
And yes Weston, I super agree with that, I was out of context since I was missing this piece that Oscar referenced.

@oskosk

oskosk approved these changes Oct 20, 2017

Copy link
Contributor

left a comment

Tested and works Ok!

@oskosk oskosk merged commit 62f4288 into master Oct 20, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@oskosk oskosk deleted the add/tiled-galleries-to-core-media-gallery-widget branch Oct 20, 2017

jeherve added a commit that referenced this pull request Oct 24, 2017

@oskosk

This comment has been minimized.

Copy link
Contributor

commented Nov 9, 2017

@lezama @enejb should this be a concern for how we sync stuff? New filter added in core and we're using it now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.