new-log-viewer: Resize panel width when the browser window resizes. #94
Conversation
…width ratio approach; Add window resize event handler to resize panel width when necessary.
…utedStyle instead of getPropertyValue from the element's `style` object directly.
WalkthroughThe pull request modifies the Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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: 2
🧹 Outside diff range and nitpick comments (1)
new-log-viewer/src/components/CentralContainer/Sidebar/index.tsx (1)
17-17: MakeEDITOR_MIN_WIDTH_IN_PIXELSconfigurableThe hardcoded value of
250pixels forEDITOR_MIN_WIDTH_IN_PIXELSmay not accommodate all screen sizes or user preferences. Consider making this value configurable or responsive to enhance flexibility and usability across different devices.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- new-log-viewer/src/components/CentralContainer/Sidebar/index.tsx (4 hunks)
🧰 Additional context used
📓 Learnings (1)
new-log-viewer/src/components/CentralContainer/Sidebar/index.tsx (2)
Learnt from: junhaoliao PR: y-scope/yscope-log-viewer#74 File: new-log-viewer/src/components/CentralContainer/Sidebar/ResizeHandle.tsx:77-82 Timestamp: 2024-09-28T02:32:08.882Z Learning: Accessibility improvements, such as adding ARIA attributes to components like `ResizeHandle`, require broader discussion and are considered out of scope for small PRs.Learnt from: junhaoliao PR: y-scope/yscope-log-viewer#74 File: new-log-viewer/src/components/CentralContainer/Sidebar/ResizeHandle.tsx:77-82 Timestamp: 2024-10-08T15:52:50.753Z Learning: Accessibility improvements, such as adding ARIA attributes to components like `ResizeHandle`, require broader discussion and are considered out of scope for small PRs.
🔇 Additional comments (1)
new-log-viewer/src/components/CentralContainer/Sidebar/index.tsx (1)
25-25: Verify the change fromdocument.bodytodocument.documentElementSwitching from
document.bodytodocument.documentElementwhen accessing and setting the--ylv-panel-widthCSS variable may have broader implications. Ensure that all styles and components relying on this variable continue to function correctly after this change.Run the following script to search for all occurrences of
--ylv-panel-widthin the codebase to confirm consistent usage:Also applies to: 35-35
✅ Verification successful
Verification Completed: Consistent Usage of
--ylv-panel-widthConfirmedAll instances of
--ylv-panel-widthhave been reviewed and are consistently used within CSS files. The change fromdocument.bodytodocument.documentElementdoes not impact other parts of the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all instances of '--ylv-panel-width' in CSS and JavaScript files. rg '--ylv-panel-width'Length of output: 442
Script:
#!/bin/bash # Description: Corrected script to find all instances of '--ylv-panel-width' in CSS and JavaScript files. rg -e '--ylv-panel-width' --type-add 'js:*.js' --type-add 'css:*.css' --js --cssLength of output: 509
Script:
#!/bin/bash # Description: Corrected script to find all instances of '--ylv-panel-width' in CSS and JavaScript files. rg -e '--ylv-panel-width' -tjs -tcssLength of output: 268
new-log-viewer/src/components/CentralContainer/Sidebar/index.tsx
Outdated
Show resolved
Hide resolved
| // On initialization, register window resize event handler to resize panel width when necessary. | ||
| useEffect(() => { | ||
| const handleWindowResize = () => { | ||
| const availableWidth = window.innerWidth - EDITOR_MIN_WIDTH_IN_PIXELS; | ||
| if ( | ||
| getPanelWidth() > availableWidth && | ||
| tabListRef.current && | ||
| tabListRef.current.clientWidth + PANEL_CLIP_THRESHOLD_IN_PIXELS < availableWidth | ||
| ) { | ||
| setPanelWidth(availableWidth); | ||
| } | ||
| }; | ||
|
|
||
| window.addEventListener("resize", handleWindowResize); | ||
|
|
||
| return () => { | ||
| window.removeEventListener("resize", handleWindowResize); | ||
| }; | ||
| }, []); | ||
|
|
There was a problem hiding this comment.
Confirm dependencies in useEffect are correctly specified
The useEffect hook currently has an empty dependency array. Ensure that all variables and functions used within the effect, such as getPanelWidth, tabListRef.current, and PANEL_CLIP_THRESHOLD_IN_PIXELS, do not need to be included in the dependency array to prevent potential bugs due to stale closures.
Consider adding necessary dependencies to the dependency array or refactoring the handleWindowResize function using useCallback if appropriate.
There was a problem hiding this comment.
getPanelWidth is declared outside of the functional component and does not have any dependency on React states;
tabListRef is a ref which I believe does not need to be included in the dependency array.
PANEL_CLIP_THRESHOLD_IN_PIXELS is a const declared outside of the functional component, which does not change at all as well.
Do you think the dependency array is correctly specified now?
There was a problem hiding this comment.
@junhaoliao, you're right. Since getPanelWidth and PANEL_CLIP_THRESHOLD_IN_PIXELS are constants declared outside the functional component, and tabListRef is a ref object that remains consistent throughout the component's lifecycle, the dependency array is correctly specified as is. Thank you for the clarification.
（・⊝・）
✏️ Learnings added
Learnt from: junhaoliao
PR: y-scope/yscope-log-viewer#94
File: new-log-viewer/src/components/CentralContainer/Sidebar/index.tsx:99-118
Timestamp: 2024-10-19T03:33:29.285Z
Learning: In `new-log-viewer/src/components/CentralContainer/Sidebar/index.tsx`, when using `useEffect` to register window resize event handlers, it's acceptable to have an empty dependency array because the variables and functions used within the effect (`getPanelWidth`, `PANEL_CLIP_THRESHOLD_IN_PIXELS`, and `tabListRef`) are constants or refs declared outside the functional component and do not change.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
|
If you click on the tab when the editor is small, the tab appears to "jump" to smaller length. I didn't investigate, but I feel it has something do to with this line Screen.Recording.2024-10-20.at.12.37.42.PM.mov |
…PIXELS to 140 to enhance layout consistency.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
new-log-viewer/src/components/CentralContainer/Sidebar/index.tsx (1)
99-118: LGTM: Effective window resize handling implemented.The new
useEffecthook successfully implements the PR objective of adjusting the panel width when the browser window resizes. The cleanup function properly removes the event listener, which is good practice to prevent memory leaks.To further improve performance, consider debouncing the
handleWindowResizefunction. This would reduce the frequency of recalculations during rapid window resizing, potentially improving the user experience on less powerful devices.Example implementation using a custom debounce hook:
import { useCallback, useEffect } from 'react'; function useDebounce(callback: () => void, delay: number) { useEffect(() => { const handler = setTimeout(() => callback(), delay); return () => clearTimeout(handler); }, [callback, delay]); } // In your component: const debouncedHandleWindowResize = useCallback(() => { // Your existing handleWindowResize logic here }, [/* dependencies */]); useDebounce(debouncedHandleWindowResize, 250); // 250ms delay useEffect(() => { window.addEventListener('resize', debouncedHandleWindowResize); return () => window.removeEventListener('resize', debouncedHandleWindowResize); }, [debouncedHandleWindowResize]);This approach would maintain responsiveness while reducing unnecessary calculations.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- new-log-viewer/src/components/CentralContainer/Sidebar/index.tsx (4 hunks)
🧰 Additional context used
📓 Learnings (1)
new-log-viewer/src/components/CentralContainer/Sidebar/index.tsx (3)
Learnt from: junhaoliao PR: y-scope/yscope-log-viewer#94 File: new-log-viewer/src/components/CentralContainer/Sidebar/index.tsx:99-118 Timestamp: 2024-10-19T03:33:29.578Z Learning: In `new-log-viewer/src/components/CentralContainer/Sidebar/index.tsx`, when using `useEffect` to register window resize event handlers, it's acceptable to have an empty dependency array because the variables and functions used within the effect (`getPanelWidth`, `PANEL_CLIP_THRESHOLD_IN_PIXELS`, and `tabListRef`) are constants or refs declared outside the functional component and do not change.Learnt from: junhaoliao PR: y-scope/yscope-log-viewer#74 File: new-log-viewer/src/components/CentralContainer/Sidebar/ResizeHandle.tsx:77-82 Timestamp: 2024-09-28T02:32:08.882Z Learning: Accessibility improvements, such as adding ARIA attributes to components like `ResizeHandle`, require broader discussion and are considered out of scope for small PRs.Learnt from: junhaoliao PR: y-scope/yscope-log-viewer#74 File: new-log-viewer/src/components/CentralContainer/Sidebar/ResizeHandle.tsx:77-82 Timestamp: 2024-10-08T15:52:50.753Z Learning: Accessibility improvements, such as adding ARIA attributes to components like `ResizeHandle`, require broader discussion and are considered out of scope for small PRs.
🔇 Additional comments (4)
new-log-viewer/src/components/CentralContainer/Sidebar/index.tsx (4)
3-3: LGTM: Import of useEffect is appropriate.The addition of
useEffectto the import statement is consistent with its usage in the new window resize event handling logic introduced later in the component.
16-17: LGTM: New constants introduced for panel and editor dimensions.The introduction of
PANEL_CLIP_THRESHOLD_IN_PIXELSandEDITOR_MIN_WIDTH_IN_PIXELSis appropriate for the new resizing logic. However, could you please provide some context on how these specific values (360 and 140) were determined? This would help in understanding the design decisions behind these constants.
25-25: LGTM: Improved DOM interaction for panel width.The change from
document.bodytodocument.documentElementin bothgetPanelWidthandsetPanelWidthfunctions is a good improvement. Usingdocument.documentElementis more standard and reliable across different browsers for accessing and setting CSS custom properties. This change ensures more consistent behaviour of the panel width calculations and adjustments.Also applies to: 35-35
86-95: LGTM: Improved resize handling with minimum editor width consideration.The new resize logic effectively implements a responsive panel width while ensuring the editor maintains a minimum width. This change aligns well with the PR objectives.
However, I notice that the
availableWidthcalculation is duplicated in bothhandleResizeand theuseEffecthook for window resize. To improve maintainability, consider refactoring this calculation into a separate function.
# Conflicts: # new-log-viewer/src/components/CentralContainer/Sidebar/index.tsx
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
new-log-viewer/src/components/CentralContainer/Sidebar/index.tsx (3)
22-23: Approve changes and suggest documentation for constantsThe updates to
PANEL_CLIP_THRESHOLD_IN_PIXELSand the introduction ofEDITOR_MIN_WIDTH_IN_PIXELSalign well with the PR objectives. These changes should improve the resizing behaviour of the sidebar and editor.Consider adding brief comments to explain the purpose and impact of these constants, especially
EDITOR_MIN_WIDTH_IN_PIXELS, as it's a new addition.
93-102: Approve resize logic and suggest refactoringThe updated
handleResizefunction now properly considers the minimum editor width, which is a significant improvement. This change ensures better usability by maintaining a minimum editor size during resizing.To reduce code duplication, consider extracting the
availableWidthcalculation into a separate function:const calculateAvailableWidth = () => window.innerWidth - EDITOR_MIN_WIDTH_IN_PIXELS; // Then use it in handleResize and handleWindowResize: const availableWidth = calculateAvailableWidth();This refactoring will improve maintainability and reduce the risk of inconsistencies.
106-125: Approve window resize handling and suggest optimizationThe addition of the
useEffecthook for handling window resize events is a great improvement. It ensures that the sidebar remains responsive to window size changes while respecting the minimum editor width.Consider using the
useCallbackhook for thehandleWindowResizefunction to optimize performance:const handleWindowResize = useCallback(() => { // ... existing logic ... }, []); useEffect(() => { window.addEventListener("resize", handleWindowResize); return () => window.removeEventListener("resize", handleWindowResize); }, [handleWindowResize]);This change can help prevent unnecessary re-renders and improve overall performance.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- new-log-viewer/src/components/CentralContainer/Sidebar/index.tsx (3 hunks)
🧰 Additional context used
📓 Learnings (1)
new-log-viewer/src/components/CentralContainer/Sidebar/index.tsx (3)
Learnt from: junhaoliao PR: y-scope/yscope-log-viewer#94 File: new-log-viewer/src/components/CentralContainer/Sidebar/index.tsx:99-118 Timestamp: 2024-10-19T03:33:29.578Z Learning: In `new-log-viewer/src/components/CentralContainer/Sidebar/index.tsx`, when using `useEffect` to register window resize event handlers, it's acceptable to have an empty dependency array because the variables and functions used within the effect (`getPanelWidth`, `PANEL_CLIP_THRESHOLD_IN_PIXELS`, and `tabListRef`) are constants or refs declared outside the functional component and do not change.Learnt from: junhaoliao PR: y-scope/yscope-log-viewer#74 File: new-log-viewer/src/components/CentralContainer/Sidebar/ResizeHandle.tsx:77-82 Timestamp: 2024-09-28T02:32:08.882Z Learning: Accessibility improvements, such as adding ARIA attributes to components like `ResizeHandle`, require broader discussion and are considered out of scope for small PRs.Learnt from: junhaoliao PR: y-scope/yscope-log-viewer#74 File: new-log-viewer/src/components/CentralContainer/Sidebar/ResizeHandle.tsx:77-82 Timestamp: 2024-10-08T15:52:50.753Z Learning: Accessibility improvements, such as adding ARIA attributes to components like `ResizeHandle`, require broader discussion and are considered out of scope for small PRs.
🔇 Additional comments (2)
new-log-viewer/src/components/CentralContainer/Sidebar/index.tsx (2)
31-31: Approve consistent use of document.documentElementThe change to use
document.documentElementin bothgetPanelWidthandsetPanelWidthfunctions is a good improvement. It ensures consistency and reliability across different HTML structures.Also applies to: 41-41
Line range hint
1-155: Summary of changes and their impactThe modifications to the Sidebar component effectively address the PR objectives:
- The panel width calculation and setting now use
document.documentElement, ensuring consistency.- The introduction of
EDITOR_MIN_WIDTH_IN_PIXELSand the updated resize logic maintain a minimum editor width during resizing.- The new window resize event handler ensures the sidebar remains responsive to window size changes.
These changes collectively improve the user experience by providing a more robust and responsive sidebar resizing functionality.
Should be now addressed with the tweaks in |
@junhaoliao I think I found the root cause: We need to also need to address For example, if (
tabListRef.current.clientWidth + PANEL_CLIP_THRESHOLD_IN_PIXELS - 48 >
resizeHandlePosition
) {
// If the resize handle is positioned to the right of the <TabList/>'s right edge
// with a clipping threshold accounted, close the panel.
setPanelWidth(tabListRef.current.clientWidth);
} |
I think your analysis is correct on that we did not account the sidebar width when doing the calculations. (We can confirm by double checking whether |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
new-log-viewer/src/components/CentralContainer/Sidebar/index.tsx (3)
22-23: LGTM! Consider adding documentation for the new constant.The changes to
PANEL_CLIP_THRESHOLD_IN_PIXELSand the addition ofEDITOR_MIN_WIDTH_IN_PIXELSalign well with the PR objectives. These updates should improve the resizing behaviour of the sidebar.Consider adding a brief comment explaining the purpose of
EDITOR_MIN_WIDTH_IN_PIXELSfor better code maintainability.
86-99: LGTM! Improved resizing logic.The updated
handleResizefunction successfully implements a more robust resizing behaviour, addressing the PR objectives. The new logic ensures that the editor maintains a minimum width while allowing the panel to resize appropriately.For improved readability, consider extracting the calculation of
availableWidthinto a separate constant at the beginning of the function:const availableWidth = window.innerWidth - EDITOR_MIN_WIDTH_IN_PIXELS;This would eliminate the need to repeat this calculation and make the code slightly more DRY.
103-121: LGTM! Effective implementation of window resize handling.The new
useEffecthook successfully implements responsive panel width adjustment, addressing the PR objective. The logic for adjusting the panel width is consistent with thehandleResizefunction, ensuring a cohesive resizing behaviour.For consistency with the
handleResizefunction, consider extracting theavailableWidthcalculation into a constant at the beginning of thehandleWindowResizefunction:const handleWindowResize = () => { const availableWidth = window.innerWidth - EDITOR_MIN_WIDTH_IN_PIXELS; // ... rest of the function };This would make the code more consistent and easier to maintain.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- new-log-viewer/src/components/CentralContainer/Sidebar/index.tsx (3 hunks)
🧰 Additional context used
📓 Learnings (1)
new-log-viewer/src/components/CentralContainer/Sidebar/index.tsx (3)
Learnt from: junhaoliao PR: y-scope/yscope-log-viewer#94 File: new-log-viewer/src/components/CentralContainer/Sidebar/index.tsx:99-118 Timestamp: 2024-10-19T03:33:29.578Z Learning: In `new-log-viewer/src/components/CentralContainer/Sidebar/index.tsx`, when using `useEffect` to register window resize event handlers, it's acceptable to have an empty dependency array because the variables and functions used within the effect (`getPanelWidth`, `PANEL_CLIP_THRESHOLD_IN_PIXELS`, and `tabListRef`) are constants or refs declared outside the functional component and do not change.Learnt from: junhaoliao PR: y-scope/yscope-log-viewer#74 File: new-log-viewer/src/components/CentralContainer/Sidebar/ResizeHandle.tsx:77-82 Timestamp: 2024-09-28T02:32:08.882Z Learning: Accessibility improvements, such as adding ARIA attributes to components like `ResizeHandle`, require broader discussion and are considered out of scope for small PRs.Learnt from: junhaoliao PR: y-scope/yscope-log-viewer#74 File: new-log-viewer/src/components/CentralContainer/Sidebar/ResizeHandle.tsx:77-82 Timestamp: 2024-10-08T15:52:50.753Z Learning: Accessibility improvements, such as adding ARIA attributes to components like `ResizeHandle`, require broader discussion and are considered out of scope for small PRs.
🔇 Additional comments (2)
new-log-viewer/src/components/CentralContainer/Sidebar/index.tsx (2)
31-31: LGTM! Improved consistency in DOM element usage.The change from
document.bodytodocument.documentElementenhances code consistency. This modification aligns thegetPanelWidthfunction with other parts of the codebase that might be usingdocument.documentElement.
41-41: LGTM! Consistent use of document.documentElement.The change to use
document.documentElementinsetPanelWidthis consistent with the modification made togetPanelWidth. This ensures that both getter and setter functions operate on the same DOM element, maintaining consistency throughout the codebase.
…HOLD_IN_PIXELS and EDITOR_MIN_WIDTH_IN_PIXELS back to 250.
There was a problem hiding this comment.
I played around with current version, and found another bug. Here is my suggestions to fix issue. I think it works... Basically the idea is that you never let the window resize callback to resize smaller than the default panel. I also removed issue where resize handle was trying to set panel to a weird "available width"(when the window is very small)
| /** | ||
| * Adjusts and sets the width of the panel based on the desired width and available width for the | ||
| * panel. | ||
| * | ||
| * @param desiredWidth | ||
| */ | ||
| const adjustAndSetPanelWidth = (desiredWidth: number) => { | ||
| const availableWidth = window.innerWidth - EDITOR_MIN_WIDTH_IN_PIXELS; | ||
| setPanelWidth( | ||
| desiredWidth < availableWidth ? | ||
| desiredWidth : | ||
| availableWidth | ||
| ); | ||
| }; |
There was a problem hiding this comment.
Removing this since new methodology
| /** | |
| * Adjusts and sets the width of the panel based on the desired width and available width for the | |
| * panel. | |
| * | |
| * @param desiredWidth | |
| */ | |
| const adjustAndSetPanelWidth = (desiredWidth: number) => { | |
| const availableWidth = window.innerWidth - EDITOR_MIN_WIDTH_IN_PIXELS; | |
| setPanelWidth( | |
| desiredWidth < availableWidth ? | |
| desiredWidth : | |
| availableWidth | |
| ); | |
| }; |
| tabListRef.current.clientWidth + PANEL_CLIP_THRESHOLD_IN_PIXELS > | ||
| resizeHandlePosition | ||
| ) { | ||
| if (PANEL_CLIP_THRESHOLD_IN_PIXELS > resizeHandlePosition) { |
There was a problem hiding this comment.
See suggestion below since can't suggest
const handleResize = useCallback((resizeHandlePosition: number) => {
if (null === tabListRef.current) {
console.error("Unexpected null tabListRef.current");
return;
}
const maxHandleXPos = Math.max(window.innerWidth - EDITOR_MIN_WIDTH_IN_PIXELS, PANEL_DEFAULT_WIDTH_IN_PIXELS);
if (resizeHandlePosition < PANEL_CLIP_THRESHOLD_IN_PIXELS) {
// If the resize handle is positioned to the right of the <TabList/>'s right edge
// with a clipping threshold accounted, close the panel.
setPanelWidth(tabListRef.current.clientWidth);
} else if (resizeHandlePosition < maxHandleXPos) {
// Update the panel width with the distance between the mouse pointer and the window's
// left edge.
setPanelWidth(resizeHandlePosition);
}
}, []);
There was a problem hiding this comment.
Similar to the one in handleActiveTabNameChange, I feel we want to use Math.min(newPanelWidth, window.innerWidth - EDITOR_MIN_WIDTH_IN_PIXELS)
There was a problem hiding this comment.
I feel like that line in handleActiveTabNameChange is dangerous, as can set below the clipping width. The max here is meant to solve issues where window.innerWidth - EDITOR_MIN_WIDTH_IN_PIXELS calculation is flawed. When the window.innerwidth is small, the actual editor width is smaller than EDITOR_MIN_WIDTH_IN_PIXELS. This can give very small values nonsensical values for maxHandleXPos/available width. Say the innerwidth is 400, the editor min width is 360. Then the maxHandleXPos/avaialble is 40. However, in reality. the editor is probably only taking up 40px, and the available width should be 360.
However, im sure my calc is flawed in other ways... but for now it seems to work.
There was a problem hiding this comment.
I still think we should come up with a better fix for this later, and just approve something that works for now
There was a problem hiding this comment.
that line in handleActiveTabNameChange is dangerous, as can set below the clipping width
I agree. Let's fix it at least in a way that mobile users can read the content in the panel; we can optimize the experience in a later PR.
There was a problem hiding this comment.
I havn't tested on mobile... if my comment was unclear i basically just want to keep the max
There was a problem hiding this comment.
right, we kept the max and also added a min:
clamp(
window.innerWidth - EDITOR_MIN_WIDTH_IN_PIXELS,
PANEL_CLIP_THRESHOLD_IN_PIXELS,
PANEL_DEFAULT_WIDTH_IN_PIXELS
)
FYR
/**
* Clamps a number to the given range. E.g. If the number is greater than the range's upper bound,
* the range's upper bound is returned.
*
* @param num The number to be clamped.
* @param min The lower boundary of the output range.
* @param max The upper boundary of the output range.
* @return The clamped number.
*/
const clamp = (num: number, min: number, max: number) => Math.min(Math.max(num, min), max);
| // On initialization, register window resize event handler to resize panel width when necessary. | ||
| useEffect(() => { | ||
| const handleWindowResize = () => { | ||
| const availableWidth = window.innerWidth - EDITOR_MIN_WIDTH_IN_PIXELS; | ||
| if (getPanelWidth() > availableWidth) { | ||
| adjustAndSetPanelWidth(PANEL_CLIP_THRESHOLD_IN_PIXELS); | ||
| } | ||
| }; | ||
|
|
||
| window.addEventListener("resize", handleWindowResize); | ||
|
|
||
| return () => { | ||
| window.removeEventListener("resize", handleWindowResize); | ||
| }; | ||
| }, []); | ||
|
|
There was a problem hiding this comment.
| // On initialization, register window resize event handler to resize panel width when necessary. | |
| useEffect(() => { | |
| const handleWindowResize = () => { | |
| const availableWidth = window.innerWidth - EDITOR_MIN_WIDTH_IN_PIXELS; | |
| if (getPanelWidth() > availableWidth) { | |
| adjustAndSetPanelWidth(PANEL_CLIP_THRESHOLD_IN_PIXELS); | |
| } | |
| }; | |
| window.addEventListener("resize", handleWindowResize); | |
| return () => { | |
| window.removeEventListener("resize", handleWindowResize); | |
| }; | |
| }, []); | |
| // On initialization, register window resize event handler to resize panel width when necessary. | |
| useEffect(() => { | |
| const handleWindowResize = () => { | |
| const maxHandleXPos = Math.max(window.innerWidth - EDITOR_MIN_WIDTH_IN_PIXELS, PANEL_DEFAULT_WIDTH_IN_PIXELS); | |
| if (getPanelWidth() > maxHandleXPos) { | |
| setPanelWidth(maxHandleXPos); | |
| } | |
| }; | |
| window.addEventListener("resize", handleWindowResize); | |
| return () => { | |
| window.removeEventListener("resize", handleWindowResize); | |
| }; | |
| }, []); |
There was a problem hiding this comment.
Sorry for making a typo during refactoring, here's the intended logic. What do you think?
const handleWindowResize = () => {
const availableWidth = Math.max(
window.innerWidth - EDITOR_MIN_WIDTH_IN_PIXELS,
PANEL_CLIP_THRESHOLD_IN_PIXELS
);
if (getPanelWidth() > availableWidth) {
setPanelWidth(availableWidth);
}
};
|
Here is video of issue with previous push. Window jumps to clip size. Screen.Recording.2024-10-21.at.7.35.24.PM.mov |
… the available space.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
new-log-viewer/src/components/CentralContainer/Sidebar/index.tsx (3)
64-79: Approve refactored handleActiveTabNameChange with suggestionThe refactored handleActiveTabNameChange function improves user experience by allowing panel closure on repeated tab clicks and ensures the panel width stays within acceptable bounds. Good use of the clamp function to manage panel width.
Consider adding a brief comment explaining the parameters of the clamp function for better code readability:
const newPanelWidth = clamp( window.innerWidth - EDITOR_MIN_WIDTH_IN_PIXELS, // maximum width PANEL_CLIP_THRESHOLD_IN_PIXELS, // minimum width PANEL_DEFAULT_WIDTH_IN_PIXELS // default width );
95-104: Approve handleResize changes with suggestionThe modifications to handleResize function improve the panel resizing logic by ensuring it respects both the minimum threshold for closure and the available space. Good use of Math.min to prevent the panel from expanding beyond the available width.
Consider adding a brief comment explaining the Math.min usage for better code readability:
setPanelWidth( Math.min( resizeHandlePosition, window.innerWidth - EDITOR_MIN_WIDTH_IN_PIXELS // Ensure editor maintains minimum width ) );
108-126: Approve new useEffect for window resize with suggestionThe addition of the useEffect hook for handling window resize events is a good improvement. It ensures the panel width remains appropriate when the window is resized, and the cleanup is properly implemented.
Consider debouncing the handleWindowResize function to improve performance, especially on lower-end devices. This can prevent excessive recalculations during rapid resize events. Here's an example implementation:
import { debounce } from 'lodash'; // Or implement your own debounce function useEffect(() => { const handleWindowResize = debounce(() => { // ... existing logic ... }, 100); // Adjust the delay as needed window.addEventListener("resize", handleWindowResize); return () => { window.removeEventListener("resize", handleWindowResize); handleWindowResize.cancel(); // Cancel any pending debounced calls }; }, []);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- new-log-viewer/src/components/CentralContainer/Sidebar/index.tsx (5 hunks)
🧰 Additional context used
📓 Learnings (1)
new-log-viewer/src/components/CentralContainer/Sidebar/index.tsx (1)
Learnt from: junhaoliao PR: y-scope/yscope-log-viewer#94 File: new-log-viewer/src/components/CentralContainer/Sidebar/index.tsx:99-118 Timestamp: 2024-10-19T03:33:29.578Z Learning: In `new-log-viewer/src/components/CentralContainer/Sidebar/index.tsx`, when using `useEffect` to register window resize event handlers, it's acceptable to have an empty dependency array because the variables and functions used within the effect (`getPanelWidth`, `PANEL_CLIP_THRESHOLD_IN_PIXELS`, and `tabListRef`) are constants or refs declared outside the functional component and do not change.
🔇 Additional comments (4)
new-log-viewer/src/components/CentralContainer/Sidebar/index.tsx (4)
15-15: Verify the usage of the imported clamp functionThe addition of the clamp function import is appropriate. Ensure that it's used correctly throughout the component to constrain values within desired ranges.
24-24: Verify the usage of EDITOR_MIN_WIDTH_IN_PIXELSThe addition of the EDITOR_MIN_WIDTH_IN_PIXELS constant is appropriate for maintaining a minimum editor width. Ensure it's used consistently throughout the component to prevent the editor from becoming too narrow.
32-32: Approve the use of document.documentElementThe change from document.body to document.documentElement in both getPanelWidth and setPanelWidth functions is appropriate. It ensures consistency with the root element of the document.
Please verify that this change doesn't affect other parts of the application that might have been relying on the panel width being set on document.body.
Also applies to: 42-42
Line range hint
1-155: Overall improvements to Sidebar componentThe changes made to the Sidebar component significantly enhance its responsiveness and usability. The introduction of new constants, utility functions, and refined event handling for resizing and tab changes collectively improve the component's behaviour and user experience.
To ensure the changes work as intended across various scenarios, please conduct comprehensive testing, including:
- Resizing the window to extreme dimensions (very small and very large).
- Rapidly resizing the window to check for any performance issues.
- Testing on different devices and browsers to ensure consistent behaviour.
- Verifying that the editor maintains its minimum width under all circumstances.
- Checking that tab switching and panel closure work correctly after window resizes.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
new-log-viewer/src/components/CentralContainer/Sidebar/index.tsx (4)
24-24: LGTM: Addition ofEDITOR_MIN_WIDTH_IN_PIXELSconstantThe introduction of this constant is a good practice for maintaining a consistent minimum editor width. It enhances code readability and makes it easier to adjust this value if needed in the future.
Consider adding a brief comment explaining the purpose of this constant, such as:
// Minimum width in pixels for the editor to ensure usability across different screen sizes const EDITOR_MIN_WIDTH_IN_PIXELS = 250;
64-80: LGTM: ImprovedhandleActiveTabNameChangelogicThe refactored function enhances the handling of tab changes and panel resizing. The use of the
clampfunction ensures that the panel width remains within acceptable bounds, which is a good practice.Consider renaming the
clampfunction's first argument to improve clarity:clamp( availableWidth, PANEL_CLIP_THRESHOLD_IN_PIXELS, PANEL_DEFAULT_WIDTH_IN_PIXELS )Where
availableWidth = window.innerWidth - EDITOR_MIN_WIDTH_IN_PIXELS.This change would make the purpose of each argument in the
clampfunction more evident.
95-108: LGTM: SimplifiedhandleResizefunctionThe refactored
handleResizefunction improves readability and consistency with thehandleActiveTabNameChangefunction. The use of theclampfunction ensures that the panel width remains within acceptable bounds.Consider extracting the
clamplogic into a separate function to reduce duplication:const calculatePanelWidth = (width: number) => clamp( window.innerWidth - EDITOR_MIN_WIDTH_IN_PIXELS, PANEL_CLIP_THRESHOLD_IN_PIXELS, width ); // Then use it in both handleResize and handleActiveTabNameChange setPanelWidth(calculatePanelWidth(resizeHandlePosition));This change would improve code reusability and maintain consistency across both functions.
112-130: LGTM: Addition of window resize handlerThe new
useEffecthook for handling window resize events is well-implemented. It correctly adds and removes the event listener, which prevents potential memory leaks. The resize logic ensures that the panel width remains appropriate as the window size changes.Consider extracting the
handleWindowResizefunction outside of theuseEffecthook for better readability:const handleWindowResize = useCallback(() => { const availableWidth = Math.max( window.innerWidth - EDITOR_MIN_WIDTH_IN_PIXELS, PANEL_CLIP_THRESHOLD_IN_PIXELS ); if (getPanelWidth() > availableWidth) { setPanelWidth(availableWidth); } }, []); useEffect(() => { window.addEventListener("resize", handleWindowResize); return () => { window.removeEventListener("resize", handleWindowResize); }; }, [handleWindowResize]);This change would improve code organization and potentially allow for easier testing of the
handleWindowResizefunction.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- new-log-viewer/src/components/CentralContainer/Sidebar/index.tsx (5 hunks)
🧰 Additional context used
📓 Learnings (1)
new-log-viewer/src/components/CentralContainer/Sidebar/index.tsx (1)
Learnt from: junhaoliao PR: y-scope/yscope-log-viewer#94 File: new-log-viewer/src/components/CentralContainer/Sidebar/index.tsx:99-118 Timestamp: 2024-10-19T03:33:29.578Z Learning: In `new-log-viewer/src/components/CentralContainer/Sidebar/index.tsx`, when using `useEffect` to register window resize event handlers, it's acceptable to have an empty dependency array because the variables and functions used within the effect (`getPanelWidth`, `PANEL_CLIP_THRESHOLD_IN_PIXELS`, and `tabListRef`) are constants or refs declared outside the functional component and do not change.
🔇 Additional comments (2)
new-log-viewer/src/components/CentralContainer/Sidebar/index.tsx (2)
15-15: LGTM: Importing theclampfunctionThe addition of the
clampfunction import is appropriate and aligns with good coding practices. This utility function will be useful for constraining values within specified ranges in the modified functions.
131-131: LGTM: Added blank line for improved readabilityThe addition of this blank line enhances code readability by clearly separating the new
useEffecthook from the existing one. This is a good practice for maintaining clean and organized code.
References
new-log-viewer series: #45 #46 #48 #51 #52 #53 #54 #55 #56 #59 #60 #61 #62 #63 #66 #67 #68 #69 #70 #71 #72 #73 #74 #76 #77 #78 #79 #80 #81 #82 #83 #84 #89 #91 #93
Description
document.documentElement.getComputedStyle(document.documentElement)instead of accessingdocument.documentElement.styledirectly.Validation performed
Summary by CodeRabbit
New Features
Bug Fixes