Add fullscreen out of Google VRView iframe #212

rafa8626 · 2017-07-06T12:09:42Z

@lincolnfrog and @tommytee this is the code of what I was talking about on #178. When I click play it doesn't go fullscreen using WorldRenderer's code. If you can advise me on how to achieve this I'd appreciate it. Thanks

…favor custom button for fullscreen

lincolnfrog · 2017-07-10T18:15:35Z

I fixed the bug where it doesn't go to fullscreen (including these changes) here: #219. Please review.

rafa8626 · 2017-07-10T18:19:07Z

I see: I missed setting the message's SET_FULLSCREEN in the receiver. You stated that there's an issue in this PR still right? The one related to using ESC to get out of the fullscreen. That happens because the control bar is not visible, and since we removed the button from the iframe is expected unless we change CSS styles for the control bar when using fullscreen. I can create the styles for that but it was intended that way.

lincolnfrog · 2017-07-10T20:01:15Z

It still creates a bug where the icon is wrong when you hit escape. Do you still want to try to merge this into master? I am of the opinion that this change is fairly custom and most users of vrview will be happy with the default fullscreen behavior. Thoughts?

rafa8626 · 2017-07-10T20:03:20Z

Could be. I just picture the scenario when you need to use GoogleVRView with any other existing major player and you need to adapt now code to make it work. If we had this PR it could facilitate integration with them. Just my 2 cents

lincolnfrog · 2017-07-10T20:08:41Z

I am not opposed to merging this PR if we can iron out the details better. In my view, here are the things I would like to see:

add my fix for the SET_FULLSCREEN case statement in the message handler
Fix the bug where when you hit escape the icon doesn't change back to "expand" state
Figure out a way to make the original default fullscreen behavior be the default and allow people to opt-in to this custom handling control paradigm.

If you can meet those three items, I would be interested in merging this.

rafa8626 · 2017-07-10T20:11:44Z

Sounds good. I'll work on these fixes and I'll give you an update soon. The point 3 is gonna take me some time but I see the value on that. Right now if you just use hideButton as false it will make the fullscreen and VR buttons appear, and that's the original behavior. You need something else on that?

lincolnfrog · 2017-07-10T20:26:25Z

I think the best thing would be to have your new button not appear by default and the original embedded fullscreen button be the only thing. You can add a section to the README.md about how to enable the custom fullscreen UI and disable the embedded UI. Does that make sense?

rafa8626 · 2017-07-10T20:35:24Z

Sounds good to me. As long as I can state that is possible to hide the fullscreen button and have a custom one I'm OK with this. So I'll take care of 1 and 2, and add a note on README for 3. Is that ok?

lincolnfrog · 2017-07-10T21:19:07Z

Note on README + default functionality is the way it is now unless you follow the toggle instructions in README, right?

…

On Mon, Jul 10, 2017 at 1:35 PM, Rafael Miranda ***@***.***> wrote: Sounds good to me. As long as I can state that is possible to hide the fullscreen button and have a custom one I'm OK with this. So I'll take care of 1 and 2, and add a note on README for 3. Is that ok? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#212 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AHml0R2UbFdTFu5KmyDBWTzGlZHI-HfSks5sMosPgaJpZM4OPi7Q> .

rafa8626 · 2017-07-10T21:20:15Z

In the example I deactivated the fullscreen so I just need to remove that piece of code to be the default functionality. Is that what you meant?

lincolnfrog · 2017-07-10T21:23:43Z

Yes, I believe so.

…

On Mon, Jul 10, 2017 at 2:20 PM, Rafael Miranda ***@***.***> wrote: In the example I deactivated the fullscreen so I just need to remove that piece of code to be the default functionality. Is that what you meant? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#212 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AHml0XNc4FbOJpNEiAEAb-ikg9hykIZ6ks5sMpWRgaJpZM4OPi7Q> .

rafa8626 · 2017-07-10T21:25:29Z

OK I'll work on this and I'll let you know when it's ready to review once more. Thanks for your help

…ontrol bar

rafa8626 · 2017-07-11T17:07:09Z

OK @lincolnfrog the changes we discussed yesterday are now in place; let me know if you need anything else. Thanks

lincolnfrog · 2017-07-11T22:57:13Z

README.md

@@ -4,6 +4,28 @@ VR View allows you to embed 360 degree VR media into websites on desktop and
 mobile. For more information, please read the documentation available at
 <http://developers.google.com/cardboard/vrview>.

+# Configuration


This is great!

lincolnfrog

This is great! Minor change - please make the param for disabling fullscreen button more descriptive. Happy to merge this after. Thanks for creating that great table of params to the readme!

lincolnfrog · 2017-07-11T22:58:03Z

README.md

+`default_yaw` | Number | (Optional) Numeric angle in degrees of the initial heading for the 360° content. By default, the camera points at the center of the underlying image.
+`is_yaw_only` | Boolean | (Optional) When true, prevents roll and pitch. This is intended for stereo panoramas.
+`loop` | Boolean | (Optional) When false, stops the loop in the video  
+`hide_button` | Boolean | (Optional) When true, the fullscreen on video are hidden. Useful when user requires to implement his own fullscreen button


this is an overly-generic name. hide_fullscreen_button?

rafa8626 · 2017-07-11T23:12:23Z

Ok I'll change the name of the configuration and I'll try to come up with a better language for it in the table. I'll get this done in a little

rafa8626 · 2017-07-11T23:28:12Z

OK changes in place. Let me know your thoughts. Thanks for the feedback

rafa8626 · 2017-07-12T00:17:46Z

Thanks

Rafael Miranda added 2 commits July 6, 2017 08:02

Merge remote-tracking branch 'upstream/master'

15dcff9

Added fullscreen capability and new option to hide iframe buttons to …

0394f72

…favor custom button for fullscreen

rafa8626 changed the title ~~Add fullscreen out of Google VRView iframe [NOT READY YET]~~ Add fullscreen out of Google VRView iframe [ON WORK] Jul 6, 2017

rafa8626 mentioned this pull request Jul 7, 2017

How to load callbacks for VR and Fullscreen buttons? #178

Closed

Added missing fix for fullscreen method and removed fullscreen from c…

f0aca29

…ontrol bar

lincolnfrog reviewed Jul 11, 2017

View reviewed changes

lincolnfrog suggested changes Jul 11, 2017

View reviewed changes

Renamed parameter and refined language on description of new parameter

fa648a1

rafa8626 changed the title ~~Add fullscreen out of Google VRView iframe [ON WORK]~~ Add fullscreen out of Google VRView iframe Jul 11, 2017

lincolnfrog merged commit 4e8e57e into googlearchive:master Jul 11, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add fullscreen out of Google VRView iframe #212

Add fullscreen out of Google VRView iframe #212

rafa8626 commented Jul 6, 2017

lincolnfrog commented Jul 10, 2017

rafa8626 commented Jul 10, 2017

lincolnfrog commented Jul 10, 2017

rafa8626 commented Jul 10, 2017

lincolnfrog commented Jul 10, 2017

rafa8626 commented Jul 10, 2017

lincolnfrog commented Jul 10, 2017

rafa8626 commented Jul 10, 2017

lincolnfrog commented Jul 10, 2017 via email

rafa8626 commented Jul 10, 2017

lincolnfrog commented Jul 10, 2017 via email

rafa8626 commented Jul 10, 2017

rafa8626 commented Jul 11, 2017

lincolnfrog Jul 11, 2017

lincolnfrog left a comment

lincolnfrog Jul 11, 2017

rafa8626 commented Jul 11, 2017

rafa8626 commented Jul 11, 2017

rafa8626 commented Jul 12, 2017

Add fullscreen out of Google VRView iframe #212

Add fullscreen out of Google VRView iframe #212

Conversation

rafa8626 commented Jul 6, 2017

lincolnfrog commented Jul 10, 2017

rafa8626 commented Jul 10, 2017

lincolnfrog commented Jul 10, 2017

rafa8626 commented Jul 10, 2017

lincolnfrog commented Jul 10, 2017

rafa8626 commented Jul 10, 2017

lincolnfrog commented Jul 10, 2017

rafa8626 commented Jul 10, 2017

lincolnfrog commented Jul 10, 2017 via email

rafa8626 commented Jul 10, 2017

lincolnfrog commented Jul 10, 2017 via email

rafa8626 commented Jul 10, 2017

rafa8626 commented Jul 11, 2017

lincolnfrog Jul 11, 2017

Choose a reason for hiding this comment

lincolnfrog left a comment

Choose a reason for hiding this comment

lincolnfrog Jul 11, 2017

Choose a reason for hiding this comment

rafa8626 commented Jul 11, 2017

rafa8626 commented Jul 11, 2017

rafa8626 commented Jul 12, 2017