-
-
Notifications
You must be signed in to change notification settings - Fork 193
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
keeping sidebars and header visible on mobile view #949
base: main
Are you sure you want to change the base?
Conversation
Review or Edit in CodeSandboxOpen the branch in Web Editor • VS Code • Insiders |
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 125a86d:
|
@@ -32,6 +32,9 @@ export class LeftPanel< | |||
this.options.panelOpen | |||
); | |||
|
|||
const viewportWidth: number = document.documentElement.clientWidth; | |||
if(viewportWidth <= 767) return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the functionality that this provides, but I wonder if we can implement it in a more flexible way that avoids hard-coded magic numbers. Might it make sense to replace the panelOpen boolean configuration setting with a panelOpenMinWidth integer setting? Then it could be 0 to always open, some incredibly high number to always close, or a configurable breakpoint the rest of the time. What do you think? @edsilv?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@demiankatz One possibility that avoids any config stuff would be to assume that this behaviour is only needed when it's a portrait-oriented, touch-enabled device, which I think in 99% of cases would be a phone/tablet?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's true, but I think @edsilv's point was that displaying the sidebar on mobile isn't the ideal solution to the problem, regardless of what approach we take for deciding to do it. Though if it's an urgent local need, I think the approach here might be able to be locally adopted as a workaround until the more complete solution is ready.
This was discussed on the December 5, 2024 Community Call; the general consensus is that we should try to implement the left sidebar navigation as a pop-up modal in mobile for a more consistent user experience, rather than making the existing panels visible in mobile. We should devote some time to this early next year and create a new PR to supersede this one. |
Description of what you did:
Modified CSS rules to keep sidebar and header visible when on mobile view. Relating to issue #938