Fix platformUtil usage in app/browser/menu.js #9990
Conversation
| const {isDarwin, isLinux} = require('../common/lib/platformUtil') | ||
| const platformUtil = require('../common/lib/platformUtil') | ||
| const isDarwin = platformUtil.isDarwin() | ||
| const isLinux = platformUtil.isLinux() |
There was a problem hiding this comment.
This is the most important part of the PR. Without this PR, isDarwin and isLinux are always truthy (because they are set to the function itself, not the return value of the function)
| @@ -530,7 +533,7 @@ class Main extends React.Component { | |||
| // used in renderer | |||
| props.isFullScreen = activeFrame.get('isFullScreen', false) | |||
| props.isMaximized = isMaximized() || isFullScreen() | |||
| props.captionButtonsVisible = isWindows() | |||
| props.captionButtonsVisible = isWindows | |||
| props.showContextMenu = !!currentWindow.get('contextMenuDetail') | |||
There was a problem hiding this comment.
This being cached at the top could potentially save some time (avoiding an extra function call and check for navigator)
There was a problem hiding this comment.
PR looks good ++ Thank you for this fix.
Just a note about imports. Personally I don't like const isWindows = require('../../../common/lib/platformUtil').isWindows(), because you lock yourself for the further use of platformUtil. If we need to cache this values I like what you did in menu.js with const platformUtil = require('../common/lib/platformUtil') and then assigning values to the appropriate OS.
…f platformUtil When #9678 was merged, unfortunately menu.js was not updated to call the function. Instead of updating to call the function, I updated all usage to resolve the value at the top of each file (preventing multiple unneeded calls) Auditors: @NejcZdovc
| } | ||
|
|
||
| module.exports.isWindows = () => { | ||
| return process.platform === 'win32' || | ||
| navigator.platform === 'Win32' | ||
| (navigator && navigator.platform === 'Win32') | ||
| } |
There was a problem hiding this comment.
These checks are put in place because if the first one fails (ex: calling isWindows on mac) from the browser process, it checks navigator (which was undefined)... throwing an error when calling navigator.platform
I followed up on travis tests and noticed they were failing :( Auditors: @NejcZdovc
I followed up on travis tests and noticed they were failing :( Auditors: @NejcZdovc
I followed up on travis tests and noticed they were failing :( Auditors: @NejcZdovc
...and standardize usage of platformUtil
When #9678 was merged, unfortunately menu.js was not updated to call the function.
Instead of updating to call the function, I updated all usage to resolve the value at the top of each file (preventing multiple unneeded calls)
Auditors: @NejcZdovc
Submitter Checklist:
git rebase -ito squash commits (if needed).Reviewer Checklist:
Tests