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

Hide fullscreen button in Viewer when not available #666

Closed
mramato opened this issue Apr 20, 2013 · 10 comments
Closed

Hide fullscreen button in Viewer when not available #666

mramato opened this issue Apr 20, 2013 · 10 comments
Labels
good first issue An opportunity for first time contributors type - bug

Comments

@mramato
Copy link
Contributor

mramato commented Apr 20, 2013

Currently, the Fullscreen button in CesiumViewerWidget is visible whether the browser/iframe supports full screen functionality or not. The widget should check for Fullscreen support during startup and hide the button if not supported. The trick here is that the layout of the Timeline needs to change so that it stretches to fill the remaining space. I believe there are some layout issues in Timeline that need to be fixed in order to let that happen.

@khare-ashwini
Copy link

I guess there's already an explicit method to check for FullScreen support Fullscreen.supportsFullscreen .So the task is effectively reduced down to checking for it and adjusting the timeline suitably. Is the issue you refer regarding Timeline's layout reported anywhere? I can just follow up there to continue with this

@emackey
Copy link
Contributor

emackey commented Apr 21, 2013

I don't think there are serious issues there. The timeline, like most widgets, needs to be notified when its parent container changes size. Look at what its window resize handler is calling, you'll have to call that after adjusting its size.

@imylyanyk
Copy link

I was working with this issue, and eventually find out that 'fullscreen button' doesn't work in IE10.
Is that ok?
I use 'google chrome frame'.

by the way, how can I verify if my patch correct? I mean, is there are any opportunity to check that button appearance in browser which really doesn't support fullscreen mode?

Thanks in advance.

@khare-ashwini
Copy link

@cupidon4uk
I guess your approach is right but there are some flaws which I found out with that patch
1.You don't need to write a separate check for supportFullscreen . As I had mentioned in the first commit, there is already a comprehensive check. You might want to check out https://github.com/AnalyticalGraphicsInc/cesium/blob/master/Source/Core/Fullscreen.js#L33 .Not only this test considers all possible cases, it's right place as far as code organisation supporting modular development is concerned

2.You assign undefined but check using if condition. Note that it will throw a reference error instead.The predefined function returns a boolean

3.You change the style to visibility : hidden .However this property only makes the element not being visible but still occupies it's space in viewport. You might want to check out on that. Ideal property to be changed should have been display:none

It's a small patch so I've wrote the necessary of my own now instead of correcting the above.

@emackey
Your views?

@imylyanyk
Copy link

@khare-ashwini, I have noticed, that there are something wrong with your code, so, I updated my patch.
Firstly, method supportsFullScreen() doesn't exists in FeatureDetection, it should be supportsFullscreen() instead.
Secondly, in your code timeline doesn't change it's width when window is resized.
In my patch I fixed both this issues.

@mramato, @emackey Is that ok now?

@khare-ashwini
Copy link

@cupidon4uk
Thanks for pointing that out. In FF, I had to add a parseInt filter else it was giving a NaN operation while computing the available width. I've committed mine( https://github.com/khare-ashwini/cesium/commit/3e0ca9ca50f8533f7a699b2cb794aa86b86fe287 ), but guess your code might work as well. Also I forgot to ask before, why are you setting a margin of 2 pixel? I tried removing the check and running code, it works fine w/o the margin, and I don't think it's the right place so removed that in my push.

@imylyanyk
Copy link

@khare-ashwini
I set margin for two pixels, because timelineContainer has border with one pixel on each side, and in this way, it looks better, I think.
If you set margin for 0px, there will be two pixels outside the screen on the right. That's not a lot, but I think that is not what was planned.

That's how I understood it. If I'm wrong, please, somebody, correct me.

@imylyanyk
Copy link

@khare-ashwini
I've checked for 'parseInt' issue, but in FF on my PC everything goes well.

@imylyanyk
Copy link

Oh, sorry, setting up github, I made a real mess here. Sorry again.

@mramato
Copy link
Contributor Author

mramato commented May 20, 2013

Since the dojo Cesium Viewer widget is going away, and the replacement is being worked on in the Viewer branch, I'm going to just close this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue An opportunity for first time contributors type - bug
Projects
None yet
Development

No branches or pull requests

4 participants