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
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Fix #14047 The bookmark bar has always remained 'visible' when removing the last item. It just happened to be reduced to a few pixels tall. This was a defect as it affected the vertical rhythm of the UI. With #13758, the toolbar always remains the set height. With this change, the toolbar is hidden automatically when the last item is removed, just like it already automatically shows when the first item is added. Test Plan: Automated - `npm run unittest -- --grep 'bookmarksReducer'` Manual - Add several bookmark items, including folders - Remove some items - observe the toolbar is still shown - Move some items - observe the toolbar is still shown - Delete all items - observe the toolbar is not shown anymore - Recreate items, and use 'move' to put all items in a folder that is not on the bookmark bar - observe the toolbar is not shown anymore
bsclifton
reviewed
May 8, 2018
| @@ -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])) | |||
There was a problem hiding this comment.
Whoops, left a console.log in there 😄
bsclifton
reviewed
May 8, 2018
| @@ -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 | |||
There was a problem hiding this comment.
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 😄 👍
bsclifton
reviewed
May 8, 2018
| 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 () { |
There was a problem hiding this comment.
Do we need a test for bookmarksReducer too?
bsclifton
force-pushed
the
fix/14047-hide-bookmarks-bar-no-items
branch
from
bae0fcf to
9b3379f
Compare
bsclifton
approved these changes
May 8, 2018
petemill
commented
May 8, 2018
petemill
added a commit
that referenced
this pull request
May 8, 2018
…items Hide bookmark bar when removing last item
petemill
added a commit
that referenced
this pull request
May 8, 2018
…items Hide bookmark bar when removing last item
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fix #14047
The bookmark bar has always remained 'visible' when removing the last item. It just happened to be reduced to a few pixels tall. This was a defect as it affected the vertical rhythm of the UI. With #13758, the toolbar always remains the set height. With this change, the toolbar is hidden automatically when the last item is removed, just like it already automatically shows when the first item is added.
Also fixes
getSettingso check for whether an object isImmutable.Mapcorrectly, so that it can be used in tests.Test Plan:
Automated
npm run unittest -- --grep 'bookmarksReducer'Manual
Submitter Checklist:
git rebase -ito squash commits (if needed).Test Plan:
Reviewer Checklist:
Tests