Fullscreen shortcut for non macOS

[yougotwill]

Feature added for #1711

Fullscreen on macOS remains Cmd+Ctrl+F
Fullscreen on Linux and Windows is F11

[romainlq]

Does it run on your windows/Linux ?

The window submenu is only displayed on macOs as far as I know, and the toggleFullScreen: is specific to macOs

Maybe you should try to place it in the View submenu, and use the setFullScreen method instead 😄

[yougotwill]

@romainwn Good to know thanks, will get right on that :)

[yougotwill]

Tested on windows and it works fine.

[romainlq]

Also tested on Windows and it works fine too.
Just tested it on macOs, when I click on the item it does toggle fullscreen, but shortcut cmd + ctrl + F doesn't toggle and instead focus the Search input.

Maybe we should keep macOs as it is, and only make your changes for the non macOs platforms ?
(The shortcut actually works on macOs in latest release)
What do you think @Rokt33r ?

[Rokt33r]

I think we should add another window menu for linux and windows.

[Rokt33r]

@yougotwill Could you fix it?

[yougotwill]

@Rokt33r Sure thing will get on it this evening 👍

[yougotwill]

@romainwn @Rokt33r I changed the logic to be either of the fullscreen methods depending on the platform. Having said that @romainwn that error you mentioned about it focusing on the search input is on master as well. :/

[yougotwill]

Ok so that change doesn't work on windows. So I have reverted the commit and will leave it until we find out if the issue comes from the master branch.

[romainlq]

I'll try to work on this feature as well today !

[Rokt33r]

@romainwn ping.

[romainlq]

oh yup sorry for the delay. I'll try to make a PR by this week end, quite busy for the next two days !

[Rokt33r]

I'll wait for it gladly.

[romainlq]

Sooo, I'm working on it right now, and found something interesting :
in index.js there is a shortcut to focus the search bar :

 // F or S key
    if (e.keyCode === 70 || e.keyCode === 83) {
      e.preventDefault()
      ee.emit('top:focus-search')
    }

So on macOs, I noticed this, if I do Cmd + Ctrl + F while a note is focused in edit mode, it does toggle Full screen. But otherwise, it will focus the search input.

Could we remove the F shortcut and only let the S one for the focus ?

Meanwhile, I'm working on the windows/macOs full screen issue

[yougotwill]

I'd like to suggest we change the focus on search bar shortcut to Cmd/Ctrl + L which is a more standard shortcut that I have seen in various apps for focusing the main input/search box in an app. (i.e. Chrome, Spotify)

Also @romainwn are you referring to this issue #1803 specifically a shortcut for the editor fullscreen option?

[romainlq]

Oh didn't notice this issue.
Actually I was referring to #1711, the one you were trying to implement which is toggle fullscreen for non-macOs.

For the FullScreen shortcut I was thinking to use F11 for windows and Linux, and keep Cmd + Ctrl + F for macOs

[yougotwill]

This branch does exactly that :) All we need to do is change the focus on search bar to a different keyboard shortcut so that it doesn't interfere with the fullscreen shortcut and once we confirm that everything works correctly again then we are sorted 👍

[yougotwill]

So I have tested by changing the top:focus-search as mentioned above. The fullscreen shortcut then works perfectly. @Rokt33r Should I put these changes in a separate pr and leave this one alone for now?

[Rokt33r]

@yougotwill I don't think so. Just update this branch. And I agree changing shortcut key for top:focus-search .

[yougotwill]

@Rokt33r Changes done, and I have tested on windows and everything works correctly 👍

[Rokt33r]

LGTM! 👍