Skip to content

Commit

Permalink
Refactors MenuBar and MenuBarItem into redux component
Browse files Browse the repository at this point in the history
Resolves brave#8652

Auditors: @bsclifton @bridiver

Test Plan:
- open menu on windows
- try clicking around
- everything should behave as did before
  • Loading branch information
NejcZdovc committed May 3, 2017
1 parent fd8ecb0 commit b8c397f
Show file tree
Hide file tree
Showing 7 changed files with 130 additions and 58 deletions.
29 changes: 19 additions & 10 deletions app/common/state/contextMenuState.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,25 +12,34 @@ const validateState = function (state) {
}

const contextMenuState = {
setContextMenu: (state, detail) => {
setContextMenu: (windowState, detail) => {
detail = makeImmutable(detail)
state = validateState(state)
windowState = validateState(windowState)

if (!detail) {
if (state.getIn(['contextMenuDetail', 'type']) === 'hamburgerMenu') {
state = state.set('hamburgerMenuWasOpen', true)
if (windowState.getIn(['contextMenuDetail', 'type']) === 'hamburgerMenu') {
windowState = windowState.set('hamburgerMenuWasOpen', true)
} else {
state = state.set('hamburgerMenuWasOpen', false)
windowState = windowState.set('hamburgerMenuWasOpen', false)
}
state = state.delete('contextMenuDetail')
windowState = windowState.delete('contextMenuDetail')
} else {
if (!(detail.get('type') === 'hamburgerMenu' && state.get('hamburgerMenuWasOpen'))) {
state = state.set('contextMenuDetail', detail)
if (!(detail.get('type') === 'hamburgerMenu' && windowState.get('hamburgerMenuWasOpen'))) {
windowState = windowState.set('contextMenuDetail', detail)
}
state = state.set('hamburgerMenuWasOpen', false)
windowState = windowState.set('hamburgerMenuWasOpen', false)
}

return state
return windowState
},

selectedIndex: (windowState) => {
const selectedIndex = windowState.getIn(['ui', 'contextMenu', 'selectedIndex'])
return (typeof selectedIndex === 'object' &&
Array.isArray(selectedIndex) &&
selectedIndex.length > 0)
? selectedIndex
: null
}
}

Expand Down
15 changes: 15 additions & 0 deletions app/common/state/menuBarState.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
/* This Source Code Form is subject to the terms of the Mozilla Public
* License, v. 2.0. If a copy of the MPL was not distributed with this file,
* You can obtain one at http://mozilla.org/MPL/2.0/. */

const {isWindows} = require('../lib/platformUtil')
const {getSetting} = require('../../../js/settings')
const settings = require('../../../js/constants/settings')

const api = {
isMenuBarVisible: (windowState) => {
return isWindows() && (!getSetting(settings.AUTO_HIDE_MENU) || windowState.getIn(['ui', 'menubar', 'isVisible']))
}
}

module.exports = api
48 changes: 28 additions & 20 deletions app/renderer/components/navigation/menuBar.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
const React = require('react')

// Components
const ImmutableComponent = require('../immutableComponent')
const ReduxComponent = require('../reduxComponent')
const MenuBarItem = require('./menuBarItem')

// Constants
Expand All @@ -14,6 +14,9 @@ const keyCodes = require('../../../common/constants/keyCodes')
// Actions
const windowActions = require('../../../../js/actions/windowActions')

// State
const contextMenuState = require('../../../common/state/contextMenuState.js')

// Utils
const {separatorMenuItem} = require('../../../common/commonMenu')
const {showContextMenu} = require('../../../common/lib/menuUtil')
Expand All @@ -24,7 +27,7 @@ const {wrappingClamp} = require('../../../common/lib/formatUtil')
* First intended use is with Windows to enable a slim titlebar.
* NOTE: the system menu is still created and used in order to keep the accelerators working.
*/
class MenuBar extends ImmutableComponent {
class MenuBar extends React.Component {
constructor () {
super()
this.onKeyDown = this.onKeyDown.bind(this)
Expand All @@ -43,11 +46,15 @@ class MenuBar extends ImmutableComponent {
* Used to position the context menu object.
*/
getMenubarItemBounds (index) {
if (typeof index !== 'number') index = this.props.selectedIndex
if (typeof index !== 'number') {
index = this.props.selectedIndex
}

const selected = document.querySelectorAll('.menubar .menubarItem[data-index=\'' + index + '\']')
if (selected.length === 1) {
return selected.item(0).getBoundingClientRect()
}

return null
}

Expand Down Expand Up @@ -112,31 +119,32 @@ class MenuBar extends ImmutableComponent {
}
}

shouldComponentUpdate (nextProps, nextState) {
return this.props.selectedIndex !== nextProps.selectedIndex ||
this.props.template !== nextProps.template
mergeProps (state, dispatchProps, ownProps) {
const currentWindow = state.get('currentWindow')
const contextMenuDetail = currentWindow.get('contextMenuDetail')

const props = {}
// used in renderer
props.template = state.getIn(['menu', 'template'])

// used in other functions
props.selectedIndex = currentWindow.getIn(['ui', 'menubar', 'selectedIndex'])
props.contextMenuSelectedIndex = contextMenuState.selectedIndex(currentWindow)
props.contextMenuDetail = !!contextMenuDetail
props.lastFocusedSelector = currentWindow.getIn(['ui', 'menubar', 'lastFocusedSelector'])

return Object.assign({}, ownProps, props)
}

render () {
let i = 0
return <div className='menubar'>
{
this.props.template.map((menubarItem) => {
let props = {
label: menubarItem.get('label'),
index: i++,
submenu: menubarItem.get('submenu').toJS(),
lastFocusedSelector: this.props.lastFocusedSelector,
menubar: this
}
if (props.index === this.props.selectedIndex) {
props.selected = true
}
return <MenuBarItem {...props} />
this.props.template.map((item, i) => {
return <MenuBarItem index={i} />
})
}
</div>
}
}

module.exports = MenuBar
module.exports = ReduxComponent.connect(MenuBar)
33 changes: 25 additions & 8 deletions app/renderer/components/navigation/menuBarItem.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,15 +5,15 @@
const React = require('react')

// Components
const ImmutableComponent = require('../immutableComponent')
const ReduxComponent = require('../reduxComponent')

// Actions
const windowActions = require('../../../../js/actions/windowActions')

// Utils
const {showContextMenu} = require('../../../common/lib/menuUtil')

class MenuBarItem extends ImmutableComponent {
class MenuBarItem extends React.Component {
constructor () {
super()
this.onClick = this.onClick.bind(this)
Expand All @@ -24,27 +24,44 @@ class MenuBarItem extends ImmutableComponent {
if (e && e.stopPropagation) {
e.stopPropagation()
}

// If clicking on an already selected item, deselect it
const selected = this.props.menubar.props.selectedIndex
if (selected && selected === this.props.index) {
if (this.props.selected) {
windowActions.setContextMenuDetail()
windowActions.setMenuBarSelectedIndex()
return
}

// Otherwise, mark item as selected and show its context menu
const rect = e.target.getBoundingClientRect()
windowActions.setMenuBarSelectedIndex(this.props.index)
windowActions.setContextMenuSelectedIndex()
const rect = e.target.getBoundingClientRect()
showContextMenu(rect, this.props.submenu, this.props.lastFocusedSelector)
}

onMouseOver (e) {
const selected = this.props.menubar.props.selectedIndex
if (typeof selected === 'number' && selected !== this.props.index) {
if (!this.props.selected) {
this.onClick(e)
}
}

mergeProps (state, dispatchProps, ownProps) {
const currentWindow = state.get('currentWindow')
const selectedIndex = currentWindow.getIn(['ui', 'menubar', 'selectedIndex'])
const template = state.getIn(['menu', 'template', ownProps.index])

const props = {}
// used in renderer
props.selected = ownProps.index === selectedIndex
props.label = template.get('label')

// used in other functions
props.submenu = template.get('submenu') && template.get('submenu').toJS()
props.lastFocusedSelector = currentWindow.getIn(['ui', 'menubar', 'lastFocusedSelector'])

return Object.assign({}, ownProps, props)
}

render () {
return <span
className={'menubarItem' + (this.props.selected ? ' selected' : '')}
Expand All @@ -56,4 +73,4 @@ class MenuBarItem extends ImmutableComponent {
}
}

module.exports = MenuBarItem
module.exports = ReduxComponent.connect(MenuBarItem)
11 changes: 1 addition & 10 deletions app/renderer/components/navigation/navigator.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ const ipc = electron.ipcRenderer
// Actions
const appActions = require('../../../../js/actions/appActions')
const windowActions = require('../../../../js/actions/windowActions')
const getSetting = require('../../../../js/settings').getSetting

// Components
const ImmutableComponent = require('../immutableComponent')
Expand Down Expand Up @@ -39,7 +38,6 @@ const cx = require('../../../../js/lib/classSet')

// Constants
const messages = require('../../../../js/constants/messages')
const settings = require('../../../../js/constants/settings')
const appConfig = require('../../../../js/constants/appConfig')

class Navigator extends ImmutableComponent {
Expand Down Expand Up @@ -197,7 +195,6 @@ class Navigator extends ImmutableComponent {
const activeTabShowingMessageBox = !!(activeTab && tabState.isShowingMessageBox(this.props.appState, activeTab.get('tabId')))
const activeFrame = frameStateUtil.getActiveFrame(this.props.windowState)
const totalBlocks = activeFrame ? this.getTotalBlocks(activeFrame) : false
const contextMenuDetail = this.props.windowState.get('contextMenuDetail')
const braverySettings = siteSettings.activeSettings(this.props.activeSiteSettings, this.props.appState, appConfig)
const shieldEnabled = braveShieldsEnabled(activeFrame)

Expand All @@ -209,13 +206,7 @@ class Navigator extends ImmutableComponent {
{
this.props.customTitlebar.menubarVisible
? <div className='menubarContainer'>
<MenuBar
template={this.props.customTitlebar.menubarTemplate}
selectedIndex={this.props.customTitlebar.menubarSelectedIndex}
contextMenuSelectedIndex={this.props.customTitlebar.contextMenuSelectedIndex}
contextMenuDetail={contextMenuDetail}
autohide={getSetting(settings.AUTO_HIDE_MENU)}
lastFocusedSelector={this.props.customTitlebar.lastFocusedSelector} />
<MenuBar />
<WindowCaptionButtons windowMaximized={this.props.customTitlebar.isMaximized} />
</div>
: null
Expand Down
17 changes: 7 additions & 10 deletions js/components/main.js
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,6 @@ const settings = require('../constants/settings')
const dragTypes = require('../constants/dragTypes')
const keyCodes = require('../../app/common/constants/keyCodes')
const keyLocations = require('../../app/common/constants/keyLocations')
const isWindows = process.platform === 'win32'
const {bookmarksToolbarMode} = require('../../app/common/constants/settingsEnums')

// State handling
Expand All @@ -59,6 +58,7 @@ const defaultBrowserState = require('../../app/common/state/defaultBrowserState'
const shieldState = require('../../app/common/state/shieldState')
const tabState = require('../../app/common/state/tabState')
const siteSettingsState = require('../../app/common/state/siteSettingsState')
const menuBarState = require('../../app/common/state/menuBarState')

// Util
const _ = require('underscore')
Expand All @@ -67,7 +67,7 @@ const eventUtil = require('../lib/eventUtil')
const siteSettings = require('../state/siteSettings')
const debounce = require('../lib/debounce')
const {getCurrentWindowId, isMaximized, isFocused, isFullScreen} = require('../../app/renderer/currentWindow')
const platformUtil = require('../../app/common/lib/platformUtil')
const {isDarwin, isWindows} = require('../../app/common/lib/platformUtil')

class Main extends ImmutableComponent {
constructor () {
Expand All @@ -93,14 +93,13 @@ class Main extends ImmutableComponent {
}
registerWindowLevelShortcuts () {
// For window level shortcuts that don't work as local shortcuts
const isDarwin = platformUtil.isDarwin()
document.addEventListener('keydown', (e) => {
switch (e.which) {
case keyCodes.ESC:
this.exitFullScreen()
break
case keyCodes.F12:
if (!isDarwin) {
if (!isDarwin()) {
ipc.emit(messages.SHORTCUT_ACTIVE_FRAME_TOGGLE_DEV_TOOLS)
}
break
Expand Down Expand Up @@ -653,13 +652,11 @@ class Main extends ImmutableComponent {
}

get customTitlebar () {
const customTitlebarEnabled = isWindows
const captionButtonsVisible = customTitlebarEnabled
const menubarVisible = customTitlebarEnabled && (!getSetting(settings.AUTO_HIDE_MENU) || this.props.windowState.getIn(['ui', 'menubar', 'isVisible']))
const menubarVisible = menuBarState.isMenuBarVisible(this.props.windowState)
const selectedIndex = this.props.windowState.getIn(['ui', 'contextMenu', 'selectedIndex'])
return {
enabled: customTitlebarEnabled,
captionButtonsVisible: captionButtonsVisible,
enabled: isWindows(),
captionButtonsVisible: isWindows(),
menubarVisible: menubarVisible,
menubarTemplate: menubarVisible ? this.props.appState.getIn(['menu', 'template']) : null,
menubarSelectedIndex: this.props.windowState.getIn(['ui', 'menubar', 'selectedIndex']),
Expand Down Expand Up @@ -858,7 +855,7 @@ class Main extends ImmutableComponent {
draggingOverData={this.props.appState.getIn(['dragData', 'dragOverData', 'draggingOverType']) === dragTypes.BOOKMARK && this.props.appState.getIn(['dragData', 'dragOverData'])}
showFavicon={showFavicon}
showOnlyFavicon={showOnlyFavicon}
shouldAllowWindowDrag={shouldAllowWindowDrag && !isWindows}
shouldAllowWindowDrag={shouldAllowWindowDrag && !isWindows()}
activeFrameKey={(activeFrame && activeFrame.get('key')) || undefined}
windowWidth={window.innerWidth}
contextMenuDetail={contextMenuDetail}
Expand Down
35 changes: 35 additions & 0 deletions test/unit/app/common/state/contextMenuStateTest.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
/* global describe, it */
const Immutable = require('immutable')
const contextMenuState = require('../../../../../app/common/state/contextMenuState.js')
const assert = require('chai').assert

const defaultWindowState = Immutable.fromJS({
ui: {
contextMenu: {
selectedIndex: null
}
}
})

describe('contextMenuState', function () {
describe('selectedIndex', function () {
it('returns null if selectedIndex is not set', function () {
const result = contextMenuState.selectedIndex(defaultWindowState)
assert.equal(result, null)
})

it('returns null if selectedIndex is only number', function () {
const index = 0
const newState = defaultWindowState.setIn(['ui', 'contextMenu', 'selectedIndex'], index)
const result = contextMenuState.selectedIndex(newState)
assert.equal(result, null)
})

it('returns array of selected index', function () {
const index = [0]
const newState = defaultWindowState.setIn(['ui', 'contextMenu', 'selectedIndex'], index)
const result = contextMenuState.selectedIndex(newState)
assert.equal(result, index)
})
})
})

0 comments on commit b8c397f

Please sign in to comment.