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

Full Screen mode #2727

Merged
merged 6 commits into from
Sep 6, 2015
Merged

Conversation

PaulAnnekov
Copy link
Contributor

We can save vertical space by using Full Screen mode. It's espesially critical for laptops, or other devices with low vertical resolution. You can view the result on this video: OSM/iD in Full Screen (POC). As you can see we are removing everything (system panel, browser tabs, osm menu), except the editor and saving a lot of space.

What do you think about this? Don't review the code, it's just a proof of concept. If you will like it - I will make a TODO list and complete this task.

@manfredbrandl
Copy link
Contributor

I like the feature. As i am not so familiar with github: Is there another way to express this except that comment?

@Abbe98
Copy link
Contributor

Abbe98 commented Jul 13, 2015

Browsers implementing full-screen natively, why would iD need it's own button for it?

@pnorman
Copy link
Contributor

pnorman commented Jul 13, 2015

This branch is currently failing the tests

@PaulAnnekov
Copy link
Contributor Author

@Abbe98 according to your logic, the Full Screen API in HTML standard is redundant? I think that you should just watch my video, then go to osm.org, open iD editor and click F11 (Full Screen) and compare the result with video:

  1. Full Screen button hides the OSM menu, but F11 - not.
  2. Not all users know about F11 hotkey. The button on panel will help them.

@pnorman I have written that it's just a proof of concept and development is not completed. I don't want to waste my time for feature that will be rejected.

@bhousel
Copy link
Member

bhousel commented Jul 15, 2015

Code looks pretty good, the travis issue is just a jslint line break warning.

@samanpwbb what do you think? Is it worth having a button for this?
Even if we don't need a button, it still might be nice to keybind F11 so that iD can fullscreen itself and hide whatever it's embedded in (e.g. OSM menu)..

@PaulAnnekov
Copy link
Contributor Author

@bhousel I have not kept the coding conventions. I will update it if this feature is useful.

Not all users know about F11 shortcut. Button is much more clear. Of course we should add an icon to the button instead of text. Maybe we should make it smaller, or with completely different style.
F11 key is reserved by browser to switch current contents to full screen. I don't know how browser will behave if we will try to steal this key.

@samanpwbb
Copy link
Member

White buttons in the top bar should be reserved for primary actions only. Having the button around is ok, but:

  • It should use a fullscreen icon rather than the cryptic letters 'FS'
  • It needs to have a distinct active state.
  • What about moving it into the toolbar:

screen_shot_2015-07-16_at_10_07_35_am

@PaulAnnekov
Copy link
Contributor Author

@samanpwbb it's just a POC, of course I will add icons for different states.

What about moving it into the toolbar

As I see, that part of toolbar is focused on actions with map. Full Screen button is a feature that changes state of whole editor, not only map. It should be at least in the second part. Before or after the Help button.

@samanpwbb
Copy link
Member

As I see, that part of toolbar is focused on actions with map.

Good point, probably ok to keep it where it is.

@PaulAnnekov
Copy link
Contributor Author

Ok. So, as I understand, this feature is useful. I will continue the development.

@bhousel
Copy link
Member

bhousel commented Jul 17, 2015

Ok. So, as I understand, this feature is useful. I will continue the development.

👍

@PaulAnnekov
Copy link
Contributor Author

I have completed the development and changed the design of the button. Checked in FF/Chrome in Ubuntu and Spartan/IE11/IE9 (in IE9 button should be hidden) in Windows 10. Please, review it.
Some remarks:

  • I have used icons from this icon set: Iconic (Attribution-Non-Commercial 3.0 Netherlands)
  • Fullscreen mode will not work on http://osm.org until you add "allowfullscreen" attribute to iD's iframe.
  • Button will be hidden if browser is not supporting fullscreen mode.

Video: OSM/iD in Full Screen. Another design

@PaulAnnekov
Copy link
Contributor Author

@bhousel so? :)

@bhousel
Copy link
Member

bhousel commented Aug 7, 2015

Thanks @PaulAnnekov I'll try to check it out this weekend.

@PaulAnnekov
Copy link
Contributor Author

@bhousel :)

@bhousel
Copy link
Member

bhousel commented Sep 5, 2015

screenshot 2015-09-05 10 24 48

I took a look at this today and it really like this feature - it does make editing much better..

My concern is just that the button doesn't really look right where it is, so my plan is to leave the button code commented out for now and just keybind some of the common browser fullscreen shortcuts. This unfortunately makes it less discoverable until I do #1481, but I really like the feature and I do want to get this PR into our release next week.

@PaulAnnekov
Copy link
Contributor Author

@bhousel I don't think that intercepting of reserved F11 key is a good idea. It can be buggy... If you still want it then you should check it's work in all supported browsers.

And yes, this shortcut must be written somewhere, if we won't have the button. Good luck :).

@bhousel
Copy link
Member

bhousel commented Sep 5, 2015

I don't think that intercepting of reserved F11 key is a good idea. It can be buggy... If you still want it then you should check it's work in all supported browsers.

We intercept a lot of the browser's built in keyboard shortcuts, e.g. Cmd-Q, Cmd-S, Cmd-C, Cmd-V and others. I don't see why F11 would be any different, but I'll definitely test it.

@PaulAnnekov
Copy link
Contributor Author

@bhousel I have tested (add 'allowfullscreen' attribute to jsfiddle's iframe before F11 click) intercepting of F11 in FF and Chrome and it works pretty good.

@bhousel bhousel merged commit 739876d into openstreetmap:master Sep 6, 2015
@bhousel
Copy link
Member

bhousel commented Sep 6, 2015

Just merged it, thank you for this! I'm sorry it took a while for me to look at it..

@PaulAnnekov
Copy link
Contributor Author

@bhousel thanks, no ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants