Skip to content
This repository has been archived by the owner on Dec 11, 2019. It is now read-only.

Hide bookmark bar when removing last item #14048

Merged
merged 2 commits into from
May 8, 2018
Merged

Hide bookmark bar when removing last item #14048

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
27bdb96
Hide bookmark bar when removing last item
petemill May 8, 2018
9b3379f
Refactored bookmarks toolbar closing code + tests
bsclifton May 8, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions app/browser/reducers/bookmarkFoldersReducer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,11 +11,16 @@ const bookmarkFoldersState = require('../../common/state/bookmarkFoldersState')
// Constants
const appConstants = require('../../../js/constants/appConstants')
const {STATE_SITES} = require('../../../js/constants/stateConstants')
const settings = require('../../../js/constants/settings')

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

// Utils
const {makeImmutable} = require('../../common/state/immutableUtil')
const syncUtil = require('../../../js/state/syncUtil')
const bookmarkFolderUtil = require('../../common/lib/bookmarkFoldersUtil')
const {getSetting} = require('../../../js/settings')

const bookmarkFoldersReducer = (state, action, immutableAction) => {
action = immutableAction || makeImmutable(action)
Expand Down Expand Up @@ -82,6 +87,12 @@ const bookmarkFoldersReducer = (state, action, immutableAction) => {

const destinationDetail = bookmarksState.findBookmark(state, action.get('destinationKey'))
state = syncUtil.updateObjectCache(state, destinationDetail, STATE_SITES.BOOKMARK_FOLDERS)

// close bookmark bar when going to 0
const bookmarkBarItemCount = bookmarksState.getBookmarksWithFolders(state, 0).size
if (bookmarkBarItemCount === 0 && getSetting(settings.SHOW_BOOKMARKS_TOOLBAR, state.get('settings'))) {
appActions.changeSetting(settings.SHOW_BOOKMARKS_TOOLBAR, false)
}
break
}
case appConstants.APP_REMOVE_BOOKMARK_FOLDER:
Expand All @@ -103,6 +114,12 @@ const bookmarkFoldersReducer = (state, action, immutableAction) => {
state = bookmarkFoldersState.removeFolder(state, folderKey)
state = syncUtil.updateObjectCache(state, folder, STATE_SITES.BOOKMARK_FOLDERS)
}

// close bookmark bar when going to 0
const bookmarkBarItemCount = bookmarksState.getBookmarksWithFolders(state, 0).size
if (bookmarkBarItemCount === 0 && getSetting(settings.SHOW_BOOKMARKS_TOOLBAR, state.get('settings'))) {
appActions.changeSetting(settings.SHOW_BOOKMARKS_TOOLBAR, false)
}
break
}
}
Expand Down
16 changes: 16 additions & 0 deletions app/browser/reducers/bookmarksReducer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,12 +10,17 @@ const bookmarksState = require('../../common/state/bookmarksState')
// Constants
const appConstants = require('../../../js/constants/appConstants')
const {STATE_SITES} = require('../../../js/constants/stateConstants')
const settings = require('../../../js/constants/settings')

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

// Utils
const {makeImmutable} = require('../../common/state/immutableUtil')
const syncUtil = require('../../../js/state/syncUtil')
const bookmarkUtil = require('../../common/lib/bookmarkUtil')
const bookmarkLocationCache = require('../../common/cache/bookmarkLocationCache')
const {getSetting} = require('../../../js/settings')

const bookmarksReducer = (state, action, immutableAction) => {
action = immutableAction || makeImmutable(action)
Expand Down Expand Up @@ -89,6 +94,12 @@ const bookmarksReducer = (state, action, immutableAction) => {

const destinationDetail = bookmarksState.findBookmark(state, action.get('destinationKey'))
state = syncUtil.updateObjectCache(state, destinationDetail, STATE_SITES.BOOKMARKS)

// close bookmark bar when going to 0
const bookmarkBarItemCount = bookmarksState.getBookmarksWithFolders(state, 0).size
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A little nit-picky... but could we move this code to app/common/lib/bookmarkUtils? Since it's the same as the code above in bookmarkFoldersReducer.js, each of these could be replaced with a single call (maybe the method would be called like closeBookmarksToolbarIfEmpty or something like that)

Should make the logic easier to unit test too 😄 👍

if (bookmarkBarItemCount === 0 && getSetting(settings.SHOW_BOOKMARKS_TOOLBAR, state.get('settings'))) {
appActions.changeSetting(settings.SHOW_BOOKMARKS_TOOLBAR, false)
}
break
}
case appConstants.APP_REMOVE_BOOKMARK:
Expand All @@ -106,6 +117,11 @@ const bookmarksReducer = (state, action, immutableAction) => {
state = bookmarksState.removeBookmark(state, bookmarkKey)
}
state = bookmarkUtil.updateActiveTabBookmarked(state)
// close bookmark bar when going to 0
const bookmarkBarItemCount = bookmarksState.getBookmarksWithFolders(state, 0).size
if (bookmarkBarItemCount === 0 && getSetting(settings.SHOW_BOOKMARKS_TOOLBAR, state.get('settings'))) {
appActions.changeSetting(settings.SHOW_BOOKMARKS_TOOLBAR, false)
}
break
}
}
Expand Down
2 changes: 1 addition & 1 deletion js/settings.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ const getDefaultSetting = (settingKey, settingsCollection) => {
}

const resolveValue = (settingKey, settingsCollection) => {
if (settingsCollection && settingsCollection.constructor === Immutable.Map &&
if (settingsCollection && Immutable.Map.isMap(settingsCollection) &&
settingsCollection.get(settingKey) !== undefined) {
return settingsCollection.get(settingKey)
}
Expand Down
35 changes: 30 additions & 5 deletions test/unit/app/browser/reducers/bookmarksReducerTest.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
* 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/. */

/* global describe, it, before, after, afterEach */
/* global describe, it, before, after, beforeEach, afterEach */
const mockery = require('mockery')
const Immutable = require('immutable')
const assert = require('assert')
Expand All @@ -11,6 +11,8 @@ const fakeElectron = require('../../../lib/fakeElectron')
const fakeAdBlock = require('../../../lib/fakeAdBlock')

const appConstants = require('../../../../../js/constants/appConstants')
const appActions = require('../../../../../js/actions/appActions')
const settingsConstants = require('../../../../../js/constants/settings')
const siteTags = require('../../../../../js/constants/siteTags')
require('../../../braveUnit')

Expand All @@ -31,7 +33,8 @@ describe('bookmarksReducer unit test', function () {
bookmarkLocation: {}
},
historySites: {},
tabs: []
tabs: [],
settings: {}
})

const stateWithData = Immutable.fromJS({
Expand Down Expand Up @@ -133,7 +136,10 @@ describe('bookmarksReducer unit test', function () {
}
},
historySites: {},
tabs: []
tabs: [],
settings: {
[settingsConstants.SHOW_BOOKMARKS_TOOLBAR]: true
}
})

before(function () {
Expand All @@ -144,6 +150,7 @@ describe('bookmarksReducer unit test', function () {
})
mockery.registerMock('electron', fakeElectron)
mockery.registerMock('ad-block', fakeAdBlock)
mockery.registerMock('../../../js/actions/appActions', appActions)
bookmarksReducer = require('../../../../../app/browser/reducers/bookmarksReducer')
bookmarksState = require('../../../../../app/common/state/bookmarksState')
bookmarkLocationCache = require('../../../../../app/common/cache/bookmarkLocationCache')
Expand Down Expand Up @@ -516,13 +523,19 @@ describe('bookmarksReducer unit test', function () {

describe('APP_REMOVE_BOOKMARK', function () {
let spy
let onChangeSettingSpy

beforeEach(function () {
spy = sinon.spy(bookmarksState, 'removeBookmark')
onChangeSettingSpy = sinon.spy(appActions, 'changeSetting')
})

afterEach(function () {
spy.restore()
onChangeSettingSpy.restore()
})

it('null case', function () {
spy = sinon.spy(bookmarksState, 'removeBookmark')
const newState = bookmarksReducer(state, {
actionType: appConstants.APP_REMOVE_BOOKMARK
})
Expand All @@ -531,7 +544,6 @@ describe('bookmarksReducer unit test', function () {
})

it('check if delete is working', function () {
spy = sinon.spy(bookmarksState, 'removeBookmark')
const newState = bookmarksReducer(stateWithData, {
actionType: appConstants.APP_REMOVE_BOOKMARK,
bookmarkKey: 'https://clifton.io/|0|0'
Expand All @@ -548,6 +560,19 @@ describe('bookmarksReducer unit test', function () {
.deleteIn(['cache', 'bookmarkLocation', 'https://clifton.io/'])
assert.equal(spy.calledOnce, true)
assert.deepEqual(newState.toJS(), expectedState.toJS())
console.log('settings', newState.get('settings').toJS(), newState.getIn(['settings', settingsConstants.SHOW_BOOKMARKS_TOOLBAR]))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whoops, left a console.log in there 😄

assert.ok(onChangeSettingSpy.neverCalledWith(settingsConstants.SHOW_BOOKMARKS_TOOLBAR, false), 'bookmarks toolbar enabled setting is unaffected by removing 1 bookmark')
})

it('hides bookmarks toolbar when all toolbar bookmarks are removed', function () {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need a test for bookmarksReducer too?

let newState = stateWithData
for (const bookmarkKey of stateWithData.get('bookmarks').keys()) {
newState = bookmarksReducer(newState, {
actionType: appConstants.APP_REMOVE_BOOKMARK,
bookmarkKey
})
}
assert.ok(onChangeSettingSpy.calledWith(settingsConstants.SHOW_BOOKMARKS_TOOLBAR, false))
})
})
})