Refactor sidebar expanded on-hover functionality and fix sidebar expa… #2288
Conversation
…nd/collapse behavior - **Preferences**: - Added a new preference `zen.view.sidebar-expanded.on-hover.paused-for-expand` to manage the paused state of the sidebar hover functionality. - Updated `zen.view.sidebar-expanded.on-hover` preference handling to ensure proper behavior when toggling the sidebar. - **ZenUIManager.mjs**: - Updated `_updateOnHoverVerticalTabs` method to handle the expanded state and remove hover attributes when the sidebar is expanded. - Refactored `_updateEvent` method to check and handle the expanded state of the sidebar, and to open or close the sidebar accordingly. - Added `openSidebar` and `closeSidebar` methods to manage the sidebar's expanded state and related attributes. - Refactored `toggleExpand` method to handle the new `paused-for-expand` preference and ensure proper toggling of the sidebar's expanded state. - **zen-settings.js**: - Updated `setCompactModeStyle` method to handle the new `paused-for-expand` preference. - Ensured that the `paused-for-expand` preference is reset when the sidebar expand strategy is changed. - **zen-browser.js**: - Added the new preference `zen.view.sidebar-expanded.on-hover.paused-for-expand` with a default value of `false`. - **Submodule Updates**: - Updated `l10n` submodule to the latest commit `dd93803b84f398834049a3f650536fbdbd14cbca`. - Updated `zen-components` submodule to the latest commit `d853fa8de4167dfd4d1289f4f15792a698c738c3-dirty`. - **Miscellaneous**: - Fixed indentation issues in `ZenUIManager.mjs`. - Removed commented-out code in `ZenUIManager.mjs`. - Ensured proper formatting and alignment of code blocks. ISSUE: zen-browser#2156
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces modifications to the Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🧰 Additional context used🔇 Additional comments (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (2)
- src/browser/base/content/ZenUIManager.mjs (8 hunks)
- src/browser/components/preferences/zen-settings.js (2 hunks)
🧰 Additional context used
📓 Learnings (1)
src/browser/base/content/ZenUIManager.mjs (1)
Learnt from: Richard-Woessner PR: zen-browser/desktop#2263 File: src/browser/base/content/ZenUIManager.mjs:115-117 Timestamp: 2024-10-21T15:48:17.759Z Learning: In `src/browser/base/content/ZenUIManager.mjs`, prefer using `let` for variable declarations throughout the file to maintain uniformity, even if the variables are not reassigned.
🪛 Biome
src/browser/base/content/ZenUIManager.mjs
[error] 76-239: Use let or const instead of var.
A variable declared with var is accessible in the whole module. Thus, the variable can be accessed before its initialization and outside the block where it is declared.
See MDN web docs for more details.
Unsafe fix: Use 'const' instead.(lint/style/noVar)
🔇 Additional comments (4)
src/browser/base/content/ZenUIManager.mjs (4)
3-3: Addition of_hoverPausedForExpandproperty looks good.The new property
_hoverPausedForExpandis properly added togZenUIManagerto manage the hover state during sidebar expansion.
147-148: Event handler correctly referencestoggleTabsOnRight.The
oncommandevent handler ininitRightSideOrderContextMenuproperly calls thetoggleTabsOnRightmethod, ensuring the context menu functions as intended.
115-136: Sidebar attribute management in_updateOnHoverVerticalTabsappears correct.The method updates the sidebar's attributes based on the hover settings and expanded state, which should result in the expected UI behavior.
212-232: Toggle logic intoggleExpandmethod looks good.The logic correctly handles different scenarios for expanding and collapsing the sidebar, including pausing and resuming hover detection.
| @@ -8,11 +9,11 @@ var gZenUIManager = { | |||
|
|
|||
| openAndChangeToTab(url, options) { | |||
| if (window.ownerGlobal.parent) { | |||
| let tab = window.ownerGlobal.parent.gBrowser.addTrustedTab(url, options); | |||
| const tab = window.ownerGlobal.parent.gBrowser.addTrustedTab(url, options); | |||
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider using let instead of const for variable declarations to maintain consistency.
Based on the team's coding style preference (as noted in prior learnings), we use let for variable declarations throughout the file for uniformity, even if the variables are not reassigned. This applies to the variable declarations at lines 12, 16, 89, 115-117, 188-191, and 212-214.
Apply these diffs to adjust the variable declarations:
-const tab = window.ownerGlobal.parent.gBrowser.addTrustedTab(url, options);
+let tab = window.ownerGlobal.parent.gBrowser.addTrustedTab(url, options);
-const tab = window.gBrowser.addTrustedTab(url, options);
+let tab = window.gBrowser.addTrustedTab(url, options);
-const tabs = document.getElementById('tabbrowser-tabs');
+let tabs = document.getElementById('tabbrowser-tabs');
-const onHover = Services.prefs.getBoolPref('zen.view.sidebar-expanded.on-hover');
-const expanded = Services.prefs.getBoolPref('zen.view.sidebar-expanded');
-const sidebar = this.navigatorToolbox;
+let onHover = Services.prefs.getBoolPref('zen.view.sidebar-expanded.on-hover');
+let expanded = Services.prefs.getBoolPref('zen.view.sidebar-expanded');
+let sidebar = this.navigatorToolbox;
-const isCompactMode = Services.prefs.getBoolPref('zen.view.compact');
-const expanded = this.expanded;
-const maxWidth = Services.prefs.getIntPref('zen.view.sidebar-expanded.max-width');
-const toolbox = document.getElementById('navigator-toolbox');
+let isCompactMode = Services.prefs.getBoolPref('zen.view.compact');
+let expanded = this.expanded;
+let maxWidth = Services.prefs.getIntPref('zen.view.sidebar-expanded.max-width');
+let toolbox = document.getElementById('navigator-toolbox');
-const pausedForExpand = this._hoverPausedForExpand;
-const onHover = Services.prefs.getBoolPref('zen.view.sidebar-expanded.on-hover');
-const expanded = Services.prefs.getBoolPref('zen.view.sidebar-expanded');
+let pausedForExpand = this._hoverPausedForExpand;
+let onHover = Services.prefs.getBoolPref('zen.view.sidebar-expanded.on-hover');
+let expanded = Services.prefs.getBoolPref('zen.view.sidebar-expanded');Also applies to: 16-16, 89-89, 115-117, 188-191, 212-214
Screen.Recording.2024-10-21.at.11.10.32.AM.mov |
|
Any chance of an animation when it is sliding back in? I remember a PR for that a while back but seems like it got closed.. |
|
Ya I messed up this pr so it closed so it hasn't been merged yet |
|
Thanks a lot! |
…nd/collapse behavior
Preferences:
zen.view.sidebar-expanded.on-hover.paused-for-expandto manage the paused state of the sidebar hover functionality.zen.view.sidebar-expanded.on-hoverpreference handling to ensure proper behavior when toggling the sidebar.ZenUIManager.mjs:
_updateOnHoverVerticalTabsmethod to handle the expanded state and remove hover attributes when the sidebar is expanded._updateEventmethod to check and handle the expanded state of the sidebar, and to open or close the sidebar accordingly.openSidebarandcloseSidebarmethods to manage the sidebar's expanded state and related attributes.toggleExpandmethod to handle the newpaused-for-expandpreference and ensure proper toggling of the sidebar's expanded state.zen-settings.js:
setCompactModeStylemethod to handle the newpaused-for-expandpreference.paused-for-expandpreference is reset when the sidebar expand strategy is changed.zen-browser.js:
zen.view.sidebar-expanded.on-hover.paused-for-expandwith a default value offalse.Submodule Updates:
l10nsubmodule to the latest commitdd93803b84f398834049a3f650536fbdbd14cbca.zen-componentssubmodule to the latest commitd853fa8de4167dfd4d1289f4f15792a698c738c3-dirty.Miscellaneous:
ZenUIManager.mjs.ZenUIManager.mjs.ISSUE: #2156
Summary by CodeRabbit
New Features
Bug Fixes
Refactor