Feature/auto hide tab bar #1527
Conversation
|
|
|
I also see that there's a discrepancy with the CLA, the email you use for your commits is not attached to your GitHub account. Can you please remedy this? Like many projects, we require all contributors to sign our Contributor License Agreement to protect us from future legal issues. It's fairly standard, however I'd recommend reading it before signing. Let me know if you have any questions. |
rritik772
force-pushed
the
feature/auto-hide-tab-bar
branch
from
afed75c to
01da190
Compare
|
There is some CLA issue from my side as I cannot update author directly without interfering the the old email id. I will do the changes as requested, and open up a new pull request. Tagging this conversation. Thanks Edit: |
rritik772
force-pushed
the
feature/auto-hide-tab-bar
branch
from
01da190 to
3fd9d97
Compare
|
I reran the check, looks like your force push fixed it :) |
window:autohidetabsbar --> window:autohidetabbar added invisible tab bar above and made it visible on hover using mouseenter and invisible mouseleave docs updated css updated
rritik772
force-pushed
the
feature/auto-hide-tab-bar
branch
from
9748972 to
98107e5
Compare
|
hmm I think there's a bit more work to do here for macOS compatibility. We should probably hide the window controls buttons when the tabbar is hidden: I also think the bottom of the tabbar needs a border |
|
Also I think you need the onmouseenter div to be separate from the tabbarwrapper div, since the onmouseenter only gets triggered over the content of the div Screen.Recording.2024-12-16.at.3.08.09.PM.mov |
For both problems, I guess I will need a Mac machine because I cannot replicate the issue on either Ubuntu GNOME or i3wm. You can see in the video. As of now I don’t have a Mac machine; let me arrange one. output.mp4 |
WalkthroughThis pull request introduces a new configuration feature that allows the tab bar in the application to automatically hide based on user interaction. The key addition is the boolean configuration Changes
Sequence DiagramsequenceDiagram
participant User
participant TabBar
participant Settings
User->>Settings: Toggle window:autohidetabbar
Settings->>TabBar: Update auto-hide setting
TabBar->>TabBar: Apply visibility logic
User->>TabBar: Move mouse near top
TabBar->>TabBar: Show/hide based on setting
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
🪧 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
🧹 Nitpick comments (5)
frontend/app/tab/tabbar.scss (1)
4-6: Consider using CSS custom property for better maintainabilityThe
$darwin-not-fullscreen-indentSCSS variable could be converted to a CSS custom property for better runtime configurability and maintainability.-$darwin-not-fullscreen-indent: 74px; +:root { + --darwin-not-fullscreen-indent: 74px; +}frontend/app/block/block.scss (1)
7-11: Consider consolidating Darwin-specific stylesWhile the implementation is correct, consider creating a separate partial for Darwin-specific styles to improve maintainability.
// Create _darwin.scss +@use "../tab/tabbar.scss" as tabbar; + +.darwin:not(.fullscreen) { + .block.block-frame-default .block-frame-default-header { + .window-drag.left { + width: tabbar.$darwin-not-fullscreen-indent; + } + } +}docs/docs/config.mdx (1)
71-71: Update documentation to reflect that no restart is required.Since this feature is implemented entirely on the frontend, it doesn't require an application restart to take effect. Consider updating the documentation to make this clear to users.
Apply this diff to update the documentation:
-| window:autohidetabbar | bool | show and hide the tab bar automatically when the mouse moves near the top of the window +| window:autohidetabbar | bool | show and hide the tab bar automatically when the mouse moves near the top of the window (changes take effect immediately)frontend/app/tab/tabbar.tsx (1)
692-692: Consider adding a separate hover area for better UX.The current implementation requires precise mouse positioning to show the tab bar. A separate hover area would make it easier to trigger.
Apply this diff to add a dedicated hover area:
return ( + <> + <div + className="tab-bar-hover-area" + style={{ + position: 'fixed', + top: 0, + left: 0, + right: 0, + height: '10px' + }} + onMouseEnter={handleAutoHideTabBar} + /> <div ref={tabbarWrapperRef} className={clsx('tab-bar-wrapper', { 'tab-bar-wrapper-auto-hide': autoHideTabBar, 'tab-bar-wrapper-always-visible': !autoHideTabBar })} >frontend/app/block/blockframe.tsx (1)
188-188: Consider removing unused ref.The
draggerLeftRefis created but never used in the component. If it's not needed for future functionality, consider removing it to keep the code clean.- const draggerLeftRef = React.useRef<HTMLDivElement>(null); const connName = blockData?.meta?.connection; const connStatus = util.useAtomValueSafe(getConnStatusAtom(connName)); const wshProblem = connName && !connStatus?.wshenabled && connStatus?.status == "connected"; // ... - <WindowDrag ref={draggerLeftRef} className="left" /> + <WindowDrag className="left" />Also applies to: 257-257
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
docs/docs/config.mdx(1 hunks)frontend/app/block/block.scss(1 hunks)frontend/app/block/blockframe.tsx(3 hunks)frontend/app/tab/tabbar.scss(2 hunks)frontend/app/tab/tabbar.tsx(3 hunks)frontend/types/gotypes.d.ts(1 hunks)pkg/wconfig/metaconsts.go(1 hunks)pkg/wconfig/settingsconfig.go(1 hunks)
🔇 Additional comments (8)
frontend/app/tab/tabbar.scss (2)
28-32: LGTM: Clear class naming and opacity handling
The renamed class with explicit opacity setting clearly indicates its purpose.
52-54: LGTM: Clean visibility toggle implementation
The visibility class implementation with opacity is clean and follows best practices.
pkg/wconfig/metaconsts.go (1)
64-64: LGTM: Consistent constant naming
The new configuration constant follows the established naming pattern and is placed in a logical position within the window-related configs.
frontend/app/block/block.scss (1)
5-5: LGTM: Proper SCSS module import
The import follows SCSS best practices using the @use directive with an alias.
pkg/wconfig/settingsconfig.go (1)
91-91: LGTM!
The new field follows the established naming conventions and JSON tag patterns for window settings.
frontend/app/tab/tabbar.tsx (1)
692-692: LGTM! Good use of clsx for class management.
The implementation correctly uses clsx to manage conditional classes, making the code more maintainable.
frontend/types/gotypes.d.ts (1)
652-652: LGTM! The type definition follows established patterns.
The new optional boolean property "window:autohidetabbar" is well-defined and follows the existing naming conventions for window-related settings.
frontend/app/block/blockframe.tsx (1)
43-43: LGTM! Clean import statement.
The import of WindowDrag follows the project's import conventions.
frontend/app/tab/tabbar.scss
Outdated
| .tab-bar-wrapper-auto-hide { | ||
| left: 0; | ||
| right: 0; | ||
|
|
||
| padding: 6px; | ||
| margin: 2px; | ||
|
|
||
| position: fixed; | ||
| z-index: 1000; | ||
|
|
||
| opacity: 0; | ||
| background: transparent; | ||
| backdrop-filter: blur(10px); | ||
| border-radius: 6px; | ||
|
|
||
| transition: opacity 0.3s ease, top 0.3s ease; | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Add browser compatibility fallbacks for backdrop-filter
Based on the codebase analysis, backdrop-filter is used in multiple components without proper fallbacks. To ensure consistent appearance across browsers, add the following:
- Add vendor prefixes and fallback styles in
frontend/app/tab/tabbar.scss:
.tab-bar-wrapper-auto-hide {
// ... existing styles ...
-webkit-backdrop-filter: blur(10px);
backdrop-filter: blur(10px);
// Fallback for browsers without backdrop-filter support
@supports not ((backdrop-filter: blur(10px)) or (-webkit-backdrop-filter: blur(10px))) {
background: rgb(from var(--block-bg-color) r g b / 0.8);
}
}- Similar fallbacks should be added to other components using backdrop-filter:
frontend/app/view/preview/directorypreview.scssfrontend/app/element/markdown.scssfrontend/app/block/block.scssfrontend/layout/lib/tilelayout.scss
🔗 Analysis chain
Verify backdrop-filter browser compatibility
The implementation looks good, but there are a few considerations:
- The backdrop-filter property might need a fallback for browsers that don't support it
- The fixed positioning with z-index might need testing with different window configurations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if there are any existing backdrop-filter fallbacks in the codebase
rg -l "backdrop-filter" | xargs rg "@supports not \(backdrop-filter:"
Length of output: 69
Script:
#!/bin/bash
# Let's first check all files using backdrop-filter and their context
rg -l "backdrop-filter"
# Then check for any browser compatibility related comments or prefixes
rg -A 3 "backdrop-filter|@supports|webkit" "*.scss" "*.css"
Length of output: 377
Script:
#!/bin/bash
# Check the content of files using backdrop-filter to see if there are any fallbacks
cat frontend/app/view/preview/directorypreview.scss frontend/app/tab/tabbar.scss frontend/app/element/markdown.scss frontend/app/block/block.scss frontend/layout/lib/tilelayout.scss
Length of output: 33002
There was a problem hiding this comment.
the -webkit-backdrop-filter suggestion seems worth it, the other one doesn't
There was a problem hiding this comment.
@esimkowitz Understood! Here’s an example of how you could add only the -webkit prefix without the fallback:
.tab-bar-wrapper-auto-hide {
left: 0;
right: 0;
padding: 6px;
margin: 2px;
position: fixed;
z-index: 1000;
opacity: 0;
// Add the -webkit prefix for Safari
-webkit-backdrop-filter: blur(10px);
backdrop-filter: blur(10px);
border-radius: 6px;
transition: opacity 0.3s ease, top 0.3s ease;
}Let me know if you need anything else!
| const handleAutoHideTabBar = (event: MouseEvent) => { | ||
| const tabBar = tabbarWrapperRef.current; | ||
| const tabBarHeight = tabBar?.clientHeight + 1; | ||
|
|
||
| if (event.type === 'mouseenter') { | ||
| tabBar.style.top = '0px'; | ||
| tabBar.addEventListener("mouseleave", handleAutoHideTabBar); | ||
| tabBar.classList.add('tab-bar-wrapper-auto-hide-visible') | ||
| } | ||
|
|
||
| if (event.type === 'mouseleave') { | ||
| tabBar.style.top = `-${tabBarHeight - 10}px`; | ||
| tabBar.removeEventListener("mouseleave", handleAutoHideTabBar); | ||
| tabBar.classList.remove('tab-bar-wrapper-auto-hide-visible') | ||
| } | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor to use CSS transforms instead of direct style manipulation.
The current implementation uses direct style manipulation which can cause unnecessary reflows. Consider using CSS transforms for better performance.
Apply this diff to use CSS transforms:
const handleAutoHideTabBar = (event: MouseEvent) => {
const tabBar = tabbarWrapperRef.current;
- const tabBarHeight = tabBar?.clientHeight + 1;
if (event.type === 'mouseenter') {
- tabBar.style.top = '0px';
- tabBar.addEventListener("mouseleave", handleAutoHideTabBar);
tabBar.classList.add('tab-bar-wrapper-auto-hide-visible')
}
if (event.type === 'mouseleave') {
- tabBar.style.top = `-${tabBarHeight - 10}px`;
- tabBar.removeEventListener("mouseleave", handleAutoHideTabBar);
tabBar.classList.remove('tab-bar-wrapper-auto-hide-visible')
}
};Add these CSS classes to your stylesheet:
.tab-bar-wrapper-auto-hide {
transform: translateY(-100%);
transition: transform 0.2s ease;
}
.tab-bar-wrapper-auto-hide-visible {
transform: translateY(0);
}There was a problem hiding this comment.
this is a good suggestion @rritik772
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| useEffect(() => { | ||
| const tabBar = tabbarWrapperRef.current; | ||
| if (!autoHideTabBar) { | ||
| tabBar.style.top = '0px'; | ||
| return; | ||
| } | ||
|
|
||
| const tabBarHeight = tabBar?.clientHeight + 1; | ||
| if (autoHideTabBar) { | ||
| tabbarWrapperRef.current.style.top = `-${tabBarHeight - 10}px` | ||
| } | ||
|
|
||
| tabbarWrapperRef.current.addEventListener("mouseenter", handleAutoHideTabBar); | ||
| return () => tabbarWrapperRef.current.removeEventListener("mouseenter", handleAutoHideTabBar); | ||
| }, [autoHideTabBar]) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve event listener management.
The current implementation could lead to memory leaks if the component unmounts while listeners are still attached. Also, the cleanup function might fail if tabbarWrapperRef.current is null.
Apply this diff to improve event listener management:
useEffect(() => {
const tabBar = tabbarWrapperRef.current;
if (!autoHideTabBar || !tabBar) {
- tabBar.style.top = '0px';
return;
}
- const tabBarHeight = tabBar?.clientHeight + 1;
- if (autoHideTabBar) {
- tabbarWrapperRef.current.style.top = `-${tabBarHeight - 10}px`
- }
- tabbarWrapperRef.current.addEventListener("mouseenter", handleAutoHideTabBar);
+ const handleMouseEnter = (e: MouseEvent) => {
+ tabBar.classList.add('tab-bar-wrapper-auto-hide-visible');
+ tabBar.addEventListener('mouseleave', handleMouseLeave);
+ };
+ const handleMouseLeave = (e: MouseEvent) => {
+ tabBar.classList.remove('tab-bar-wrapper-auto-hide-visible');
+ tabBar.removeEventListener('mouseleave', handleMouseLeave);
+ };
+ tabBar.addEventListener("mouseenter", handleMouseEnter);
- return () => tabbarWrapperRef.current.removeEventListener("mouseenter", handleAutoHideTabBar);
+ return () => {
+ tabBar.removeEventListener("mouseenter", handleMouseEnter);
+ tabBar.removeEventListener("mouseleave", handleMouseLeave);
+ };
}, [autoHideTabBar]);Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
this is also a good suggestion, just in case the mouseleave handler is still attached
There was a problem hiding this comment.
@esimkowitz, glad to hear you find the suggestion helpful! By attaching and detaching the listeners in a more localized manner, we reduce the risk of orphaned event handlers if the component unmounts prematurely. Let me know if you need further refinements to the approach.
feature/auto-hide-tab-bar fixed issue of window title bar feature/auto-hide-tab-bar fixed issue in non hide mode
rritik772
force-pushed
the
feature/auto-hide-tab-bar
branch
from
c2653cc to
bce3106
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/app/tab/tabbar.scss (1)
6-14: Consider documenting the Darwin-specific indent valueThe
$darwin-not-fullscreen-indentvariable's value (74px) appears to be specifically chosen for macOS window controls. Consider adding a comment explaining this magic number's significance for future maintenance.+// 74px accounts for the macOS window controls and spacing in non-fullscreen mode $darwin-not-fullscreen-indent: 74px;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
frontend/app/block/block.scss(1 hunks)frontend/app/block/blockframe.tsx(3 hunks)frontend/app/tab/tabbar.scss(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/app/block/block.scss
- frontend/app/block/blockframe.tsx
🔇 Additional comments (2)
frontend/app/tab/tabbar.scss (2)
34-50: Add browser compatibility fallbacks for backdrop-filter
The backdrop-filter property needs vendor prefixes and fallbacks for cross-browser compatibility.
Document the z-index choice
The z-index value of 1000 seems arbitrary. Consider using a SCSS variable with documentation explaining its relationship to other layered elements.
+// Z-index needs to be higher than regular content but lower than modals (2000)
+$tab-bar-z-index: 1000;
.tab-bar-wrapper-auto-hide {
// ...
- z-index: 1000;
+ z-index: $tab-bar-z-index;
// ...
}49-54:
Verify the top property transition
The transition includes top 0.3s ease, but the top property is not set in either class. This might cause the transition to have no effect.
Consider adding the top property:
.tab-bar-wrapper-auto-hide {
left: 0;
right: 0;
+ top: 0;
// ...
}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
frontend/app/tab/tabbar.scss (1)
49-49: Consider adjusting transition timing for accessibilityThe current transition duration of 0.3s might be too long for users who prefer reduced motion. Consider:
- Adding a media query for reduced motion
- Adjusting the timing based on user preferences
.tab-bar-wrapper-auto-hide { // ... other styles ... transition: opacity 0.3s ease, top 0.3s ease; + @media (prefers-reduced-motion: reduce) { + transition: none; + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
docs/docs/config.mdx(1 hunks)frontend/app/block/block.scss(1 hunks)frontend/app/block/blockframe.tsx(3 hunks)frontend/app/tab/tabbar.scss(2 hunks)frontend/types/gotypes.d.ts(1 hunks)pkg/wconfig/metaconsts.go(1 hunks)pkg/wconfig/settingsconfig.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- pkg/wconfig/metaconsts.go
- frontend/app/block/block.scss
- docs/docs/config.mdx
- frontend/types/gotypes.d.ts
- pkg/wconfig/settingsconfig.go
- frontend/app/block/blockframe.tsx
🔇 Additional comments (2)
frontend/app/tab/tabbar.scss (2)
4-17: LGTM! Good improvement in variable management
The change from CSS variable to SCSS variable improves maintainability, and the comment clearly explains the 74px value for macOS window controls.
51-56: Add browser compatibility fallbacks for backdrop-filter
The backdrop-filter implementation needs vendor prefixes for broader browser support.
| position: fixed; | ||
| z-index: 1000; |
There was a problem hiding this comment.
💡 Codebase verification
Z-index value needs adjustment to follow established system
The z-index value of 1000 in tabbar.scss does not align with the established z-index system in theme.scss. The highest z-index value defined in the theme is 550 (for flash-error-container), and most UI components use values between 1-100. The tabbar should use a themed z-index variable that fits within this hierarchy.
- Change
z-index: 1000to use a themed variable like--zindex-tabbarthat should be defined in theme.scss with an appropriate value (likely between 100-500 based on other component values)
🔗 Analysis chain
Verify z-index value in the component hierarchy
The z-index value of 1000 seems arbitrary. Please verify that this value:
- Doesn't conflict with other fixed-position elements
- Is high enough to appear above all necessary elements
- Follows any existing z-index system in the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other z-index values in the codebase for comparison
rg "z-index:" "frontend/**/*.scss" "frontend/**/*.css"
Length of output: 171
Script:
#!/bin/bash
# Search for z-index values in the codebase without path restrictions
rg "z-index:"
Length of output: 119041
Script:
#!/bin/bash
# Find all CSS and SCSS files in the repository
fd -e scss -e css
Length of output: 2818
Script:
#!/bin/bash
# Search for any style files containing 'tabbar' in their name
fd "tabbar" -e scss -e css
Length of output: 55
Script:
#!/bin/bash
# Search for z-index declarations in theme.scss to check for any z-index system
rg "z-index|--zindex" frontend/app/theme.scss
# Search for fixed position elements with z-index
rg "position:\s*fixed.*z-index|z-index.*position:\s*fixed" -A 2 -B 2
Length of output: 1103
There was a problem hiding this comment.
@esimkowitz Thank you for confirming. Let me know if there's anything else you need!
This PR adds a new feature of auto hiding tab bar. Requires restart for change
Issue #1525
"window:autohidetabsbar": false
output.mp4
"window:autohidetabsbar": true
output1.mp4