fix(web): sidebar layout #1194
fix(web): sidebar layout #1194
Conversation
WalkthroughThe pull request implements several modifications across multiple components in the dashboard, primarily focusing on restructuring and styling. Key changes include the reorganization of sidebar components, such as replacing Changes
Possibly related PRs
Suggested reviewers
Poem
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
Documentation and Community
|
✅ Deploy Preview for reearth-web ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (6)
web/src/beta/features/Dashboard/ContentsContainer/Projects/StarredProject.tsx (1)
17-26: LGTM: Improved naming and accessibilityGood improvements:
- Fixed variable naming from
statredProjecttoproject- Added title attribute for better accessibility
Consider using nullish coalescing for the project name for consistency:
- <TitleWrapper title={project?.name}> - {project?.name} + <TitleWrapper title={project?.name ?? ""}> + {project?.name ?? ""}web/src/beta/features/Dashboard/LeftSidePanel/index.tsx (2)
Line range hint
42-66: Consider adding ARIA landmarks for improved accessibilityThe new structure with SidebarMainSection and visual dividers improves the layout organization. However, consider adding ARIA landmarks to enhance accessibility.
- <SidebarMainSection> + <SidebarMainSection role="navigation" aria-label="Main navigation">
Line range hint
68-84: Enhance footer section accessibilityThe footer section implementation maintains consistency with the main section's structure. Consider adding ARIA landmarks here as well.
- <SidebarFooterSection> + <SidebarFooterSection role="contentinfo" aria-label="Sidebar footer">web/src/beta/lib/reearth-ui/components/Collapse/index.tsx (2)
160-165: LGTM! IconWrapper styling improvements enhance icon positioning and animation.The changes improve the icon's layout and animation:
- Fixed transition property syntax
- Added explicit dimensions (16x16)
- Centered icon using flex layout
Consider using theme variables for the icon dimensions to maintain consistency:
- width: 16, - height: 16, + width: theme.metrics.icon.small, + height: theme.metrics.icon.small,
Line range hint
146-159: Simplify the rotation logic for better maintainability.The nested ternary operators for rotation calculation make the code harder to read.
Consider extracting the rotation logic into a helper function:
const IconWrapper = styled("div")<{ isCollapsed?: boolean; iconPosition?: "left" | "right"; -}>(({ isCollapsed, iconPosition }) => ({ - rotate: - iconPosition === "left" - ? isCollapsed - ? "-180deg" - : "-90deg" - : isCollapsed - ? "0deg" - : "-90deg", +}>(({ isCollapsed, iconPosition }) => { + const getRotation = () => { + if (iconPosition === "left") { + return isCollapsed ? "-180deg" : "-90deg"; + } + return isCollapsed ? "0deg" : "-90deg"; + }; + return { + rotate: getRotation(),web/src/beta/features/ProjectSettings/index.tsx (1)
88-100: LGTM: Improved sidebar structure with better component organization.The new structure using
SidebarMainSectionandSidebarButtonsWrapperprovides better organization and flexibility. Consider extracting the tabs mapping to a separate component if this pattern is reused across other settings pages.Example refactor:
const SettingsTabs: React.FC<{ tabs: typeof tabs }> = ({ tabs }) => ( <SidebarButtonsWrapper> {tabs?.map((t) => ( <SidebarMenuItem key={t.id} path={t.path} text={t.text} active={t.id === tab} icon={t.icon} /> ))} </SidebarButtonsWrapper> );Then use it as:
<SidebarMainSection> <SettingsTabs tabs={tabs} /> </SidebarMainSection>
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (9)
- web/src/beta/features/Dashboard/ContentsContainer/Projects/StarredProject.tsx (2 hunks)
- web/src/beta/features/Dashboard/LeftSidePanel/index.tsx (5 hunks)
- web/src/beta/features/Dashboard/LeftSidePanel/profile.tsx (0 hunks)
- web/src/beta/features/ProjectSettings/index.tsx (2 hunks)
- web/src/beta/features/ProjectSettings/innerPages/PublicSettings/index.tsx (2 hunks)
- web/src/beta/features/WorkspaceSetting/index.tsx (1 hunks)
- web/src/beta/lib/reearth-ui/components/Collapse/index.tsx (1 hunks)
- web/src/beta/ui/components/SettingBase/index.tsx (2 hunks)
- web/src/beta/ui/components/Sidebar/index.tsx (2 hunks)
💤 Files with no reviewable changes (1)
- web/src/beta/features/Dashboard/LeftSidePanel/profile.tsx
🔇 Additional comments (16)
web/src/beta/features/WorkspaceSetting/index.tsx (2)
28-40: LGTM! Clean component structure with proper separation of concerns.The component structure follows React best practices with proper use of fragments, conditional rendering, and clean prop passing.
39-40: Verify CursorStatus positioning.The CursorStatus component has been moved outside of SettingBase. Please ensure this change doesn't affect its visual positioning on the workspace settings page.
✅ Verification successful
Let me try different commands to gather more information about the CursorStatus component and its styling.
Based on the search results, I can now provide a conclusive response about the CursorStatus positioning:
CursorStatus positioning is properly maintained
The CursorStatus component has absolute positioning built into its implementation, making it independent of its parent container's structure. From the search results:
- The component has its own
Wrapperstyled component withposition: "absolute"and explicitleft/toppositioning- It's consistently used as a top-level component in multiple pages (Dashboard, WorkspaceSetting, ProjectSettings, Editor, AccountSetting)
- Moving it outside SettingBase won't affect its positioning as it uses absolute positioning relative to the viewport
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any related styling or positioning logic for CursorStatus # Test 1: Look for CursorStatus component definition and its styles rg -A 5 "CursorStatus" --type tsx --type css --type scss # Test 2: Check for any z-index or positioning styles that might affect CursorStatus ast-grep --pattern 'styled.$_` $$$ position: $_ $$$ `'Length of output: 147
Script:
#!/bin/bash # Search for CursorStatus component and its usage rg -A 5 "CursorStatus" # Search for styled-components definitions related to CursorStatus rg -A 5 "styled.*CursorStatus" # Look for the previous location of CursorStatus in SettingBase rg -A 5 "SettingBase"Length of output: 8743
web/src/beta/features/Dashboard/ContentsContainer/Projects/StarredProject.tsx (2)
15-15: LGTM: Improved padding controlThe switch to
noPaddingon the Collapse component provides better control over spacing through the wrapper components, aligning with the PR's layout flexibility goals.
38-43: Verify layout behavior across browsersThe combination of
height: 0andflexGrow: 1on the Wrapper, along withflex: 1on ProjectsWrapper, might cause inconsistent layout behavior across different browsers.Please verify the following scenarios:
- The sidebar content scrolls properly when it overflows
- The layout remains stable when resizing the window
- The component behaves consistently across different browsers
Would you like me to provide a more robust CSS solution that ensures consistent behavior?
Also applies to: 48-50
web/src/beta/features/Dashboard/LeftSidePanel/index.tsx (2)
2-5: LGTM: Clean import organizationThe new Sidebar component imports are well-organized and follow consistent naming conventions.
Line range hint
42-84: Verify consistent usage of new sidebar componentsThe refactored sidebar structure looks good, but let's verify that these components are used consistently across other parts of the application.
✅ Verification successful
Let me try a different search strategy to verify the component usage.
Sidebar components are consistently used across the application
The verification shows that the sidebar components (
SidebarMainSection,SidebarFooterSection, andSidebarButtonsWrapper) are properly defined inweb/src/beta/ui/components/Sidebar/index.tsxand consistently used across multiple features:
- Dashboard's LeftSidePanel
- Project Settings (main and public settings)
- Setting Base component
The implementation follows a consistent pattern across all usages, maintaining the same structural hierarchy and component relationships. The changes align well with the existing codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for usage of the new sidebar components echo "Checking usage of new sidebar components:" rg "Sidebar(MainSection|FooterSection|ButtonsWrapper)" -t tsxLength of output: 184
Script:
#!/bin/bash # Search for usage of the sidebar components with broader file patterns echo "Checking usage of sidebar components:" rg "SidebarMainSection|SidebarFooterSection|SidebarButtonsWrapper" --type-add 'tsx:*.tsx' --type-add 'ts:*.ts' --type tsx --type ts # Also check for styled-components definitions echo -e "\nChecking component definitions:" rg "styled\.(div|section|aside)" -A 2 --type-add 'tsx:*.tsx' --type-add 'ts:*.ts' --type tsx --type ts | grep -i "sidebar"Length of output: 3277
web/src/beta/ui/components/SettingBase/index.tsx (2)
6-9: LGTM: Import changes align with the sidebar refactoring.The new imports are properly organized and consistent with the sidebar layout improvements.
34-47: LGTM: Improved sidebar structure with better component organization.The new structure using
SidebarMainSectionandSidebarButtonsWrapperprovides better semantic organization while maintaining the original functionality.Let's verify that this new sidebar structure is consistently applied across the codebase:
✅ Verification successful
Based on the shell script results, I can now generate the final response:
✅ Sidebar structure changes are consistently implemented
The verification shows that:
SidebarMainSectionandSidebarButtonsWrapperare defined inweb/src/beta/ui/components/Sidebar/index.tsxand used consistently across all sidebar implementations- No legacy
SidebarSectionusage remains in the codebase- The new structure is uniformly applied in:
- Dashboard's LeftSidePanel
- ProjectSettings
- PublicSettings
- SettingBase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for consistent usage of new sidebar components # Test 1: Find all files using SidebarMainSection echo "Files using SidebarMainSection:" rg "SidebarMainSection" # Test 2: Find any remaining usage of old SidebarSection echo -e "\nChecking for remaining SidebarSection usage:" rg "SidebarSection" # Test 3: Find all files using SidebarButtonsWrapper echo -e "\nFiles using SidebarButtonsWrapper:" rg "SidebarButtonsWrapper"Length of output: 2826
web/src/beta/ui/components/Sidebar/index.tsx (5)
81-86: LGTM! Good improvements to spacing and naming.The renaming from
SidebarTopSectionWrappertoSidebarButtonsWrapperbetter reflects its purpose, and the spacing improvements using theme values ensure consistency.
89-94: LGTM! Good improvements to layout and naming.The renaming to
SidebarMainSectionand the addition offlex: 1improve both clarity and layout behavior. The consistent use of theme spacing values maintains design consistency.
96-101: LGTM! Well-structured footer section.Good implementation with
flexShrink: 0to prevent compression and consistent theme-based spacing.
103-107: LGTM! Clean divider implementation.Good use of theme values for colors and spacing, ensuring consistency with the design system.
123-123: LGTM! Consistent spacing implementation.Good use of theme spacing values for padding, maintaining design consistency.
web/src/beta/features/ProjectSettings/innerPages/PublicSettings/index.tsx (2)
3-5: LGTM: Import changes align with sidebar refactoringThe new sidebar component imports align with the PR's objective to improve sidebar layout flexibility.
113-126: LGTM: Improved sidebar structure with better component organizationThe new nested structure using
SidebarMainSectionandSidebarButtonsWrapperprovides better organization while maintaining the existing functionality.Let's verify the consistent usage of this new sidebar structure across other components:
✅ Verification successful
Let me try a different approach to verify the sidebar structure usage across components.
This should help us understand:
- Which files use these components
- Whether they are consistently used together
- The actual implementation patterns in different files
Sidebar structure is consistently implemented across components
The verification results show that the sidebar structure is consistently implemented across the codebase:
Both
SidebarMainSectionandSidebarButtonsWrappercomponents are used together in all relevant files:
- web/src/beta/features/ProjectSettings/index.tsx
- web/src/beta/features/ProjectSettings/innerPages/PublicSettings/index.tsx
- web/src/beta/features/Dashboard/LeftSidePanel/index.tsx
- web/src/beta/ui/components/SettingBase/index.tsx
The components follow the same nesting pattern (
SidebarMainSection > SidebarButtonsWrapper) in all implementations, maintaining a consistent structure.The base components are properly defined in the shared UI components directory (
web/src/beta/ui/components/Sidebar/index.tsx).The new sidebar structure is properly standardized across the application, ensuring a consistent UI pattern.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for consistent sidebar structure implementation # Expected: Similar nesting pattern of SidebarMainSection and SidebarButtonsWrapper # Search for similar sidebar structure patterns ast-grep --pattern 'SidebarMainSection { $$$ SidebarButtonsWrapper { $$$ } $$$ }'Length of output: 95
Script:
#!/bin/bash # Search for SidebarMainSection and SidebarButtonsWrapper components separately echo "=== Files using SidebarMainSection ===" rg "SidebarMainSection" -l echo -e "\n=== Files using SidebarButtonsWrapper ===" rg "SidebarButtonsWrapper" -l echo -e "\n=== Sample implementations ===" # Show some context around the usage rg "SidebarMainSection" -A 5 -B 2Length of output: 9753
web/src/beta/features/ProjectSettings/index.tsx (1)
5-5: LGTM: Import changes align with sidebar restructuring.The replacement of
SidebarSectionwithSidebarMainSectionand addition ofSidebarButtonsWrapperalign well with the PR's objective to improve sidebar layout flexibility.Also applies to: 7-8
Overview
Sidebar layout was not flex enough, this PR refactor the structure and optimize the styles.
This PR fixed the active gql indicator's position on workspace settings page as well.
What I've done
What I haven't done
How I tested
Which point I want you to review particularly
Memo
Summary by CodeRabbit
Release Notes
New Features
SidebarMainSection,SidebarButtonsWrapper, andSidebarFooterSection.Bug Fixes
Documentation
These updates aim to improve user experience by streamlining navigation and enhancing visual clarity within the application.