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

Add default label and accelerator for menu item roles #6190

Merged
merged 19 commits into from Jun 23, 2016

Conversation

Projects
None yet
4 participants
@kevinsawicki
Contributor

kevinsawicki commented Jun 22, 2016

Adds fallback labels and accelerators for all MenuItem roles.

This allows you to use menu item roles like so:

{
      label: 'Edit',
      submenu: [
        {
          role: 'undo'
        },
        {
          role: 'redo'
        },
        {
          type: 'separator'
        },
        {
          role: 'cut'
        },
        {
          role: 'copy'
        },
        {
          role: 'paste'
        }
}

And the accelerators and labels will be automatically provided as so:

screen shot 2016-06-22 at 2 19 30 pm

Close #2812

@kevinsawicki

This comment has been minimized.

Contributor

kevinsawicki commented Jun 22, 2016

/cc @sindresorhus 👀

@sindresorhus

This comment has been minimized.

Contributor

sindresorhus commented Jun 22, 2016

Awesome!

One of the main benefits of doing this would have been automatic localization. Doesn't look like this PR handles that. From the issue:

we can probably copy and use the resources from Chromium.

@kevinsawicki

This comment has been minimized.

Contributor

kevinsawicki commented Jun 22, 2016

One of the main benefits of doing this would have been automatic localization. Doesn't look like this PR handles that.

Nope, that will be handled in a subsequent up pull request. 🌎

about: {
get label () {
const {app} = require('electron')
return `About ${app.getName()}`

This comment has been minimized.

@sindresorhus

sindresorhus Jun 22, 2016

Contributor

Should be:

process.platform === 'linux' ? 'About' : `About ${appName}`

On Linux, they don't include the app name.

quit: {
get label () {
const {app} = require('electron')
return process.platform === 'win32' ? 'Exit' : `Quit ${app.getName()}`

This comment has been minimized.

},
delete: {
label: 'Delete',
accelerator: 'Delete',

This comment has been minimized.

@sindresorhus

sindresorhus Jun 22, 2016

Contributor

On macOS there's no accelerator.

screen shot 2016-06-22 at 23 58 50

},
pasteandmatchstyle: {
label: 'Paste and Match Style',
accelerator: 'Shift+Command+V',

This comment has been minimized.

@sindresorhus

sindresorhus Jun 22, 2016

Contributor

Shouldn't this be Shift+CmdOrCtrl+V?

}
}
exports.getDefaultLabel = function (role) {

This comment has been minimized.

@sindresorhus

sindresorhus Jun 22, 2016

Contributor

Arrow function?

const {app} = require('electron')
return `Hide ${app.getName()}`
},
accelerator: 'Command+H'

This comment has been minimized.

@sindresorhus

sindresorhus Jun 22, 2016

Contributor

Nitpick, but for consistency I would use Cmd instead of Command, or change all the CmdOrCtrl+ definitions.

},
hide: {
get label () {
const {app} = require('electron')

This comment has been minimized.

@sindresorhus

sindresorhus Jun 22, 2016

Contributor

Would be cleaner to require it at the top. You use app multiple times.

This comment has been minimized.

@kevinsawicki

kevinsawicki Jun 22, 2016

Contributor

Yeah, this requires reworking app.js a bit since putting this at the top would currently fail because of a circular require between app, menu, and menu item.

@sindresorhus

This comment has been minimized.

Contributor

sindresorhus commented Jun 22, 2016

// @danhp

kevinsawicki added some commits Jun 22, 2016

@kevinsawicki

This comment has been minimized.

Contributor

kevinsawicki commented Jun 22, 2016

@sindresorhus thanks for the review, I've incorporated your feedback 👍

@sindresorhus

This comment has been minimized.

Contributor

sindresorhus commented Jun 22, 2016

LGTM

},
togglefullscreen: {
label: 'Toggle Full Screen',
accelerator: process.platform === 'darwin' ? 'Ctrl+Command+F' : 'F11',

This comment has been minimized.

@danhp

danhp Jun 22, 2016

Member

Ctrl -> Control

@danhp

This comment has been minimized.

Member

danhp commented Jun 22, 2016

Aside from the tiny nitpick, it looks good.

@zcbenz

This comment has been minimized.

Contributor

zcbenz commented Jun 23, 2016

👍

@zcbenz zcbenz merged commit e70c622 into master Jun 23, 2016

8 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
electron-linux-arm Build #3507292 succeeded in 41s
Details
electron-linux-ia32 Build #3507293 succeeded in 35s
Details
electron-linux-x64 Build #3507294 succeeded in 121s
Details
electron-mas-x64 Build #1679 succeeded in 6 min 17 sec
Details
electron-osx-x64 Build #1687 succeeded in 7 min 0 sec
Details
electron-win-ia32 Build #680 succeeded in 6 min 26 sec
Details
electron-win-x64 Build #671 succeeded in 6 min 58 sec
Details

@zcbenz zcbenz deleted the default-label-and-accelerator branch Jun 23, 2016

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