-
Notifications
You must be signed in to change notification settings - Fork 53
Make app responsive and toggle sidebar #533
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
@wojtekn , are you ok merging this? or would you prefer to add it behind a feature flag? In my opinion the responsive is an independent feature from the Assistant, the same way the float on top is already released. |
|
Nice work @sejas! I was only able to test it on Mac, but spotted a few minor things. Is it possible to centre the icons in the top bar with the window controls?
Can you increase the height of the user element in the top bar to 24px so it matches the height of the help icon when hovered.
The user avatar and help icon should be closer together. The avatar is also missing the white outline.
When the offline icon is displayed, the header bar gets taller. Screen.Recording.2024-09-13.at.14.35.38.movIs it possible to automatically collapse the sidebar if the user tries to resize the window below the min-width? (Totally fine to tackle this in a separate PR if needed.) Screen.Recording.2024-09-13.at.14.46.20.mov |
|
I've tested it on macOS. Looks great! I noticed some issues:
I will test it on Windows soon.
It makes sense. Let's release it for all users. |
Yep. Reducing the height to |
|
It works well on Windows, with the following issues:
User logged in: |
|
@wojtekn , @matt-west , thanks for reviewing the PR. I've fixed everything you mentioned.
I've implemented the memory size for both states, with and without sidebar, but this behaviour can conflict if/when we implement the window manual resizing:
We could need a bit of discussion about making the window responsive by resizing it. It's possible, and it makes sense to me. But it seems incompatible combined with saving the previous width. I'll merge this PR and if we find new bugs or enhancements, we can open new PRs/issues. Thank you! |
|
@wojtekn , @matt-west , in fact Wojtek's suggestion was simpler and it's totally compatible to automatically displaying/hiding the sidebar on window resize. Check the PR below.
I misunderstood this explanation thinking to keep the size memory, but you actually were suggesting a simpler workflow. 👌 I've created a PR applying your approach and Matt's suggestion.
Done! 🥳 Let me know what you think: |







Proposed Changes
Testing Instructions
npm startMac
responsive-studio.mp4
Windows
q4uVLd.mp4
Offline indicator
Pre-merge Checklist