Conversation
eliorivero
left a comment
There was a problem hiding this comment.
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 masterto catch up. The tests are not working due to this.
Thanks!
modules/shortcodes/vr.php
Outdated
| return vr_viewer_get_html( $url_params ); | ||
| } | ||
|
|
||
| return '[vr] shortcode requires a data source to be given'; |
There was a problem hiding this comment.
Is it ok to output this string? Maybe it can be displayed only to current_user_can( 'edit_posts' )? Or maybe in a comment?
|
|
||
| $this->assertContains( $img, $shortcode_content ); | ||
| } | ||
|
|
There was a problem hiding this comment.
Needs at least one more test covering the case when there's not a valid source.
modules/shortcodes/vr.php
Outdated
| 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;">'; |
There was a problem hiding this comment.
Instead of forcing the width here, what would you say about using $content_width to define the maximum width of the embeds?
modules/shortcodes/vr.php
Outdated
| } | ||
|
|
||
| // add check for user | ||
| if ( current_user_can('editor') || current_user_can('administrator') ) { |
There was a problem hiding this comment.
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!
modules/shortcodes/vr.php
Outdated
|
|
||
| return '[vr] shortcode requires a data source to be given'; | ||
| // add check for user | ||
| if ( current_user_can('editor') || current_user_can('administrator') ) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
Only thing pending for this to be ready for merge is to add inline docs to functions. |
|
I just tested adding this to a post and got a PHP notice. 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.
but we need to take into account the case where a user might inadvertently add only the URL. |
|
That actually works as intended, but seems very strange since you are using a 360 photo. 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: |
|
That's cool, what about the PHP notice? |
|
LGTM! 🐑 |
|
ported to 4.5 in 2553bc8 |
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 feature
Testing instructions:
[vr url=path-to-photo.jpg view=360]See: https://en.support.wordpress.com/embedding-360-photos-and-virtual-reality-vr-content/
Proposed changelog entry for your changes: