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

Commit

Permalink
Fix bookmark drag and drop after order cache changes
Browse files Browse the repository at this point in the history
Fix #11040

Auditors: @NejcZdovc
  • Loading branch information
bbondy committed Sep 21, 2017
1 parent 954f0ab commit 0c0524c
Show file tree
Hide file tree
Showing 5 changed files with 16 additions and 3 deletions.
4 changes: 4 additions & 0 deletions app/common/lib/bookmarkUtil.js
Original file line number Diff line number Diff line change
Expand Up @@ -68,15 +68,18 @@ const getDNDBookmarkData = (state, bookmarkKey) => {
}

let oldBookmarks
let oldBookmarkOrderCache
let oldFolders
let lastValue
let lastWidth
const getToolbarBookmarks = (state) => {
const windowWidth = window.innerWidth
const allBookmarks = bookmarksState.getBookmarks(state)
const bookmarkOrderCache = bookmarksState.getBookmarkOrder(state)
const allFolders = bookmarkFoldersState.getFolders(state)
if (
allBookmarks === oldBookmarks &&
bookmarkOrderCache === oldBookmarkOrderCache &&
allFolders === oldFolders &&
lastWidth === windowWidth &&
lastValue
Expand All @@ -85,6 +88,7 @@ const getToolbarBookmarks = (state) => {
}

oldBookmarks = allBookmarks
oldBookmarkOrderCache = bookmarkOrderCache
oldFolders = allFolders
lastWidth = windowWidth

Expand Down
4 changes: 4 additions & 0 deletions app/common/state/bookmarksState.js
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,10 @@ const bookmarksState = {
return state.getIn([STATE_SITES.BOOKMARKS, key], Immutable.Map())
},

getBookmarkOrder: (state) => {
return state.getIn(STATE_SITES.BOOKMARK_ORDER_PATH)
},

/**
* Use this function if you only have a key and don't know if key is for folder or regular bookmark
* @param state
Expand Down
3 changes: 1 addition & 2 deletions app/renderer/components/bookmarks/bookmarksToolbar.js
Original file line number Diff line number Diff line change
Expand Up @@ -61,8 +61,7 @@ class BookmarksToolbar extends React.Component {
if (droppedOn.selectedRef) {
const isRightSide = !dnd.isLeftSide(ReactDOM.findDOMNode(droppedOn.selectedRef), e.clientX)
const droppedOnKey = droppedOn.selectedRef.props.bookmarkKey
const isDestinationParent = droppedOn.selectedRef.props.isFolder && droppedOn && droppedOn.isDroppedOn

const isDestinationParent = droppedOn.selectedRef.state.isFolder && droppedOn && droppedOn.isDroppedOn
if (bookmark.get('type') === siteTags.BOOKMARK_FOLDER) {
appActions.moveBookmarkFolder(bookmark.get('key'), droppedOnKey, isRightSide, isDestinationParent)
} else {
Expand Down
3 changes: 2 additions & 1 deletion js/constants/stateConstants.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,8 @@ const STATE_SITES = {
BOOKMARKS: 'bookmarks',
BOOKMARK_FOLDERS: 'bookmarkFolders',
HISTORY_SITES: 'historySites',
PINNED_SITES: 'pinnedSites'
PINNED_SITES: 'pinnedSites',
BOOKMARK_ORDER_PATH: ['cache', 'bookmarkOrder']
}

module.exports = {
Expand Down
5 changes: 5 additions & 0 deletions test/unit/app/common/state/bookmarksStateTest.js
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,11 @@ const stateWithData = Immutable.fromJS({
})

describe('bookmarkState unit test', function () {
describe('getBookmarkOrder', function () {
it('resturns order state', function () {
assert.deepEqual(bookmarksState.getBookmarkOrder(stateWithData), stateWithData.getIn(['cache', 'bookmarkOrder']))
})
})
describe('getBookmarksByParentId', function () {
it('null case', function () {
const result = bookmarksState.getBookmarksByParentId(stateWithData)
Expand Down

5 comments on commit 0c0524c

@NejcZdovc
Copy link
Contributor

Choose a reason for hiding this comment

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

@bbondy This can be reverted, because ordering is fixed with #10325 and it's a known issue. Can you just fix this line with this PR const isDestinationParent = droppedOn.selectedRef.state.isFolder && droppedOn && droppedOn.isDroppedOn

@bbondy
Copy link
Member Author

@bbondy bbondy commented on 0c0524c Sep 29, 2017

Choose a reason for hiding this comment

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

It's not clear to me, you want to keep only the props => state change or the rest?

@NejcZdovc
Copy link
Contributor

Choose a reason for hiding this comment

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

@NejcZdovc
Copy link
Contributor

Choose a reason for hiding this comment

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

As you can see when we merged text calc there is no more getToolbarBookmarks https://github.com/brave/browser-laptop/blob/master/app/common/lib/bookmarkUtil.js

@bbondy
Copy link
Member Author

@bbondy bbondy commented on 0c0524c Oct 3, 2017

Choose a reason for hiding this comment

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

OK thanks, I left it for 0.19.x and 0.20.x since it's still used there but for master I removed the unused things. 9cfeae0

Please sign in to comment.