Move Vertical tab settings to brave://settings #18146
Conversation
* Create a Tab section in brave://settings#appearance * Make a vertical tab related settings * Move other tab related settings to the same section * Remove context menus related to vertical tabs
github-actions
bot
added
the
CI/storybook-url
|
A Storybook has been deployed to preview UI for the latest push |
| @@ -315,6 +317,18 @@ const PrefsUtil::TypedPrefMap& BravePrefsUtil::GetAllowlistedKeys() { | |||
| // NFT pinning pref | |||
| (*s_brave_allowlist)[kAutoPinEnabled] = | |||
| settings_api::PrefType::PREF_TYPE_BOOLEAN; | |||
|
|
|||
| #if defined(TOOLKIT_VIEWS) | |||
There was a problem hiding this comment.
Just double checking we need the TOOLKIT_VIEWS check here - this file is in extensions, so it might only exist on desktop 😆
There was a problem hiding this comment.
Yeah, I'm not sure about it. @simonhong , what do you think about this? kSidebarShowOption is guarded with TOOLKIT_VIEWS too, though it seems to be replacement of ENABLE_SIDEBAR.
There was a problem hiding this comment.
Hmm.. I picked TOOLKIT_VIEWS to mark sidebar is desktop UI feature.
Previously, ENABLE_SIDEBAR was used but realized that both are defined with same condition.
So, redundant ENABLE_SIDEBAR is removed and all things outside of views is replaced with TOOLKIT_VIEWS.
Actually, removing this will work but I want to use it here because this file brave_prefs_util.cc is not guarded by toolkit_views in the gn file. If this file is guarded by toolkit_views in gn, I think we don't need to use this flag.
There was a problem hiding this comment.
Yeah, unless we have assert(tookit_views) in browser/extensions/BUILD.gn, it'd be safer to have TOOLKIT_VIEWS in sources like this. FYI, the BUILD.gn also has conditional with toolkit_views.
if (toolkit_views) {
deps += [ "//brave/components/sidebar" ]
}
So, we might want to check if assert(toolkit_views) is better, but I think we can deal with it in a separate issue.
There was a problem hiding this comment.
Filed an issue for it : brave/brave-browser#29918
| @@ -141,6 +142,11 @@ void BraveSettingsUI::AddResources(content::WebUIDataSource* html_source, | |||
|
|
|||
| html_source->AddBoolean("extensionsManifestV2Feature", | |||
| base::FeatureList::IsEnabled(kExtensionsManifestV2)); | |||
| #if defined(TOOLKIT_VIEWS) | |||
There was a problem hiding this comment.
Just double checking we need this check - I think most webui pages aren't included on android (but some are 😆 )
There was a problem hiding this comment.
I think we should keep this as long as we include the tabs section from the toolbar.html file only when it's toolkit_views. But Android doesn't seem to have brave://settings page, so we might be able to remove the guard in the toolbar.html together?
@simonhong, I'm wondering your opinion.
There was a problem hiding this comment.
Same here. When we're confusing something like this, I think we could sync with gn's configuration. :)
|
❤️ |
|
Verification PASSED on Using the STR/Cases outlined via brave/brave-browser#29843 & #18207 (comment), went through the following:
|
|
My notes about the most relevant prosumer settings: Tab appearanceSelect a Tab Layout: [Radiobutton] Standard Tabstrip [Radiobutton] Multirow Tabstrip [Auto],[1-5] [Radiobutton] Vertical Tree Tabstrip
The 3 typical tab layouts that cover all needs. Tab behaviourConfirm before closing multiple tabs. Prevent media files from starting until the tab get focus. When you open a link in a new tab, switch to it immediately. Move pinned tabs to the bottom of sidebar. Tab groupsUse isolated file and cookie container for every [Tab],[Tab Group]. Hide Tabs outside selected Tab group. Keep assigned group of pinned Tabs. Think about the opportunity to create standard groups with icons (which exist permanently) like Firefox and the opportunity to route every group through a different connection. Tab colorization and size
Tab size ratio: [100%] The ability to automatically change the color of a tab provides a better overview and helps to find the relevant tab faster. It also provides a sense of time if you spend too much time researching a specific topic and realize that the first tabs are already in saving mode. The "Tab size Ratio" setting provides the user with a simple solution for scaling tab size to accommodate more tabs/icons without relying on external extensions or manually trying out individual acceptable pixel sizes. Memory ConfigurationHibernate tabs in background after [15] minutes. Unload tab in background after [45] minutes Merge open tabs with same link if they are discarded/hibernated Ignore pinned tabs and tabs in different Tab groups before merging Enable smart memory clean up when RAM usage reaches [1-64GB]Dealing with multiple tabs requires a significant amount of memory, and when a browser consumes more than average memory, it is perceived as a disadvantage, despite the fact that the increasing memory consumption is naturally linked to progress. Providing users with more options related to memory saving can give them the impression that efficiency is being prioritized, while new features that immediately catch the user's attention justify the use of more memory. Brave aims to establish itself alongside Chrome, but it is filled with features that not everyone may find useful. Therefore, it is important to assure users that efficiency is not being overlooked. Chrome was able to persuade its users of the increased memory demand by delivering noticeable improvements in the user interface experience beside performance gains. |
This doesn't satisfy our latest design. But it would require to fix others altogether. So this PR just makes the tab section go well with others, such as "Sidebar" section.
Resolves brave/brave-browser#29843
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 lint,npm run presubmitwiki,npm run gn_check,npm run tslintgit rebase master(if needed)Reviewer Checklist:
gnAfter-merge Checklist:
changes has landed on
Test Plan: