-
Notifications
You must be signed in to change notification settings - Fork 4
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
fix(web): add some missing styling #1279
Conversation
WalkthroughThe changes in this pull request include the addition of the Changes
Possibly related PRs
Suggested labels
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
CodeRabbit Configuration File (
|
✅ Deploy Preview for reearth-cms ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
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.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
web/src/components/molecules/Asset/Asset/AssetBody/sideBarCard.tsx (1)
Line range hint
18-26
: Consider standardizing color formats and extracting magic numbers.While the white background addition improves visual separation, there are some style consistency improvements that could be made:
const SideBarCardWrapper = styled.div` padding: 12px; margin-bottom: 8px; width: 100%; display: flex; flex-direction: column; - box-shadow: 0 4px 4px 0 #00000025; - background-color: #fff; + box-shadow: 0 4px 4px 0 rgba(0, 0, 0, 0.25); + background-color: rgba(255, 255, 255, 1); `;web/src/components/molecules/Content/Table/filterDropdown.tsx (1)
98-103
: Consider adding a tooltip for truncated textThe Title styled component implementation correctly handles text overflow using standard CSS practices. However, when text is truncated, users might need to see the full text.
Consider wrapping the Title component with a Tooltip component to show the full text on hover:
+import Tooltip from "@reearth-cms/components/atoms/Tooltip"; const FilterDropdown: React.FC<Props> = ({ // ... return ( // ... - <Title>{filter.title}</Title> + <Tooltip title={filter.title}> + <Title>{filter.title}</Title> + </Tooltip> // ... ); };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (9)
- web/src/components/atoms/Icon/icons.ts (2 hunks)
- web/src/components/molecules/Asset/Asset/AssetBody/Asset.tsx (2 hunks)
- web/src/components/molecules/Asset/Asset/AssetBody/sideBarCard.tsx (2 hunks)
- web/src/components/molecules/Asset/AssetList/index.tsx (1 hunks)
- web/src/components/molecules/Common/CommentsPanel/index.tsx (3 hunks)
- web/src/components/molecules/Content/Table/DropdownRender/index.tsx (1 hunks)
- web/src/components/molecules/Content/Table/filterDropdown.tsx (2 hunks)
- web/src/components/molecules/Content/Table/index.tsx (4 hunks)
- web/src/components/molecules/Request/List/index.tsx (1 hunks)
✅ Files skipped from review due to trivial changes (4)
- web/src/components/molecules/Asset/Asset/AssetBody/Asset.tsx
- web/src/components/molecules/Asset/AssetList/index.tsx
- web/src/components/molecules/Content/Table/DropdownRender/index.tsx
- web/src/components/molecules/Request/List/index.tsx
🧰 Additional context used
🔇 Additional comments (10)
web/src/components/molecules/Asset/Asset/AssetBody/sideBarCard.tsx (1)
Line range hint
4-16
: LGTM! Removing the style prop improves maintainability.The removal of the style prop centralizes styling in styled-components, making the component more consistent and maintainable. While this reduces flexibility for edge cases, it's a good tradeoff for maintaining design consistency.
web/src/components/molecules/Common/CommentsPanel/index.tsx (3)
75-76
: LGTM: Improved icon alignment.The styling changes improve the vertical alignment of the collapsed state icon by using padding-top and center justification.
85-86
: LGTM: Enhanced typography.The font-size reduction and line-height addition improve readability and maintain consistent typography across the application.
45-45
: Verify the "comment" icon availability.The icon has been changed from "message" to "comment". Let's verify that this icon is properly imported and available in the icons collection.
✅ Verification successful
Let me gather more information to verify the complete icon setup.
Let me try a different approach to verify the icon setup completely.
Based on the results, I can now provide the final response:
The "comment" icon is properly configured and available
The verification shows that:
CommentOutlined
is correctly imported from@ant-design/icons
- The icon is properly mapped in the icons object as
comment: CommentOutlined
- This confirms that the icon change from "message" to "comment" in the CommentsPanel component is valid and will work as expected
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if the "comment" icon is properly imported and mapped in icons.ts # Test: Search for comment icon definition rg -A 5 "CommentOutlined.*from.*@ant-design/icons" rg -A 5 "comment:.*CommentOutlined"Length of output: 199
Script:
#!/bin/bash # Check the icon imports and mapping in the Icon component rg -A 10 "import.*CommentOutlined" web/src/components/atoms/Icon/Length of output: 67
Script:
#!/bin/bash # Check the content of the icons.ts file to verify the complete setup cat web/src/components/atoms/Icon/icons.ts # Also check the imports in index.ts cat web/src/components/atoms/Icon/index.tsLength of output: 5202
web/src/components/molecules/Content/Table/filterDropdown.tsx (1)
80-80
: LGTM: Title component integrationThe Title component is appropriately placed to handle the filter title display within the button content.
web/src/components/atoms/Icon/icons.ts (2)
54-54
: LGTM! Import follows consistent patternThe CommentOutlined icon import maintains the established import structure and alphabetical ordering.
181-181
: LGTM! Verify icon usage in CommentsPanelThe comment icon mapping follows the established pattern and is appropriately named.
Let's verify the icon usage in the CommentsPanel component:
✅ Verification successful
Let me check the actual usage of the comment icon in these files to verify it's being used correctly.
Icon mapping is correctly used in the codebase
The verification shows that the
comment
icon is properly used in the CommentsPanel component as a toggle button for collapsing/expanding the comments section. The implementation inweb/src/components/molecules/Common/CommentsPanel/index.tsx
uses the exact icon key that was added to the mapping:<StyledIcon icon="comment" onClick={() => onCollapse(false)} />🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of the new comment icon # Expected: Should find usage in CommentsPanel component # Search for usage of the comment icon rg -l "icon.*comment" web/src/components/Length of output: 222
Script:
#!/bin/bash # Check how the comment icon is used in the identified files rg "icon.*comment" web/src/components/molecules/Request/Details/Comment.tsx web/src/components/molecules/Common/CommentsPanel/index.tsx web/src/components/molecules/Common/CommentsPanel/Comment.tsx -B 2 -A 2Length of output: 1751
web/src/components/molecules/Content/Table/index.tsx (3)
529-529
: LGTM: Simplified dropdown render implementation.The changes improve code readability by removing redundant type declarations and unused style props.
Also applies to: 538-538
713-713
: Good defensive programming with null coalescing.The addition of the null coalescing operator (
??
) provides better handling of potentially undefinedfield.id
values.
883-893
: Verify dropdown menu's max dimensions.The new styling improves the dropdown menu's appearance and handling of overflowing content. However, the fixed dimensions might need verification:
- max-height: 256px might be too restrictive for menus with many items
- max-width: 332px might truncate longer text items
Run this script to check for potential content that might be affected:
✅ Verification successful
Based on the search results, I can see extensive usage of dropdown menus across the codebase. Let me analyze the specific dropdown menu dimensions in the context of their usage.
This will help verify if there are any dropdown menu items with long text content that might be affected by the max-width restriction.
Let me check one more aspect - the actual content being displayed in these dropdowns to understand if the height limitation might be an issue.
This will help verify if there are any dropdown menus with potentially long lists that might be affected by the max-height restriction.
Based on the analysis of the codebase, I can now provide a final assessment of the dropdown menu dimensions.
The dropdown menu dimensions are appropriate for the use cases.
The max dimensions (max-height: 256px, max-width: 332px) are well-suited because:
Content length: The search for long text content (>30 chars) shows only a few instances, primarily in form labels and settings, which are properly handled with text overflow ellipsis.
Menu items: The dropdown implementations found in the codebase are used for:
- Model card options (ProjectOverview/ModelCard.tsx)
- Color palette selection (MultiValueField/MultValueColoredTag)
- These are typically short lists with concise text
Scrolling behavior: The
overflow-y: auto
property ensures longer lists remain accessible even with the height restriction.The styling provides a good balance between:
- Readable width for common text lengths
- Reasonable height that prevents dropdowns from dominating the viewport
- Proper overflow handling with ellipsis and scrolling
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for dropdown menu items that might exceed the max-width rg -g '*.{ts,tsx,js,jsx}' --multiline --multiline-dotall 'items.*=.*\[.*\{.*label:.*\}.*\]' -A 5Length of output: 137757
Script:
#!/bin/bash # Search for dropdown menu items with long text content rg -g '*.{ts,tsx,js,jsx}' 'label:.*[^"]{30,}' -A 1Length of output: 1118
Script:
#!/bin/bash # Search for dropdown menu implementations with list items rg -g '*.{ts,tsx,js,jsx}' --multiline 'menu.*=.*\{.*items:.*\[.*\].*\}' -A 5Length of output: 1414
Overview
This PR adds some missing styling.
What I've done
Summary by CodeRabbit
Release Notes
New Features
CommentOutlined
icon to the icon set for enhanced visual representation.Styling Enhancements
AssetMolecule
,CommentsPanel
,DropdownRender
, andRequestListMolecule
components to improve visual presentation.Title
styled component in theFilterDropdown
for better text handling.Bug Fixes