Add a option for toggling vertical tabs in tabstrip/toolbar #26108
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
jagadeshjai
commented
Oct 19, 2024
|
@simonhong Could you kindly review this PR? |
simonhong
reviewed
Oct 25, 2024
chromium_src/chrome/browser/ui/views/frame/system_menu_model_builder.h
Outdated
Show resolved
Hide resolved
jagadeshjai
force-pushed
the
feature_add_vertical_tabs_option_in_toolbar
branch
from
2a7fbf7 to
71e9017
Compare
jagadeshjai
force-pushed
the
feature_add_vertical_tabs_option_in_toolbar
branch
from
71e9017 to
5509cf2
Compare
|
Running CI - #26270 |
|
Got all green from CI - #26270 |
jagadeshjai
force-pushed
the
feature_add_vertical_tabs_option_in_toolbar
branch
from
5509cf2 to
8946dcd
Compare
|
cc: @iefremov |
bsclifton
force-pushed
the
feature_add_vertical_tabs_option_in_toolbar
branch
from
8946dcd to
a94ff8d
Compare
goodov
approved these changes
Dec 4, 2024
…ommand in tab_strip/toolbar.
jagadeshjai
force-pushed
the
feature_add_vertical_tabs_option_in_toolbar
branch
from
a94ff8d to
2c1ea72
Compare
Chromium change:
https://chromium.googlesource.com/chromium/src/+/f1147f74e2db4891bd99bd6a5d0feffe8b453ab2
commit f1147f74e2db4891bd99bd6a5d0feffe8b453ab2
Author: Joseph Park <[email protected]>
Date: Fri Oct 18 17:39:17 2024 +0000
Move SimpleMenuModel out of ui/base
This is a precursor CL to allow SimpleMenuModel to include actions.h, in order to be able to create menu items from action items.
Currently, simple_menu_model cannot include actions, as doing so creates a dependency cycle from ui/base -> ui/actions -> ui/base.
Many of the changes here are simply renaming include paths from ui/base/models -> ui/menus/models. The only significant changes here that need to be reviewed are changes in BUILD.gn and DEPS files.
Change-Id: I345efc6c42bbf2d7fdd539c0b312a8f5db338382
bridiver
reviewed
Jan 6, 2025
| ui::SimpleMenuModel* model) { | ||
| if (tabs::utils::SupportsVerticalTabs(browser())) { | ||
| std::optional<size_t> task_manager_index = | ||
| model->GetIndexOfCommandId(IDC_TASK_MANAGER); |
There was a problem hiding this comment.
this doesn't seem right to insert after IDC_TASK_MANAGER since that is gated on CanOpenTaskManager. We don't want this to just coincidentally line up with SupportsVerticalTabs for desktop. Why aren't we just appending it to the end? Or placing it after something like IDC_BOOKMARK_ALL_TABS where it seems to make more sense (grouping all tab items together)?
There was a problem hiding this comment.
Yeah, I feel it might be better to place it after IDC_BOOKMARK_ALL_TABS(i.e "Bookmark All tabs..).
@rebron Could you kindly confirm?
There was a problem hiding this comment.
After IDC_BOOKMARK_ALL_TABS works for me. Let's do that.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Resolves brave/brave-browser#41721
OutCome
brave_vertical_option_in_toolbar.webm
Submitter Checklist:
QA/YesorQA/No;release-notes/includeorrelease-notes/exclude;OS/...) to the associated issuenpm run test -- brave_browser_tests,npm run test -- brave_unit_testswikinpm run presubmitwiki,npm run gn_check,npm run tslintgit rebase master(if needed)Reviewer Checklist:
gnAfter-merge Checklist:
changes has landed on
Test Plan: