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

Windows application menu take two #991

Merged
merged 116 commits into from Mar 28, 2017

Conversation

Projects
None yet
4 participants
@niik
Contributor

niik commented Mar 1, 2017

This implements a custom menu bar component on Windows, see #836 for more details on why.

Navigating with mouse

menubar

Navigating with keyboard

This shows me hitting Alt+F to open the file menu and then O to open Options.

menubar-access-key

Things this does not do / known limitations

Full screen

The menu bar is currently not accessible when running the app in full-screen. I plan to tackle that in a separate PR rather than drag this out any longer.

Submenus

Sub menus are not handled with any grace at the moment. They work but they do not position themselves as one would expect. We currently don't have any sub menus for top level menus so I don't plan to tackle that until it becomes a problem as it is non-trivial to implement well.

image

Fixes #836

niik added some commits Feb 28, 2017

Remove the app title
We're not using it for anything
Bump the title bar height on windows
To accomodate for icon and menu bar
Switch menus when hovered
Only when some menu is open
Capture unhandled keypresses on toolbar button instead
This allows us to capture arrow keys even though no menu items have focus

@niik niik referenced this pull request Mar 14, 2017

Merged

Add an add menu #1018

@niik

This comment has been minimized.

Show comment
Hide comment
@niik

niik Mar 27, 2017

Contributor

EDIT: having the File menu item be active when pressing ALT would actually be really great. But I'm fine to 👢 that to a follow-up PR.

Good catch, we should indeed support that scenario. I modeled the interactions after how Notepad does things as that seemed like a good example of how things should be done. I've documented some of my findings in https://github.com/desktop/desktop/blob/88e4525ac9f8f94392eff44ca35f8e891557ed8b/docs/windows-menu-bar.md

I'm pretty happy with the current state so it's time for another look

menubar-focus-opt

Contributor

niik commented Mar 27, 2017

EDIT: having the File menu item be active when pressing ALT would actually be really great. But I'm fine to 👢 that to a follow-up PR.

Good catch, we should indeed support that scenario. I modeled the interactions after how Notepad does things as that seemed like a good example of how things should be done. I've documented some of my findings in https://github.com/desktop/desktop/blob/88e4525ac9f8f94392eff44ca35f8e891557ed8b/docs/windows-menu-bar.md

I'm pretty happy with the current state so it's time for another look

menubar-focus-opt

@shiftkey

Shout out to some great docs. I'm mostly nit-picking things now.

I'd also love @joshaber or @iAmWillShepherd to pull this down and test it out a bit, just incase I've missed something.

Show outdated Hide outdated docs/windows-menu-bar.md
## Keyboard navigation
When a user is holding down the `ALT` key the access-keys (which is separate from

This comment has been minimized.

@shiftkey

shiftkey Mar 28, 2017

Member

Reviewer's note: if you're using something like Parallels here you might see the Option/ALT key behave differently during testing this - it'll raise the keydown and keyup events after you release the key on the host OS.

If you bust out your Onscreen Keyboard in the guest OS you'll see the right timing for events be sent to the app.

@shiftkey

shiftkey Mar 28, 2017

Member

Reviewer's note: if you're using something like Parallels here you might see the Option/ALT key behave differently during testing this - it'll raise the keydown and keyup events after you release the key on the host OS.

If you bust out your Onscreen Keyboard in the guest OS you'll see the right timing for events be sent to the app.

Show outdated Hide outdated docs/windows-menu-bar.md
File -> New Branch item a user should be able to press `ALT+F` followed by `B`.
Pressing the `ALT` key and releasing it without pressing an access key should
put focus on the first top level menu item if no menu is currently expanded. If

This comment has been minimized.

@shiftkey

shiftkey Mar 28, 2017

Member

This one is also relevant to your test setup, in case you're in a VM.

@shiftkey

shiftkey Mar 28, 2017

Member

This one is also relevant to your test setup, in case you're in a VM.

Show outdated Hide outdated app/src/ui/app-menu/menu-list-item.tsx
* Whether or not to render the accelerator (shortcut) next to the label.
* This can be turned off when the menu item is used as a stand-alone item
*/
readonly renderAcceleratorText?: boolean

This comment has been minimized.

@shiftkey

shiftkey Mar 28, 2017

Member

Is it worth calling out the default behaviour here when no value is defined? Both this and renderSubMenuArrow seem to default to rendering when no value set, which might be confusing to the caller...

@shiftkey

shiftkey Mar 28, 2017

Member

Is it worth calling out the default behaviour here when no value is defined? Both this and renderSubMenuArrow seem to default to rendering when no value set, which might be confusing to the caller...

This comment has been minimized.

@niik

niik Mar 28, 2017

Contributor

Good call

@niik

niik Mar 28, 2017

Contributor

Good call

Show outdated Hide outdated app/src/ui/app.tsx
private renderTitlebar() {
const winControls = __WIN32__
? <WindowControls />
: null
// On windows it's not possible to resize a frameless window if the

This comment has been minimized.

@shiftkey

shiftkey Mar 28, 2017

Member

capital W for Windows

@shiftkey

shiftkey Mar 28, 2017

Member

capital W for Windows

Show outdated Hide outdated app/src/ui/app-menu/app-menu-bar-button.tsx
private onKeyDown = (event: React.KeyboardEvent<HTMLDivElement>) => {
if (event.key === 'Escape') {
// Are we currently collapsed?

This comment has been minimized.

@shiftkey

shiftkey Mar 28, 2017

Member

Duplicate comments and checks make me think this check could be extracted a variable or function to make this easier to understand. Thoughts?

@shiftkey

shiftkey Mar 28, 2017

Member

Duplicate comments and checks make me think this check could be extracted a variable or function to make this easier to understand. Thoughts?

@niik

This comment has been minimized.

Show comment
Hide comment
@niik

niik Mar 28, 2017

Contributor

🔊

Contributor

niik commented Mar 28, 2017

🔊

@joshaber joshaber referenced this pull request Mar 28, 2017

Merged

No menu for you #1082

@joshaber

This comment has been minimized.

Show comment
Hide comment
@joshaber

joshaber Mar 28, 2017

Member

Damn, this is a hell of a thing. Works well in the cursory testing I did 👍

Member

joshaber commented Mar 28, 2017

Damn, this is a hell of a thing. Works well in the cursory testing I did 👍

? 'open'
: 'closed'
const dropDownState = this.isMenuOpen ? 'open' : 'closed'

This comment has been minimized.

@shiftkey

shiftkey Mar 28, 2017

Member

We seem to have dropped the openMenu && openMenu.id === item.id check here as part of the refactoring. No longer necessary?

@shiftkey

shiftkey Mar 28, 2017

Member

We seem to have dropped the openMenu && openMenu.id === item.id check here as part of the refactoring. No longer necessary?

This comment has been minimized.

@niik

niik Mar 28, 2017

Contributor

Nope, this is ensured at the parent component already, it was a remnant from a colder darker era

@niik

niik Mar 28, 2017

Contributor

Nope, this is ensured at the parent component already, it was a remnant from a colder darker era

This comment has been minimized.

@shiftkey

shiftkey Mar 28, 2017

Member

Then onward and upward!

@shiftkey

shiftkey Mar 28, 2017

Member

Then onward and upward!

@shiftkey shiftkey merged commit 470c123 into master Mar 28, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@shiftkey shiftkey deleted the app-menu-take-two branch Mar 28, 2017

@ZexuanTHU

This comment has been minimized.

Show comment
Hide comment
@ZexuanTHU

ZexuanTHU Jun 4, 2018

The support of cross=platform titlebar from Electron is nasty, maybe you guys should consider to PR your efforts to the Electron repo and makes it kinda a standard.

ZexuanTHU commented Jun 4, 2018

The support of cross=platform titlebar from Electron is nasty, maybe you guys should consider to PR your efforts to the Electron repo and makes it kinda a standard.

@lock lock bot locked as resolved and limited conversation to collaborators Aug 3, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.