style(web): move scroll bar styles to theme #1211
Conversation
WalkthroughThe changes in this pull request primarily focus on the removal of custom scrollbar styles across multiple components in the application, replacing them with a centralized approach using 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
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 (1)
web/src/services/theme/reearthTheme/common/index.ts (1)
Line range hint 1-38: Consider adding documentation for scroll bar usage patterns.
The centralization of scroll bar styles in the theme is a good architectural decision that promotes consistency and maintainability. To maximize the benefits of this change, consider:
- Adding JSDoc comments to document when to use
scrollBarvsuniversalScrollBar - Creating usage examples in the component documentation
- Adding migration guidelines for components still using custom scroll bar styles
This will help maintain consistency as the codebase grows and make it easier for other developers to adopt these themed scroll bars.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (12)
- web/src/beta/features/Dashboard/index.tsx (1 hunks)
- web/src/beta/features/Editor/Map/shared/SharedComponent.tsx (1 hunks)
- web/src/beta/features/ProjectSettings/index.tsx (1 hunks)
- web/src/beta/features/Visualizer/Crust/Widgets/Widget/builtin/DataAttribution/index.tsx (1 hunks)
- web/src/beta/lib/reearth-ui/components/PopupMenu/index.tsx (1 hunks)
- web/src/beta/lib/reearth-ui/components/Selector/index.tsx (1 hunks)
- web/src/beta/ui/components/ManagerBase/ManagerContent.tsx (1 hunks)
- web/src/beta/ui/components/SettingBase/index.tsx (1 hunks)
- web/src/beta/ui/fields/CommonField.tsx (1 hunks)
- web/src/beta/ui/layout/Area/index.tsx (1 hunks)
- web/src/services/theme/reearthTheme/common/index.ts (3 hunks)
- web/src/services/theme/reearthTheme/common/scrollBar.ts (1 hunks)
✅ Files skipped from review due to trivial changes (6)
- web/src/beta/features/ProjectSettings/index.tsx
- web/src/beta/features/Visualizer/Crust/Widgets/Widget/builtin/DataAttribution/index.tsx
- web/src/beta/lib/reearth-ui/components/PopupMenu/index.tsx
- web/src/beta/lib/reearth-ui/components/Selector/index.tsx
- web/src/beta/ui/components/SettingBase/index.tsx
- web/src/services/theme/reearthTheme/common/scrollBar.ts
🔇 Additional comments (7)
web/src/beta/ui/components/ManagerBase/ManagerContent.tsx (1)
9-9: LGTM! Clean implementation of theme-based scrollbar styling.
The change effectively implements the centralized scrollbar styling approach while maintaining the component's responsive behavior.
Let's verify the theme type definitions:
✅ Verification successful
Theme integration verified and properly implemented
The universalScrollBar is correctly defined in the theme system and properly used in the component:
- Type definitions and implementation exist in
web/src/services/theme/reearthTheme/common/scrollBar.ts - The theme object is correctly imported and spread in the styled component
- The implementation is consistent with other components using the same styling
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify theme type definitions include universalScrollBar
# Expected: Find type definitions for universalScrollBar in theme files
# Search for theme type definitions
rg -t typescript "universalScrollBar.*interface|interface.*universalScrollBar|type.*universalScrollBar"
# Search for actual theme implementation
rg -t typescript "universalScrollBar.*=.*{|export.*universalScrollBar"
Length of output: 371
Script:
#!/bin/bash
# Search for theme type definitions and implementations with fixed regex
rg "universalScrollBar"
# Specifically look for theme-related files
fd "theme" --type f --extension ts --extension tsx --exec cat {}
Length of output: 6315
Script:
#!/bin/bash
# Get the content of the scrollBar.ts file to verify the implementation
cat web/src/services/theme/reearthTheme/common/scrollBar.ts
# Check how the theme is imported and used in the component
cat web/src/beta/ui/components/ManagerBase/ManagerContent.tsx
Length of output: 1316
web/src/beta/ui/fields/CommonField.tsx (1)
35-35: Verify if scrollbar styles are necessary for this component.
The Wrapper component doesn't have any overflow properties set, so the scrollbar styles won't have any effect. Consider if this component actually needs scrolling behavior.
If scrolling is needed:
const Wrapper = styled("div")(({ theme }) => ({
display: "flex",
flexDirection: "column",
gap: theme.spacing.small,
+ overflow: "auto",
...theme.universalScrollBar
}));If not, remove the unnecessary scrollbar styles:
const Wrapper = styled("div")(({ theme }) => ({
display: "flex",
flexDirection: "column",
gap: theme.spacing.small,
- ...theme.universalScrollBar
}));Let's verify if this component needs scrolling behavior:
web/src/services/theme/reearthTheme/common/index.ts (3)
4-9: LGTM! Clean import structure.
The new scroll bar imports are well-organized and follow the existing pattern of importing both types and values.
33-35: LGTM! Consistent object structure.
The new scroll bar properties are properly added to the common object, maintaining consistency with the type definition.
23-24: Verify the distinction between scrollBar and universalScrollBar.
While the type additions are clean, it would be helpful to understand the intended use cases for each scroll bar type to ensure they're applied consistently across components.
Let's check the implementation details:
✅ Verification successful
The previous output revealed the implementation difference (universalScrollBar uses * ::-webkit-scrollbar while scrollBar uses ::-webkit-scrollbar), but we need to understand their usage patterns to provide a complete verification.
Clear distinction found between scrollBar and universalScrollBar implementations
The verification reveals that both scroll bar types serve different purposes:
-
universalScrollBar(with* ::-webkit-scrollbar) is used for global styling contexts, applied to components that may contain nested scrollable content. Found in:- Layout components (Area, ManagerContent, Dashboard)
- Setting panels (SettingBase, ProjectSettings)
- Form fields (CommonField)
- Data displays (DataAttribution)
-
scrollBar(with::-webkit-scrollbar) is used for self-contained components where the scroll styling should only apply to the component itself. Found in:- UI components (Selector, PopupMenu)
The distinction is appropriate and consistently applied across the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Examine the scrollBar module implementation and its usage
# Check scrollBar.ts implementation
echo "=== ScrollBar Implementation ==="
cat web/src/services/theme/reearthTheme/common/scrollBar.ts
# Check usage patterns
echo -e "\n=== Usage Patterns ==="
rg -A 2 "theme\.scrollBar|theme\.universalScrollBar" --type ts --type tsx
Length of output: 1178
Script:
#!/bin/bash
# Let's try a different approach to find usage patterns
# Search for any mentions of scrollBar or universalScrollBar in ts files
echo "=== ScrollBar Usage Patterns ==="
rg "scrollBar|universalScrollBar" --type ts -C 2
# Also check for style-related imports and usage
echo -e "\n=== Style Imports and Usage ==="
rg "import.*scrollBar|from.*scrollBar" --type ts -C 2
Length of output: 6898
web/src/beta/features/Dashboard/index.tsx (1)
79-79: LGTM! Scrollbar styling successfully moved to theme
The change appropriately implements the centralized scrollbar styling approach by utilizing theme.universalScrollBar. This aligns well with the PR's objective of standardizing scrollbar styles across the application.
Let's verify that the theme properly defines the universal scrollbar styles:
✅ Verification successful
Scrollbar styling implementation verified and properly defined
The theme.universalScrollBar is correctly implemented in the theme system with proper styling for scrollbar width, track, thumb, and hover states. The implementation uses rgba colors for proper transparency and includes all necessary scrollbar pseudo-elements.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the theme implementation of universalScrollBar
# Expected: Find the definition of universalScrollBar in theme files
# Search for universalScrollBar definition in theme files
rg -g '*.ts*' 'universalScrollBar.*=' --type ts
Length of output: 141
Script:
#!/bin/bash
# Let's check the actual implementation of universalScrollBar
cat web/src/services/theme/reearthTheme/common/scrollBar.ts
Length of output: 938
web/src/beta/ui/layout/Area/index.tsx (1)
425-425: LGTM! Consistent with the theme standardization effort.
The addition of theme.universalScrollBar to the Window component aligns with the PR objective of centralizing scrollbar styles in the theme.
Let's verify the theme implementation:
✅ Verification successful
Let me gather more information about the theme implementation.
✅ Verified: Theme's universalScrollBar is properly implemented and consistently used
The universalScrollBar is correctly defined in web/src/services/theme/reearthTheme/common/scrollBar.ts with a complete set of scrollbar styling rules, and it's being consistently used across 8 components including the Area component. The implementation aligns with the PR's objective of standardizing scrollbar styling.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the universalScrollBar theme implementation and its usage across the codebase
# Test 1: Check if universalScrollBar is properly defined in the theme
rg -A 5 "universalScrollBar.*=.*{"
# Test 2: Check for consistent usage of universalScrollBar across components
rg "theme\.universalScrollBar"
Length of output: 845
Script:
#!/bin/bash
# Search for the definition of universalScrollBar in theme files
rg -l "universalScrollBar" web/src/beta/theme/
# Check the actual implementation
ast-grep --pattern 'const universalScrollBar = $_'
ast-grep --pattern 'export const universalScrollBar = $_'
Length of output: 2843
| ["* ::-webkit-scrollbar-thumb:hover"]: { | ||
| background: theme.relative.lighter | ||
| } | ||
| ...theme.universalScrollBar |
There was a problem hiding this comment.
💡 Codebase verification
Custom scrollbar styles need to be migrated to use theme.scrollBar or theme.universalScrollBar
The verification reveals inconsistencies in scrollbar styling implementation:
- Several components still use direct
::-webkit-scrollbarstyling:web/src/beta/features/Visualizer/Crust/StoryPanel/PanelContent/index.tsxweb/src/beta/features/Visualizer/Crust/Widgets/Widget/builtin/Timeline/UI/index.tsx
- Only two components are currently using the themed scrollbar styles:
web/src/beta/lib/reearth-ui/components/Selector/index.tsxweb/src/beta/lib/reearth-ui/components/PopupMenu/index.tsx
While this change is correct, there are other components that need similar updates to maintain consistency across the codebase.
🔗 Analysis chain
LGTM! The change aligns with the centralization of scrollbar styles.
The modification correctly implements the universal scrollbar styles from the theme, which supports the PR's objective of centralizing scrollbar styling.
Let's verify the theme implementation:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the consistency of scrollbar style implementation across the codebase
# Expected: All components should use either theme.universalScrollBar or theme.scrollBar
# Test 1: Check for any remaining webkit-scrollbar implementations that should be migrated
rg -g '*.{ts,tsx}' '::-webkit-scrollbar'
# Test 2: Verify the usage of theme scrollbar styles
rg -g '*.{ts,tsx}' 'theme\.(universal)?scrollBar'
Length of output: 1539
Overview
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
Window, in theArealayout to improve layout management.Documentation
Chores