Add fullscreen out of Google VRView iframe

[rafa8626]

@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

[lincolnfrog]

I fixed the bug where it doesn't go to fullscreen (including these changes) here: https://github.com/googlevr/vrview/pull/219. Please review.

[rafa8626]

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]

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]

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]

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: 1) add my fix for the SET_FULLSCREEN case statement in the message handler 2) Fix the bug where when you hit escape the icon doesn't change back to "expand" state 3) 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]

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]

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]

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]

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 notifications@github.com 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 https://github.com/googlevr/vrview/pull/212#issuecomment-314228607, or mute the thread https://github.com/notifications/unsubscribe-auth/AHml0R2UbFdTFu5KmyDBWTzGlZHI-HfSks5sMosPgaJpZM4OPi7Q .

[rafa8626]

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]

Yes, I believe so.

On Mon, Jul 10, 2017 at 2:20 PM, Rafael Miranda notifications@github.com 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 https://github.com/googlevr/vrview/pull/212#issuecomment-314249398, or mute the thread https://github.com/notifications/unsubscribe-auth/AHml0XNc4FbOJpNEiAEAb-ikg9hykIZ6ks5sMpWRgaJpZM4OPi7Q .

[rafa8626]

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

[rafa8626]

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

[rafa8626]

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]

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

[rafa8626]

Thanks