-
-
Notifications
You must be signed in to change notification settings - Fork 45
[FEAT] : Added Adjustable Vertical Seperator between Editor and ContentViewer #173
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
base: main
Are you sure you want to change the base?
[FEAT] : Added Adjustable Vertical Seperator between Editor and ContentViewer #173
Conversation
@JeelRajodiya Waiting for your feedbacks on this...🙂 |
@Yashwanth1906 I'm currently busy with my exams, I'll review it sometime after 3rd may |
@JeelRajodiya Okay.. |
pathList.push({ | ||
markdownPath: [item.folderName, step.fileName], | ||
markdownPath: pathSegments, | ||
}); | ||
}); | ||
}); |
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.
are these changes relevant to the issue? Please remove these changes,
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.
@Yashwanth1906 Please reply here
app/components/ResizableContent.tsx
Outdated
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 see, we are wrapping the EditorNOutput into this new component. however we are importing the styles from the different component file (i.e. page.module.css) .
move current file into a new folder of the same name, and add the css file in that folder to maintain consistent component declaration
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.
consider moving these changes to the page.tsx file (in the /content/[...markdownPath] folder) if you prefer to not to create a new component
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.
-
The divider and scroll bar have the same color and are difficult to differentiate. Moreover, when you click on the divider sometimes some text from the content is getting selected unintentionally. consider changing the styles of both dividers to make them differentiable (you can take reference from leetcode UI to implement into ours)
-
The whole page does not have the height of 100% and there are some space left at the very bottom
@Yashwanth1906 The vertical Separator is working as expected. could you please address my comments? |
I'll do it. |
@Yashwanth1906, any updates on the PR? |
@JeelRajodiya Sry i had my sem. I'll do it by this weekend |
Co-authored-by: Copilot <[email protected]>
…1906/Json-Schema-Tour into Dynamic-Vertical-Split
@JeelRajodiya I think I have completed all the changes you asked for. Can u review it? |
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.
Pull Request Overview
This PR introduces a resizable vertical separator between the content viewer and editor panels, allowing users to adjust panel widths according to their preferences. The implementation replaces the static layout with a dynamic, interactive interface that includes visual feedback and persistent width preferences.
- Refactored the layout from static flex panels to a new ResizableContent component with draggable divider
- Added visual feedback with hover/active states using primary theme colors and smooth transitions
- Implemented localStorage persistence to remember user's preferred panel width across sessions
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.
Show a summary per file
File | Description |
---|---|
app/content/[...markdownPath]/page.tsx | Refactored to use ResizableContent component and fixed path generation logic |
app/content/[...markdownPath]/page.module.css | Updated styles to support resizable layout with new pane and divider styles |
app/components/ResizableContent/ResizableContent.tsx | New component implementing draggable divider functionality with mouse event handling |
app/components/ResizableContent/ResizableContent.module.css | Complete styling for resizable layout including divider animations and scrollbar customization |
app/components/EditorNOutput/EditorNOutput.module.css | Removed max-width constraint and updated to fill available space |
app/components/ContentViewer/ContentViewer.module.css | Simplified padding and updated layout to work within resizable container |
import React from "react"; | ||
import { parseLessonFolder } from "@/lib/server-functions"; | ||
import ContentViewer from "@/app/components/ContentViewer"; | ||
import EditorNOutput from "@/app/components/EditorNOutput"; | ||
import ResizableContent from "@/app/components/ResizableContent/ResizableContent"; |
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.
The title and description contain a spelling error: 'Seperator' should be 'Separator'. This same error appears in the import path - consider renaming the component to 'ResizableSeparator' or similar to match the corrected spelling.
import ResizableContent from "@/app/components/ResizableContent/ResizableContent"; | |
import ResizableSeparator from "@/app/components/ResizableSeparator/ResizableSeparator"; |
Copilot uses AI. Check for mistakes.
|
||
const containerRect = containerRef.current!.getBoundingClientRect(); |
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.
Using the non-null assertion operator (!) without proper null checking is risky. The containerRef.current could be null if the component unmounts during dragging. Add a null check: if (!containerRef.current) return;
const containerRect = containerRef.current!.getBoundingClientRect(); | |
if (!containerRef.current) return; | |
const containerRect = containerRef.current.getBoundingClientRect(); |
Copilot uses AI. Check for mistakes.
onMouseMove={handleMouseMove} | ||
onMouseUp={handleMouseUp} |
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.
The onMouseMove handler will fire continuously even when not dragging, causing unnecessary function calls. Consider adding global mouse event listeners only during drag operations to improve performance.
onMouseMove={handleMouseMove} | |
onMouseUp={handleMouseUp} | |
Copilot uses AI. Check for mistakes.
|
||
if (newLeftWidth >= minWidth && newLeftWidth <= maxWidth) { | ||
setLeftWidth(newLeftWidth); | ||
localStorage.setItem("horizontalLeftWidth", String(newLeftWidth)); |
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.
Writing to localStorage on every mouse move during dragging can cause performance issues. Consider using a debounced approach or only saving the value when dragging ends.
localStorage.setItem("horizontalLeftWidth", String(newLeftWidth)); |
Copilot uses AI. Check for mistakes.
localStorage.setItem("horizontalLeftWidth", String(newLeftWidth)); | ||
} | ||
}; | ||
|
||
useEffect(() => { | ||
const savedWidth = localStorage.getItem("horizontalLeftWidth"); | ||
if (savedWidth) { | ||
setLeftWidth(Number(savedWidth)); |
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.
Accessing localStorage should be wrapped in a try-catch block as it can throw exceptions in private browsing mode or when storage is disabled.
localStorage.setItem("horizontalLeftWidth", String(newLeftWidth)); | |
} | |
}; | |
useEffect(() => { | |
const savedWidth = localStorage.getItem("horizontalLeftWidth"); | |
if (savedWidth) { | |
setLeftWidth(Number(savedWidth)); | |
try { | |
localStorage.setItem("horizontalLeftWidth", String(newLeftWidth)); | |
} catch (error) { | |
console.error("Failed to save width to localStorage:", error); | |
} | |
} | |
}; | |
useEffect(() => { | |
try { | |
const savedWidth = localStorage.getItem("horizontalLeftWidth"); | |
if (savedWidth) { | |
setLeftWidth(Number(savedWidth)); | |
} | |
} catch (error) { | |
console.error("Failed to retrieve width from localStorage:", error); |
Copilot uses AI. Check for mistakes.
What kind of change does this PR introduce?
Issue Number:
Screenshots/videos:
https://github.com/user-attachments/assets/9259dc52-cd5e-4a6e-8c6e-399232a21b4f
If relevant, did you update the documentation?
Summary
This PR introduces a resizable vertical separator between the content viewer and editor panels, improving the user experience by allowing users to adjust the width of each panel according to their preferences. The separator features:
The implementation uses CSS flexbox for layout management and includes proper cursor styling to indicate the resizable nature of the divider.
Does this PR introduce a breaking change?
No, this is a non-breaking change that enhances the existing UI without affecting any functionality or existing features.
Technical Details:
page.module.css