-
Notifications
You must be signed in to change notification settings - Fork 887
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Introduce sidebar bookmarks page #11897
Conversation
5976611
to
1cc7f90
Compare
1cc7f90
to
b716cab
Compare
fc41836
to
5d8838c
Compare
5d8838c
to
f705751
Compare
Really curious what you think @petemill for the forking from https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/resources/read_later/side_panel/ It's great that we can make any change we'd like (and also we won't get any unexpected changes on Chromium upgrade). But I would be curious if we did want to "rebase", what that would look like. We for sure have to fork any files which are referencing the BRAVE resource IDs - but the logic like keyboard navigation (ex: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everything looks great to me 😄 Left a comment about how macOS is weird with no open folder icon... but this is consistent with existing behavior (macOS doesn't have open folder right now)
Really nice work! 😄 It works great with keyboard shortcuts too (both keyboard navigation and using keyboard shortcuts like shift + click)
Kindly ping :) |
f705751
to
870265a
Compare
@goodov Thanks for review. All addressed. PTAL again :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
fix brave/brave-browser#20500 Applied bookmark part of chromium's side panel to sidebar bookmark panel. sidebar.mojom is used for interacting between sidebar's bookmarks panel with SidebarBookmarksPageHandler. Most of webui impls in browser/resources/sidebar/bookmarks are copied from upstream and modified to show bookmark part only. (side panel's page shows read later and bookmark part both.) Bookmarks page uses custom context menu by asking to native via Sidebar:ShowCustomContextMenu interface.
870265a
to
86f1dc0
Compare
Set CI/skip label as previous run was all green. |
Resolves brave/brave-browser#20500
Applied bookmark part of chromium's side panel to sidebar bookmark panel.
sidebar.mojom
is used for interacting between sidebar's bookmarks panel withSidebarBookmarksPageHandler
.Most of webui impls in
browser/resources/sidebar/bookmarks
are copied from upstreamand modified to show bookmark part only. (side panel's page shows read later and bookmark part both.)
Bookmarks page uses custom context menu by asking to native via
Sidebar:ShowCustomContextMenu
interface.Resources from Figma:
https://www.figma.com/file/sS3l8tqUFxt54MEiBTalUI/Desktop-toolbar-and-menu?node-id=9%3A5824
macOS light and dark theme.
Linux light and dark theme.
Bookmark item's context menu
Submitter Checklist:
QA/Yes
orQA/No
;release-notes/include
orrelease-notes/exclude
;OS/...
) to the associated issuenpm run test -- brave_browser_tests
,npm run test -- brave_unit_tests
,npm run lint
,npm run gn_check
,npm run tslint
git rebase master
(if needed)Reviewer Checklist:
gn
After-merge Checklist:
changes has landed on
Test Plan: