Refactor menu internals #1867
Refactor menu internals #1867
Conversation
ppot
force-pushed
the
part1/keymap
branch
2 times, most recently
from
658c60c to
72e0cd0
Compare
app/index.js
Outdated
| } | ||
| })); | ||
| const makeMenu = () => { | ||
| const menu = new AppMenu(createWindow, () => { |
There was a problem hiding this comment.
This menu should be decotated by plugins
app/config.js
Outdated
| let configDir = homedir(); | ||
| if (isDev) { | ||
| let configDir = _paths.homeDir; | ||
| if (_paths.isDev) { |
There was a problem hiding this comment.
Maybe it could be better to abstract this "config path if we are in dev mode" in config/paths. Strange to move isDev in it and not this dev path mechanism.
isDev is an environnement notion, not a path one. maybe better to separate it from paths
app/menus/menu.js
Outdated
| if (process.platform === 'darwin') { | ||
| this.darwinMenu = darwinMenu(accelerators, configFile); | ||
| } else { | ||
| this.editMenu.submenu.push( |
There was a problem hiding this comment.
It could be better to add this submenu directly in editMenu
app/menus/menu.js
Outdated
| } | ||
| ); | ||
|
|
||
| this.helpMenu.submenu.push( |
There was a problem hiding this comment.
It could be better to add this submenu directly in helpMenu
app/menus/menu.js
Outdated
| const helpMenu = require('./menus/help'); | ||
| const darwinMenu = require('./menus/darwin'); | ||
|
|
||
| module.exports = class Menu { |
There was a problem hiding this comment.
I don't understand why you made a class here. Do you plan to extend it?
app/menus/menu.js
Outdated
| this.windowMenu = windowMenu(accelerators); | ||
| this.helpMenu = helpMenu(os, app, shell); | ||
|
|
||
| const configFile = path.resolve(config.getConfigDir(), _paths.conf); |
There was a problem hiding this comment.
Maybe config/paths should know this absolutePath
There was a problem hiding this comment.
As my previous comment : see previous comment
app/menus/menu.js
Outdated
| } | ||
|
|
||
| const menu = [].concat( | ||
| this.shellMenu, |
There was a problem hiding this comment.
We should avoid using this. I think it is ok to concat directly like:
const menu = [].concat(
shellMenu(accelerators, createWindow),
editMenu(accelerators, _paths.confPath),
...
)There was a problem hiding this comment.
Yeah. I kept this since it was in a class before.
app/menus/menu.js
Outdated
| @@ -0,0 +1,39 @@ | |||
| const _paths = require('../config/paths'); | |||
| const {accelerators} = require('../accelerators'); | |||
There was a problem hiding this comment.
Maybe submenus could require paths and accelerators directly when needed. I don't know. What is the advantage to do this here ?
There was a problem hiding this comment.
Using this will only be a temp fix waiting keymaps commands.
Keymaps sub implementation
Include menus refactor under
/menus