new-log-viewer: Add tabbed Sidebar with resizable panel; Rearrange open and settings buttons. #74
Conversation
…filename class to CSS.
junhaoliao
changed the title
junhaoliao
changed the title
There was a problem hiding this comment.
- I find it a bit unintuitive that clicking the file with arrow icon opens an open dialog. I think two things would help:
- Replace the icon with a folder icon. Pretty sure that's still the most common icon for opening a file, right? At least Office applications still use it.
- Move the button to replace the current file icon in the top left. I think the file icon in the top left is not doing much, especially since we now have the file info as a tab in the sidebar.
- Alternatively, we could move the open icon above the settings icon, but I think that placement is less intuitive.
- When no file is open, can we hide the icons and perhaps display something like "No file is open."
- I know in a future PR, we'll probably hide this UI entirely when no file is open, but I think it's a small change to make for now?
- Since "FILE INFO" is the title, the other "File Info" should probably be "Name"?
- Can we add the compressed size and compression ratio to the file info?
- Can we un-highlight the last opened tab when the sidebar is closed? I could see someone misinterpreting it as a notification instead of the last tab that they had open.
- Can we add tool tips for the icons? Perhaps:
- "Open"
- "File info"
- "Settings"
- Do we want to increase the min-width before the sidebar snaps closed?
- This is minor, but can we make the buttons square rather than rectangles?
- Can we decrease the padding in the sidebar by maybe 25%?
# Conflicts: # new-log-viewer/src/components/MenuBar/index.css # new-log-viewer/src/components/StatusBar/index.css
Moved the Open button back into the menu bar.
The initial
Fixed the typo.
Can we add that in another PR? Estimating uncompressed (formatted text) sizes is possible but the calculations for different file types are worth discussions.
Done.
Done by utilizing the "title" prop. In a future PR (soon), we will add JoyUI tooltips.
Increased to 250px.
Done with width and height being 64px. To avoid misalignments with the file open button in the menu bar, added a YScope logo to match the width of the tab button.
Done. |
new-log-viewer/src/components/CentralContainer/Sidebar/SidebarTabs/FileInfoTabPanel.tsx
Outdated
Show resolved
Hide resolved
new-log-viewer/src/components/CentralContainer/Sidebar/SidebarTabs/FileInfoTabPanel.tsx
Outdated
Show resolved
Hide resolved
new-log-viewer/src/components/CentralContainer/Sidebar/index.tsx
Outdated
Show resolved
Hide resolved
Co-authored-by: kirkrodrigues <[email protected]>
![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 (5)
new-log-viewer/src/components/CentralContainer/Sidebar/SidebarTabs/FileInfoTabPanel.tsx (2)
24-28: Consider enhancing the component documentation.While the current JSDoc comment provides a basic description of the component's purpose, it could be more comprehensive. Consider adding information about:
- Any props the component might accept (even if currently there are none)
- The structure of the rendered output
- Any important state or context dependencies
Example:
/** * Displays a panel containing the file name and on-disk size of the selected file. * * This component relies on the StateContext to access file information. * It renders either a message when no file is open or a list of file details. * * @returns {JSX.Element} A CustomTabPanel containing file information */
29-59: LGTM: Well-structured component with good use of React hooks.The component is well-implemented, making good use of React hooks and context. The memoization of the formatted file size is a nice optimization touch.
One minor suggestion for improved readability:
Consider extracting the conditional rendering logic into a separate function or component. This can make the JSX more readable, especially if more complex rendering logic is added in the future.
Example:
const renderContent = () => { if (isFileUnloaded) { return "No file is open."; } return ( <List> {/* ... existing list items ... */} </List> ); }; return ( <CustomTabPanel tabName={TAB_NAME.FILE_INFO} title={TAB_DISPLAY_NAMES[TAB_NAME.FILE_INFO]} > {renderContent()} </CustomTabPanel> );new-log-viewer/src/contexts/StateContextProvider.tsx (3)
73-73: LGTM: Improved default message for log dataChanging the default
logDatamessage to "No file is open." improves user feedback and aligns with the PR comments. This change effectively addresses the suggestion to display a message when no file is open.Consider internationalizing this string for future localization support. You could use a constant like
NO_FILE_OPEN_MESSAGEthat can be easily translated in the future.
261-261: LGTM: Update file size state from worker responseThe addition of
setonDiskFileSizeInBytes(args.onDiskFileSizeInBytes);correctly updates the state with the file size received from the worker. This is a necessary step to maintain the new state.Note that if you update the setter function name as suggested in the previous comment, you'll need to update this line as well to use
setOnDiskFileSizeInBytes.
330-332: LGTM: State updates when loading a new fileThe changes in the
loadFilefunction correctly update the state when loading a new file:
- Setting
fileNameandlogDatato "Loading..." provides immediate user feedback.- Resetting
onDiskFileSizeInBytesto its default value is appropriate when starting to load a new file.Consider using a constant for the "Loading..." string to maintain consistency and ease future updates or internationalization. For example:
const LOADING_MESSAGE = "Loading..."; // Then use LOADING_MESSAGE instead of the string literal
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- new-log-viewer/src/components/CentralContainer/Sidebar/SidebarTabs/FileInfoTabPanel.tsx (1 hunks)
- new-log-viewer/src/contexts/StateContextProvider.tsx (7 hunks)
- new-log-viewer/src/services/LogFileManager/index.ts (4 hunks)
- new-log-viewer/src/services/MainWorker.ts (1 hunks)
- new-log-viewer/src/typings/worker.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- new-log-viewer/src/services/LogFileManager/index.ts
- new-log-viewer/src/services/MainWorker.ts
- new-log-viewer/src/typings/worker.ts
🧰 Additional context used
📓 Learnings (1)
new-log-viewer/src/components/CentralContainer/Sidebar/SidebarTabs/FileInfoTabPanel.tsx (2)
Learnt from: junhaoliao PR: y-scope/yscope-log-viewer#74 File: new-log-viewer/src/components/CentralContainer/Sidebar/SidebarTabs/FileInfoTabPanel.tsx:37-46 Timestamp: 2024-10-08T15:52:50.753Z Learning: In contexts where icons have labels provided via the `title` prop next to them, adding `aria-label` attributes to the icons is less necessary.Learnt from: junhaoliao PR: y-scope/yscope-log-viewer#74 File: new-log-viewer/src/components/CentralContainer/Sidebar/SidebarTabs/FileInfoTabPanel.tsx:37-46 Timestamp: 2024-09-28T01:19:47.165Z Learning: In contexts where icons have labels provided via the `title` prop next to them, adding `aria-label` attributes to the icons is less necessary.
🪛 GitHub Check: lint-check
new-log-viewer/src/contexts/StateContextProvider.tsx
[warning] 233-233:
useState call is not destructured into value + setter pair
🔇 Additional comments (5)
new-log-viewer/src/components/CentralContainer/Sidebar/SidebarTabs/FileInfoTabPanel.tsx (1)
1-21: LGTM: Imports are well-organized and complete.The imports are logically grouped, separating React hooks, Material-UI components, and custom utilities. All necessary dependencies for the component's functionality are present.
new-log-viewer/src/contexts/StateContextProvider.tsx (4)
56-56: LGTM: New state property added to interfaceThe addition of
onDiskFileSizeInBytesto theStateContextTypeinterface is correct and aligns with the PR objectives. This change ensures type safety for the new state property.
76-76: LGTM: Default state for new propertyThe initialization of
onDiskFileSizeInBytesto 0 in theSTATE_DEFAULTobject is correct and consistent with the interface change. This provides a sensible default value for the file size when no file is loaded.
438-438: LGTM: New state provided in contextThe addition of
onDiskFileSizeInBytesto the context provider value is correct and necessary. This change ensures that consumers of the context can access the new file size information.
Line range hint
1-452: Overall assessment: Well-implemented feature with minor suggestionsThe implementation of the new
onDiskFileSizeInBytesstate is consistent and aligns well with the PR objectives. The changes improve the user experience by providing more information about the loaded file and better feedback when no file is open.Key points:
- The new state is correctly added to the interface, default state, and context provider.
- State updates are handled appropriately in the worker response and file loading function.
- User feedback has been improved with better default messages.
Minor suggestions for improvement include:
- Consistent naming for the state setter function.
- Using constants for string literals to support future internationalization.
- Destructuring the useState call for consistency.
These small refinements will enhance code quality and maintainability. Great work overall!
| {isFileUnloaded ? | ||
| "No file is open." : | ||
| <List> | ||
| <CustomListItem | ||
| content={fileName} | ||
| icon={<AbcIcon/>} | ||
| slotProps={{content: {sx: {wordBreak: "break-word"}}}} | ||
| title={"Name"}/> | ||
| <Divider/> | ||
| <CustomListItem | ||
| content={formattedOriginalSize} | ||
| icon={<StorageIcon/>} | ||
| title={"Original Size"}/> | ||
| </List>} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider internationalizing the hardcoded strings.
To improve the component's readiness for internationalization, consider extracting the hardcoded English strings into a translation file or constant. This will make it easier to support multiple languages in the future.
Example using a hypothetical useTranslation hook:
import { useTranslation } from '../path/to/translation/hook';
const FileInfoTabPanel = () => {
const { t } = useTranslation();
// ...
return (
// ...
{isFileUnloaded ?
t('noFileOpen') :
<List>
<CustomListItem
content={fileName}
icon={<AbcIcon />}
title={t('name')}
/>
<Divider />
<CustomListItem
content={formattedOriginalSize}
icon={<StorageIcon />}
title={t('originalSize')}
/>
</List>
}
// ...
);
};This change will make it easier to add support for multiple languages in the future without modifying the component's logic.
| const [onDiskFileSizeInBytes, setonDiskFileSizeInBytes] = | ||
| useState(STATE_DEFAULT.onDiskFileSizeInBytes); |
There was a problem hiding this comment.
Improve state hook declaration
While the addition of the state hook for onDiskFileSizeInBytes is necessary, there are a couple of improvements we can make:
- The setter function name should follow React's convention of using "set" + capitalized state name.
- The
useStatecall should be destructured for consistency with other state declarations in this file.
Please update the state hook declaration as follows:
-const [onDiskFileSizeInBytes, setonDiskFileSizeInBytes] =
- useState(STATE_DEFAULT.onDiskFileSizeInBytes);
+const [onDiskFileSizeInBytes, setOnDiskFileSizeInBytes] = useState(STATE_DEFAULT.onDiskFileSizeInBytes);This change improves consistency and addresses the static analysis warning.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const [onDiskFileSizeInBytes, setonDiskFileSizeInBytes] = | |
| useState(STATE_DEFAULT.onDiskFileSizeInBytes); | |
| const [onDiskFileSizeInBytes, setOnDiskFileSizeInBytes] = useState(STATE_DEFAULT.onDiskFileSizeInBytes); |
🧰 Tools
🪛 GitHub Check: lint-check
[warning] 233-233:
useState call is not destructured into value + setter pair
| const {fileName, onDiskFileSizeInBytes} = useContext(StateContext); | ||
|
|
||
| const isFileUnloaded = 0 === fileName.length; | ||
| const formattedOriginalSize = useMemo( |
new-log-viewer/src/components/CentralContainer/Sidebar/SidebarTabs/FileInfoTabPanel.tsx
Outdated
Show resolved
Hide resolved
davemarco
changed the title
| display: flex; | ||
| justify-content: center; | ||
| width: 48px; | ||
| height: var(--ylv-status-bar-height); |
There was a problem hiding this comment.
This should be --ylv-menu-bar-height although they both have the same value. Similarly, the height in .menu-bar should be referencing --ylv-menu-bar-height instead as well.
@coderabbitai create a bug issue to track this.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for status-bar-height variable
echo "Searching for --ylv-status-bar-height:"
rg "--ylv-status-bar-height" -A 1 -B 1
echo ""
# Search for menu-bar-height variable (to check if it exists)
echo "Searching for --ylv-menu-bar-height:"
rg "--ylv-menu-bar-height" -A 1 -B 1
echo ""
# Check the CSS variables defined in the project
echo "Looking for CSS variable definitions:"
rg "^\s*--ylv-" -A 1 -B 1
Length of output: 2386
🏁 Script executed:
#!/bin/bash
# Create a GitHub issue to track the CSS variable inconsistency
gh issue create \
--title "Bug: MenuBar CSS uses incorrect height variable" \
--body "## Description
The MenuBar component's CSS is using the incorrect CSS variable for heights:
1. In \`.menu-bar\` class, it's using \`--ylv-status-bar-height\` but should be using \`--ylv-menu-bar-height\`
2. In \`.menu-bar-logo-container\` class, it's using \`--ylv-status-bar-height\` but should be using \`--ylv-menu-bar-height\`
Although both variables currently have the same value (32px), for semantic correctness and future maintainability, the proper variables should be used.
## Suggested Fix
In \`new-log-viewer/src/components/MenuBar/index.css\`:
\`\`\`diff
.menu-bar {
z-index: var(--ylv-menu-bar-z-index);
display: flex;
flex-direction: row;
align-items: center;
- height: var(--ylv-status-bar-height);
+ height: var(--ylv-menu-bar-height);
/* stylelint-disable-next-line custom-property-pattern */
box-shadow: 0 1px 0 0 var(--joy-palette-neutral-outlinedBorder);
}
.menu-bar-logo-container {
display: flex;
justify-content: center;
width: 48px;
- height: var(--ylv-status-bar-height);
+ height: var(--ylv-menu-bar-height);
\`\`\`
## Issue identified in PR #74 by @junhaoliao" \
--assignee "junhaoliao"
Length of output: 1309
@junhaoliao I've created a bug issue to track this CSS variable inconsistency. You're right - the MenuBar component should be using --ylv-menu-bar-height instead of --ylv-status-bar-height in both places for semantic correctness, even though they currently have the same value (32px).
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
Description
<SidebarContainer/>.<FileInfoTab/>.Validation performed
Summary by CodeRabbit
Release Notes
New Features
FileInfoTabPanelcomponent to display file information, including file name and size, in a user-friendly format.CustomListItemcomponent for improved list item presentation with icons and contextual text.CentralContainercomponent that integrates theSidebar,DropFileContainer, andEditorfor a cohesive layout.ResizeHandlecomponent for user-friendly resizing of interface elements.Styling Enhancements
CentralContainerto optimize space usage.StatusBarstyling with a newz-indexand shadow effect for improved visual layering.Context and State Management