Fix: Topbar icons display padding in macOS fullscreen - #833
Conversation
There was a problem hiding this comment.
Nice work!
I like the effect it creates when the window becomes full-size 👍
One thing that I find a bit strange is when you go from the full size window to the smaller window and for a couple of seconds while the transition is happening, the buttons are overlapping the sidebar toggle. I could see this happening both with either buttons or key board shortcuts. I tried to capture it in the video below. Hopefully it illustrates well what I mean but let me know if I am not explaining this well.
Screen.Recording.2025-01-23.at.5.25.12.PM.mov
I am not sure if there is an easy solution for this though. Perhaps this is okay.
Everything worked well on my end for rtl and ltr languages.
| import { renderHook } from '@testing-library/react'; | ||
| import { act } from 'react'; | ||
| import { getIpcApi } from '../../lib/get-ipc-api'; | ||
| import { useFullscreen } from '../use-fullscreen'; |
There was a problem hiding this comment.
Nice, thanks for adding tests!
fredrikekelund
left a comment
fredrikekelund
left a comment
There was a problem hiding this comment.
LGTM 👍 I agree with @katinthehatsite that it would be nice if the transition ran earlier, but looking at when the enter-full-screen and leave-full-screen events are fired, that seems difficult to pull off. I also couldn't find any other viable methods for listening to earlier events about fullscreen state changes, so we may just have to accept that small nuisance.
| // Leave space for window controls depending on which side they are on | ||
| // Take into account the "chrome" padding, the position of which depends on the language direction | ||
| isWindowFrameRtl() && | ||
| 'transition-[padding] duration-700 ease-in-out', |
There was a problem hiding this comment.
| 'transition-[padding] duration-700 ease-in-out', | |
| 'transition-[padding] duration-500 ease-in-out', |
I would speed up this animation slightly. 500ms matches the sidebar transition
| await act( async () => { | ||
| await Promise.resolve(); | ||
| } ); |
There was a problem hiding this comment.
| await act( async () => { | |
| await Promise.resolve(); | |
| } ); | |
| await waitFor( () => { | |
| expect( mockIpcApi.isFullscreen ).toHaveBeenCalledTimes( 1 ); | |
| } ); |
Optional, but I found the current code a bit cryptic. I think this clarifies the intention.
There was a problem hiding this comment.
Much cleaner approach, thank you!
| await act( async () => { | ||
| await Promise.resolve(); | ||
| } ); |
There was a problem hiding this comment.
Same thing here about potentially changing.
|
Thanks, @katinthehatsite and @fredrikekelund for the review! I've addressed the other comments, if you both agree, I would propose we merge like this and accept this as a limitation for now. |
|
I say |
* add use-fullscreen hook and adjust topbar icons when fullscreen on mac * add unit tests * reduce delay and improve unit tests
3 participants
Related issues
Proposed Changes
Testing Instructions
Expected Behavior
Pre-merge Checklist