Bookmark fixes for 0.12.5 #4756
Conversation
| <BookmarkFolderItem bookmarkFolder={bookmarkFolder} | ||
| this.props.isRoot && bookmarkFolder.get('parentFolderId') === -1 | ||
| ? null | ||
| : <BookmarkFolderItem bookmarkFolder={bookmarkFolder} | ||
| allBookmarkFolders={this.props.allBookmarkFolders} |
There was a problem hiding this comment.
This check is so that folders that are children of "Other Bookmarks" don't show under "Bookmarks Toolbar". Since the above check (line 50) was removed, it will now bind ALL bookmarks to this control.
| const childBookmarkFolders = this.props.bookmarkFolder.get('folderId') === -1 | ||
| ? [] | ||
| : this.props.allBookmarkFolders | ||
| const childBookmarkFolders = this.props.allBookmarkFolders |
There was a problem hiding this comment.
Can this still return -1? If so won't a .filter() throw?
There was a problem hiding this comment.
Good eye 😄 The folderId of -1 is reserved for the "Other Bookmarks" folder (just like 0 is reserved for "Bookmarks Toolbar"). This binding creates an item for each child by matching folderId with the parentFolderId bound to this control. This does match successfully for "Other Bookmarks", but not at the root level (the one below which has the isRoot check, which is where "Other Bookmarks" is isolated to, the bottom)
Sorry that was an awfully big mouthful 😄 (hopefully it makes sense!)
There was a problem hiding this comment.
I use -1 for "Other Bookmarks" in importer too.
|
Code LGTM! I posted that one concern but as long as that's not a problem ++ from me! |
| if (bookmark) { | ||
| // Don't allow a bookmark folder to be moved into itself | ||
| if (bookmark.get('folderId') === this.props.bookmarkFolder.get('folderId')) { |
There was a problem hiding this comment.
we have another check somewhere that checks all decedents but I don't remember where off hand. Is this a strong enough check? I.e. could someone do something like this?
A
--- B
--- C
And then move A into C to create a circular ref?
There was a problem hiding this comment.
we have something like this in the moveSite code; I'll make sure that code is rock solid by working on some unit tests 😄
|
merging, thanks |
|
++ |
git rebase -ito squash commits (if needed).Auditors: @jkup @darkdh
Other Bookmarks
Other Bookmarks in about:bookmarks now displays sub-folders properly
Fixes #4685
Test Plan:
Fix disappearing bookmark folders
Bookmark folders can no longer be moved into themselves (which causes them to disappear).
(This was a long-standing bug and this commit adds a test to ensure it's not possible)
Fixes #4751
Test Plan:
This was hard to reproduce... I would suggest trying to drag and drop a lot in the UI. @alexwykoff had captured the steps as dragging a bookmark folder into the right pane. The bookmark folders should never disappear