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

Bookmark bar stays visible after all bookmarks are deleted #14047

Closed
btlechowski opened this issue May 7, 2018 · 2 comments · Fixed by #14048
Closed

Bookmark bar stays visible after all bookmarks are deleted #14047

btlechowski opened this issue May 7, 2018 · 2 comments · Fixed by #14048
Assignees
@petemill
Labels
bug feature/bookmarks QA/checked-Linux QA/checked-macOS QA/checked-Win64 QA/test-plan-specified release-notes/include
Milestone
0.22.x Release 3 ...

Comments

@btlechowski
Copy link
Contributor

Steps to Reproduce

  1. Clean install
  2. Click the star
  3. Click 'Remove'

Actual result:
Bookmark bar stays visible
image

Expected result:
Bookmark bar is hidden

Reproduces how often:
100%

Brave Version

Brave: 0.22.702 
V8: 6.6.346.26 
rev: e4a853d9990c62328a8a4320fec21f58c9cdc0c1 
Muon: 6.0.7 
OS Release: 4.13.0-21-generic 
Update Channel: Beta 
OS Architecture: x64 
OS Platform: Linux 
Node.js: 7.9.0 
Brave Sync: v1.4.2 
libchromiumcontent: 66.0.3359.139

Also reproduced on Mac and Windows by @srirambv @LaurenWags @kjozwiak

Reproducible on current live release:
No. Tested v0.22.669

Cc @bsclifton @petemill
@btlechowski btlechowski added feature/bookmarks regression 0.22.x-single-webview Issue first seen on single-webview build against v0.22.x branch labels May 7, 2018
@btlechowski btlechowski added this to the 0.22.x Release 3 (Beta channel) milestone May 7, 2018
@petemill
Copy link
Member

petemill commented May 8, 2018

Note that this is also sort of reproducible in the current release, except that in the current release, the bookmarks toolbar gets really short, which itself was considered a bug since a user could not have the bookmarks toolbar with normal height with no bookmark items in order to interact with it.
However, I'd be happy to look at fixing for this release, since it's now more obvious with the fix from #13758

@petemill petemill self-assigned this May 8, 2018
@petemill petemill removed regression 0.22.x-single-webview Issue first seen on single-webview build against v0.22.x branch labels May 8, 2018
petemill added a commit that referenced this issue May 8, 2018
@petemill
Hide bookmark bar when removing last item
27bdb96 
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
@petemill petemill mentioned this issue May 8, 2018
Merged
10 tasks
petemill added a commit that referenced this issue May 8, 2018
@petemill
Hide bookmark bar when removing last item
5d480be 
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
petemill added a commit that referenced this issue May 8, 2018
@petemill
Hide bookmark bar when removing last item
efc67f1 
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
@btlechowski
Copy link
Contributor Author

btlechowski commented May 9, 2018 •
edited by srirambv
Loading

Verified on Ubuntu 17.10 x64

  • 0.22.703 903b8d0
  • libchromiumcontent 66.0.3359.139
  • muon: 6.0.8

Verified on macOS 10.13.3 x64 using the following build:

  • 0.22.703 903b8d0
  • libchromiumcontent: 66.0.3359.139
  • muon: 6.0.8

Verified on Windows x64

  • 0.22.703 903b8d0
  • libchromiumcontent 66.0.3359.139
  • muon: 6.0.8

@kjozwiak kjozwiak mentioned this issue May 16, 2018
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@petemill petemill

Labels
bug feature/bookmarks QA/checked-Linux QA/checked-macOS QA/checked-Win64 QA/test-plan-specified release-notes/include
Projects
None yet
Milestone
0.22.x Release 3 (Release Channel)
Development

Successfully merging a pull request may close this issue.

Hide bookmark bar when removing last item brave/browser-laptop
4 participants
@petemill @kjozwiak @srirambv @btlechowski