Add vr shortcode #5917

mkaz · 2016-12-15T22:46:09Z

Add VR shortcode feature

Testing instructions:

Add vr shortcode to post: [vr url=path-to-photo.jpg view=360]
Confirm the 360 photo displays properly

See: https://en.support.wordpress.com/embedding-360-photos-and-virtual-reality-vr-content/

Proposed changelog entry for your changes:

Adds VR shortcode to embed 360° photos

eliorivero

Hi @mkaz, thanks for porting this, looks awesome. In addition to the two comments in the code:

functions must be prefixed with jetpack_
functions need inline documentation
the branch needs a git rebase master to catch up. The tests are not working due to this.

Thanks!

eliorivero · 2016-12-16T14:52:43Z

modules/shortcodes/vr.php

+		return vr_viewer_get_html( $url_params );
+	}
+
+	return '[vr] shortcode requires a data source to be given';


Is it ok to output this string? Maybe it can be displayed only to current_user_can( 'edit_posts' )? Or maybe in a comment?

eliorivero · 2016-12-16T14:58:29Z

tests/modules/shortcodes/test_class.vr.php

+
+		$this->assertContains( $img, $shortcode_content );
+	}
+


Needs at least one more test covering the case when there's not a valid source.

Test added in 1b8e7cc

jeherve · 2016-12-16T18:09:51Z

modules/shortcodes/vr.php

+function jetpack_vr_viewer_get_html( $url_params ) {
+	$iframe = add_query_arg( $url_params, 'https://vr.me.sh/view/' );
+
+	$rtn  = '<div style="position: relative; max-width: 720px; margin-left: auto; margin-right: auto; overflow: hidden;">';


Instead of forcing the width here, what would you say about using $content_width to define the maximum width of the embeds?

… tests

jeherve · 2016-12-16T18:59:50Z

modules/shortcodes/vr.php

+	}
+
+	// add check for user
+	if ( current_user_can('editor') || current_user_can('administrator') ) {


Since some folks use plugins like Members to customize capabilities for existing roles or to create new roles, it might be best to look for a capability (edit_others_posts) instead of checking for 2 roles?

You'll also want to add spaces between braces and quotes, as per the WP coding standards.

Thanks!

eliorivero · 2016-12-16T19:07:28Z

modules/shortcodes/vr.php

@@ -68,7 +68,12 @@ function jetpack_vr_viewer_shortcode( $atts ) {
 		return jetpack_vr_viewer_get_html( $url_params );
 	}

-	return '[vr] shortcode requires a data source to be given';
+	// add check for user
+	if ( current_user_can('editor') || current_user_can('administrator') ) {


IMHO it's better to check specifically for edit_posts since an administrator might have been set by another admin to manage some options of the site but not edit posts, maybe due to privacy. Besides, we only need to make one check.

Another example is a new role basic_editor created with Members plugin with only the edit_posts capability. It's not editor nor administrator but it can still edit the posts.

eliorivero · 2016-12-19T17:05:00Z

Only thing pending for this to be ready for merge is to add inline docs to functions.

eliorivero · 2016-12-19T21:00:15Z

I just tested adding this to a post

[vr url=https://en-blog.files.wordpress.com/2016/12/regents_park.jpg]

and got a PHP notice.

 Notice: Undefined index: view in /.../wp-content/plugins/jetpack/modules/shortcodes/vr.php on line 74

and the image was rendered like this:

If 'view' is set by default to '360' here, the notice is gone and the image is properly rendered.
I know your testing instructions were

Add vr shortcode to post: [vr url=path-to-photo.jpg view=360]

but we need to take into account the case where a user might inadvertently add only the URL.

mkaz · 2016-12-19T21:12:01Z

That actually works as intended, but seems very strange since you are using a 360 photo.
The default view is for viewing panoramas (which we call cinema mode). It is setup this way since there are more panoramic photos than 360 photos.

So looks better if using a panoramic image URL such as this one: https://mfkaz.files.wordpress.com/2016/11/nobhill-pano.jpg

See Post at: https://mkaz.blog/2016/10/02/nob-hill-pano/

Or behavior in GIF here:

eliorivero · 2016-12-19T21:16:35Z

That's cool, what about the PHP notice?

eliorivero · 2016-12-19T21:54:16Z

LGTM! 🐑

dereksmart · 2016-12-20T14:02:09Z

ported to 4.5 in 2553bc8

@see 18012a3 Changelog: add #5976, #5978, #5983 Changelog: add #5917 Changelog: add #5832

@tyxla

Changelog: add #5867 Changelog: add #5874 Changelog: add #5905 Changelog: add #5906 Changelog: add #5931 Changelog: add #5933 Changelog: add #5934 Bring over 4.4.2 changelog from branch-4.4 @see 18012a3 Changelog: add #5976, #5978, #5983 Changelog: add #5917 Changelog: add #5832 Changelog: add 4.4.2 release post link. CHangelog: add #5457 Changelog: add #5487 Changelog: add #5708 Changelog: add #5879 Changelog: add #5932 Changelog: add #5963 Changelog: add #5968 Changelog: add #5996 Changelog: add #5998 Changelog: add #5999 Changelog: add #6012 Changelog: add #6013 Changelog: add #6014 Changelog: add #6015 Changelog: add #6023 Changelog: add #6024 Changelog: add #6030 Changelog: add #5465 CHangelog: add #6063 Changelog: add #6025 Changelog: add #5974 Changelog: add #6059 Changelog: add #6046 Changelog: add #5418 Changelog: move things around and add missing information. Changelog: add #5565 Changelog: add #6087 Changelog: add #6095 Readme: add @tyxla to the list of contributors. Improved changelog for your readability and enjoyment updated the release date finalizing the changelog with a few more edits

Add vr shortcode

1ed1cb4

mkaz added the [Status] In Progress label Dec 15, 2016

Add initial VR unit test

271979f

eliorivero suggested changes Dec 16, 2016

View reviewed changes

eliorivero added [Feature] Shortcodes / Embeds [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it labels Dec 16, 2016

mkaz added 2 commits December 16, 2016 10:04

Prefix functions with jetpack_

022ee9a

Merge branch 'master' into add/vr-shortcode

126f693

jeherve reviewed Dec 16, 2016

View reviewed changes

mkaz added 3 commits December 16, 2016 10:37

Fix location for vr shortcode test

897c612

Only show error message if user has editor||administrator permissions

ab696b0

Add test for missing URL parameter. Update jetpack version to 4.5 for…

1b8e7cc

… tests

jeherve reviewed Dec 16, 2016

View reviewed changes

eliorivero reviewed Dec 16, 2016

View reviewed changes

mkaz added 2 commits December 16, 2016 11:13

Switch permissions to 'edit_posts'

0e5e9d9

Use content_width if defined for iframe max-width

1575629

jeherve added [Status] Needs Review To request a review from Crew. Label will be renamed soon. and removed [Status] In Progress labels Dec 16, 2016

Add inline documentation

bba4e12

eliorivero approved these changes Dec 19, 2016

View reviewed changes

eliorivero added the [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! label Dec 19, 2016

Fix warnings if parameter is not set

ab22336

mkaz removed the [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! label Dec 19, 2016

eliorivero added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review To request a review from Crew. Label will be renamed soon. labels Dec 19, 2016

samhotchkiss merged commit f07dbf5 into master Dec 19, 2016

samhotchkiss deleted the add/vr-shortcode branch December 19, 2016 22:21

samhotchkiss added the [Status] Needs Changelog label Dec 19, 2016

dereksmart restored the add/vr-shortcode branch December 20, 2016 13:59

jeherve added this to the 4.5 milestone Dec 23, 2016

jeherve added [Status] Has Changelog and removed [Status] Needs Changelog labels Dec 23, 2016

jeherve added a commit that referenced this pull request Dec 23, 2016

Changelog: add #5917

7fc48c8

dereksmart pushed a commit that referenced this pull request Jan 17, 2017

Bring over 4.4.2 changelog from branch-4.4

c1e8244

@see 18012a3 Changelog: add #5976, #5978, #5983 Changelog: add #5917 Changelog: add #5832

kraftbj removed the [Status] Ready to Merge Go ahead, you can push that green button! label Oct 13, 2020

matticbot added the Touches WP.com Files label Oct 13, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add vr shortcode #5917

Add vr shortcode #5917

mkaz commented Dec 15, 2016

eliorivero left a comment

eliorivero Dec 16, 2016

eliorivero Dec 16, 2016

mkaz Dec 16, 2016

jeherve Dec 16, 2016

jeherve Dec 16, 2016

eliorivero Dec 16, 2016

eliorivero Dec 16, 2016

eliorivero commented Dec 19, 2016

eliorivero commented Dec 19, 2016 •

edited

mkaz commented Dec 19, 2016 •

edited

eliorivero commented Dec 19, 2016

eliorivero commented Dec 19, 2016

dereksmart commented Dec 20, 2016

Add vr shortcode #5917

Add vr shortcode #5917

Conversation

mkaz commented Dec 15, 2016

Add VR shortcode feature

Testing instructions:

Proposed changelog entry for your changes:

eliorivero left a comment

Choose a reason for hiding this comment

eliorivero Dec 16, 2016

Choose a reason for hiding this comment

eliorivero Dec 16, 2016

Choose a reason for hiding this comment

mkaz Dec 16, 2016

Choose a reason for hiding this comment

jeherve Dec 16, 2016

Choose a reason for hiding this comment

jeherve Dec 16, 2016

Choose a reason for hiding this comment

eliorivero Dec 16, 2016

Choose a reason for hiding this comment

eliorivero Dec 16, 2016

Choose a reason for hiding this comment

eliorivero commented Dec 19, 2016

eliorivero commented Dec 19, 2016 • edited

mkaz commented Dec 19, 2016 • edited

eliorivero commented Dec 19, 2016

eliorivero commented Dec 19, 2016

dereksmart commented Dec 20, 2016

eliorivero commented Dec 19, 2016 •

edited

mkaz commented Dec 19, 2016 •

edited