chore: ui improvements (detailed inside) #1786
Conversation
… gap adjustment, max-width
WalkthroughThis pull request includes several modifications primarily focused on adjusting the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (9)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (8)
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 kleros-v2-neo ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for kleros-v2-university ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for kleros-v2-testnet ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
❌ Deploy Preview for kleros-v2-testnet-devtools failed. Why did it fail? →
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (6)
web/src/layout/Header/Logo.tsx (2)
7-7: Remove unnecessary empty lineThis empty line disrupts the logical grouping of imports.
import { Link } from "react-router-dom"; - import { ArbitratorTypes, getArbitratorType } from "consts/index";
64-64: Remove unnecessary space literalThe
{" "}string literal appears to be unnecessary and can be removed.<Container> - {" "} <Link to={"/"}>web/src/pages/GetPnk/index.tsx (1)
22-22: LGTM! Consistent container width adjustment.The max-width reduction from 1780px to 1400px aligns with the systematic UI updates across multiple components, maintaining a more compact and consistent layout while preserving the responsive design.
This change is part of a broader design system update that standardizes container widths across the application, improving visual consistency.
web/src/pages/Courts/CourtDetails/StakePanel/index.tsx (1)
67-68: Consider optimizing the court name logic.While the conditional display logic works, it could be optimized for better performance and maintainability.
Consider this optimization:
- {courtName} - {courtName.toLowerCase().endsWith("court") || courtName.toLowerCase().startsWith("corte") ? null : " Court"} + {(() => { + const lowerName = courtName.toLowerCase(); + return courtName + (!lowerName.endsWith("court") && !lowerName.startsWith("corte") ? " Court" : ""); + })()}web/src/pages/Courts/CourtDetails/Stats.tsx (1)
43-43: LGTM: Responsive padding implementation looks good.The conditional padding implementation using
landscapeStyleis well-structured and aligns with the broader UI improvements mentioned in the PR objectives.Consider extracting the padding value to a constant or theme variable for better maintainability:
+ const LANDSCAPE_PADDING = '24px'; ${landscapeStyle( () => css` > * > div > div { - padding: 0 24px; + padding: 0 ${LANDSCAPE_PADDING}; } ` )}Also applies to: 48-55
web/src/layout/Header/MobileHeader.tsx (1)
Line range hint
22-26: Consider adding transitions for orientation changesThe component is hidden in landscape mode using
display: none. Consider adding CSS transitions for smoother orientation changes and ensuring the header height adjustment doesn't cause abrupt layout shifts.${landscapeStyle( () => css` + transition: all 0.3s ease; display: none; ` )}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
web/src/components/ErrorFallback.tsx(1 hunks)web/src/layout/Header/DesktopHeader.tsx(1 hunks)web/src/layout/Header/Logo.tsx(3 hunks)web/src/layout/Header/MobileHeader.tsx(1 hunks)web/src/layout/Header/index.tsx(1 hunks)web/src/pages/Cases/index.tsx(1 hunks)web/src/pages/Courts/CourtDetails/StakePanel/index.tsx(1 hunks)web/src/pages/Courts/CourtDetails/Stats.tsx(2 hunks)web/src/pages/Courts/CourtDetails/index.tsx(1 hunks)web/src/pages/Courts/index.tsx(1 hunks)web/src/pages/Dashboard/index.tsx(1 hunks)web/src/pages/GetPnk/index.tsx(1 hunks)web/src/pages/Home/index.tsx(1 hunks)web/src/pages/Resolver/index.tsx(1 hunks)web/src/pages/Settings/index.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (5)
- web/src/pages/Dashboard/index.tsx
- web/src/pages/Settings/index.tsx
- web/src/layout/Header/DesktopHeader.tsx
- web/src/pages/Cases/index.tsx
- web/src/pages/Courts/index.tsx
🧰 Additional context used
📓 Learnings (1)
web/src/pages/Courts/CourtDetails/StakePanel/index.tsx (1)
Learnt from: Harman-singh-waraich
PR: kleros/kleros-v2#1775
File: web/src/pages/Courts/CourtDetails/StakePanel/StakeWithdrawButton.tsx:0-0
Timestamp: 2024-12-09T12:36:59.441Z
Learning: In the `StakeWithdrawButton` component, the transaction flow logic is tightly linked to component updates, so extracting it into a custom hook does not provide significant benefits.
🔇 Additional comments (10)
web/src/layout/Header/Logo.tsx (2)
32-34: LGTM! Good approach to logo resizing
The use of max-height with auto width is the correct approach to maintain the logo's aspect ratio while controlling its size.
64-66: LGTM! Good component simplification
The removal of the unnecessary styled wrapper for the Link component improves code maintainability.
web/src/pages/Home/index.tsx (1)
25-25: LGTM! Consistent with application-wide container updates.
The max-width adjustment maintains visual consistency with other pages while preserving the existing layout structure and component hierarchy.
web/src/pages/Courts/CourtDetails/StakePanel/index.tsx (1)
67-68: Verify the language-specific court name handling.
The current implementation assumes "corte" as a valid prefix. Please verify if this covers all necessary language variations for court names in the system.
✅ Verification successful
Based on the search results, I can now provide a final response regarding the language-specific court name handling:
The current implementation is sufficient for handling multilingual court names.
The codebase shows that:
- Court names are managed through a policy registry system where names are set and stored with their proper localized forms
- The check for both "court" suffix and "corte" prefix is appropriate as:
- It handles English courts with the "Court" suffix
- It handles Spanish/Portuguese courts with the "Corte" prefix
- It avoids duplicate suffixes for already properly named courts
The implementation is flexible enough to handle the current multilingual requirements without needing additional language variations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other court name patterns in the codebase
echo "Searching for court name patterns..."
rg -i "court|corte" --type tsx --type ts | grep -i "name.*=.*['\"].*court"
Length of output: 189
Script:
#!/bin/bash
# Search for court name patterns in the codebase using ripgrep
echo "Searching for court name patterns..."
rg -i "court|corte" -t ts -t tsx -C 2
# Also search for any translations or language-specific configurations
echo -e "\nSearching for translation files..."
fd -e json -e yaml -e yml | xargs rg -i "court|corte"
Length of output: 82767
Script:
#!/bin/bash
# Search for court name patterns in the codebase using ripgrep with proper file type recognition
echo "Searching for court name patterns..."
rg -i "court|corte" -g "*.{ts,tsx}" -C 2
# Search for any translation files or language configurations
echo -e "\nSearching for language-specific files..."
fd -e json -e yaml -e yml | xargs rg -i "language|translation|locale"
Length of output: 68243
web/src/components/ErrorFallback.tsx (1)
20-20: LGTM: Consistent max-width standardization
The max-width reduction to 1400px aligns with the standardization being applied across multiple components in the application. This change helps maintain consistent container widths throughout the UI.
web/src/pages/Resolver/index.tsx (1)
41-41: LGTM: Consistent container width adjustment
The max-width reduction to 1400px maintains consistency with other components while preserving the responsive layout of the dispute resolver interface.
✅ Verification successful
LGTM: Verified consistent max-width usage across components
The max-width change to 1400px in the Resolver component is consistent with the established pattern across major page components. All main page containers (Dashboard, Home, Courts, Cases, Settings, GetPnk) use the same 1400px max-width, confirming this is the intended standard width for primary content areas.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify consistent max-width usage across components
# Look for any remaining instances of 1780px that might have been missed
rg -l "1780px" 'web/src'
Length of output: 22
Script:
#!/bin/bash
# Let's search for various max-width values to understand the pattern
rg "max-width:\s*\d+" web/src
Length of output: 1538
web/src/pages/Courts/CourtDetails/index.tsx (1)
80-88: LGTM: Refined spacing in panel layout
The adjustments to gap (20px → 16px) and flex basis calculation (50% - 10px → 50% - 8px) maintain the balanced two-column layout while implementing tighter spacing. These changes align with the PR's objective of layout refinements.
✅ Verification successful
The previous search might have failed because:
- The code is using styled-components which might not be recognized as CSS
- The gap property might be written in a different format
- We should search in the entire web/src directory without type restriction
Let's gather more context about the layout patterns and gap usage.
Based on the search results, I can now verify the gap usage patterns in the codebase:
Gap adjustments align with design system
The change from 20px to 16px gap and corresponding flex basis adjustment (50% - 10px → 50% - 8px) aligns with the common gap values used throughout the codebase:
- 16px is a frequently used gap value across many components (navbar, forms, cards, etc.)
- The flex basis calculation consistently follows the pattern of subtracting half the gap value to maintain proper spacing
- Other components using similar two-column layouts also use 16px gaps
The changes maintain consistency with the established design system patterns.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent gap usage in similar layouts
# Look for other components using the same pattern to ensure consistency
rg "gap: \d+px" --type css 'web/src'
Length of output: 36
Script:
#!/bin/bash
# Let's try a broader search pattern that includes styled-components syntax
rg "gap:\s*\d+px" web/src
# Also search for the landscapeStyle usage to understand the context better
rg "landscapeStyle" web/src -A 5
Length of output: 65685
web/src/pages/Courts/CourtDetails/Stats.tsx (1)
2-2: LGTM: Import changes are appropriate.
The addition of css from styled-components and landscapeStyle imports are necessary for the new conditional styling implementation.
Also applies to: 4-4
web/src/layout/Header/index.tsx (1)
20-20: Verify header alignment with padding removal
The removal of vertical padding (8px → 0) while maintaining horizontal padding could affect the vertical alignment of header elements. This change should be verified in conjunction with the new fixed heights (64px) in both Desktop and Mobile headers.
✅ Verification successful
Header vertical alignment is properly maintained
The padding removal (8px → 0) in the HeaderContainer is compensated by proper height configurations across header components:
- Both DesktopHeader and MobileHeader have explicit
height: 64px - Logo component has controlled height with
max-height: 40px - Child components maintain their internal spacing with appropriate paddings and margins
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any other padding/margin/height adjustments in header components
rg -t tsx "padding|margin|height" "web/src/layout/Header"
Length of output: 85
Script:
#!/bin/bash
# Search for any other padding/margin/height adjustments in header components
# Using ripgrep without file type filter since .tsx extension isn't recognized
rg "padding|margin|height" "web/src/layout/Header/"
Length of output: 5177
web/src/layout/Header/MobileHeader.tsx (1)
20-20: Consider responsive height adjustments
While setting a fixed height ensures consistency, consider using min-height or making the height responsive for different screen sizes and content lengths. The current fixed height of 64px might be restrictive on smaller devices or with varying content.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
web/src/pages/Courts/CourtDetails/StakePanel/Simulator/Header.tsx (1)
Line range hint
1-65: Consider enhancing component maintainabilityWhile the current implementation is functional, there are a few suggestions to improve maintainability:
- Consider extracting magic numbers (32px, 14px) to theme constants
- The Last30Days text could be externalized for i18n support
Example implementation:
+ import { useTranslation } from 'react-i18next'; + import { theme } from 'styles/theme'; const StyledPNKLogo = styled(PNKLogo)` - width: 32px; - height: 32px; + width: ${theme.iconSizes.medium}; + height: ${theme.iconSizes.medium}; // ... `; const Last30DaysText = styled.p` margin: 0; - font-size: 14px; + font-size: ${theme.typography.sizes.small}; font-weight: 600; `; const Header: React.FC = () => { + const { t } = useTranslation(); return ( // ... - <Last30DaysText>Last 30 Days</Last30DaysText> + <Last30DaysText>{t('common.last30Days')}</Last30DaysText> // ... ); };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
web/src/components/DisputePreview/DisputeContext.tsx(2 hunks)web/src/components/ExtraStatsDisplay.tsx(3 hunks)web/src/hooks/queries/usePopulatedDisputeData.ts(2 hunks)web/src/pages/Cases/CaseDetails/Evidence/EvidenceSearch.tsx(1 hunks)web/src/pages/Courts/CourtDetails/StakePanel/Simulator/Header.tsx(1 hunks)web/src/pages/Courts/CourtDetails/StakePanel/index.tsx(2 hunks)web/src/pages/Home/CourtOverview/ExtraStats.tsx(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- web/src/pages/Cases/CaseDetails/Evidence/EvidenceSearch.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- web/src/pages/Courts/CourtDetails/StakePanel/index.tsx
🧰 Additional context used
📓 Learnings (1)
web/src/hooks/queries/usePopulatedDisputeData.ts (1)
Learnt from: Harman-singh-waraich
PR: kleros/kleros-v2#1703
File: web/src/hooks/queries/usePopulatedDisputeData.ts:58-61
Timestamp: 2024-11-12T04:49:53.174Z
Learning: In `web/src/hooks/queries/usePopulatedDisputeData.ts`, the query and subsequent logic only execute when `disputeData.dispute?.arbitrableChainId` and `disputeData.dispute?.externalDisputeId` are defined, so `initialContext` properties based on these values are safe to use without additional null checks.
🔇 Additional comments (10)
web/src/pages/Courts/CourtDetails/StakePanel/Simulator/Header.tsx (1)
17-17: LGTM! Gap adjustment aligns with design system changes
The reduction in gap from 16px to 12px is consistent with the PR's objective of standardizing spacing across components.
web/src/components/ExtraStatsDisplay.tsx (2)
34-36: LGTM: Styled component follows best practices
The StyledInternalLink component is well-implemented, maintaining consistency with the existing styling patterns.
44-44: Verify the court detail routes
The implementation looks good, but let's ensure the court detail routes are consistently defined across the application.
Also applies to: 50-62
✅ Verification successful
Let me gather more information about the route definitions to ensure completeness of the verification.
Based on the gathered information, I can now provide the final response:
Court route patterns are consistently implemented
The verification shows that court-related routes are consistently implemented across the application:
- Main courts route pattern
/courts/:idis consistently used in links and navigation - The parent route
courts/*in app.tsx properly handles all court-related routes - A special case
/court/*exists in the Resolver context, which is a separate feature
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistency in court detail route patterns
# Expected: All court detail routes should follow the pattern '/courts/:id'
# Search for court detail route definitions
rg -g '*.{ts,tsx}' "courts/.*" --type-add 'tsx:*.{ts,tsx}'
# Search for route components handling court details
ast-grep --pattern 'Route $_ path="/courts/$_"'
Length of output: 952
Script:
#!/bin/bash
# Search for all route-related patterns in the codebase
ast-grep --pattern 'Route $_ path=$_'
# Search for any court-related route definitions or constants
rg -g '*.{ts,tsx}' 'const.*[Rr]oute.*court' --type-add 'tsx:*.{ts,tsx}'
rg -g '*.{ts,tsx}' 'path.*court' --type-add 'tsx:*.{ts,tsx}'
Length of output: 464
web/src/pages/Home/CourtOverview/ExtraStats.tsx (1)
90-91: LGTM: Component update is well-implemented
The ExtraStats component correctly implements the new courtId functionality.
web/src/hooks/queries/usePopulatedDisputeData.ts (1)
51-51: LGTM: Chain ID handling is appropriate
The addition of DEFAULT_CHAIN to the graphqlBatcher.fetch call is well-implemented. Based on the retrieved learning, the null checks in this context are properly handled.
web/src/components/DisputePreview/DisputeContext.tsx (5)
17-17: LGTM! Good use of ExternalLink component.
Using the ExternalLink component instead of a regular anchor tag ensures consistent styling and behavior across the application.
69-76: LGTM! Improved markdown content structure.
The separate ReactMarkdownWrapper components for question and description provide better organization and styling control.
79-81: LGTM! Good use of ExternalLink with proper attributes.
The implementation maintains important accessibility attributes (target, rel) while using the consistent ExternalLink component.
88-91: LGTM! Better semantic HTML structure.
The simplified structure using semantic HTML elements (<small> and <label>) improves accessibility and maintainability.
24-28: Consider making ReactMarkdownWrapper more reusable.
The styling for markdown content might be needed in other components. Consider moving this styled component to a shared location if it's used elsewhere in the application.
✅ Verification successful
Let's check if there are existing styled wrappers for markdown content and examine the shared ReactMarkdown component to see if this styling concern is already addressed elsewhere or if it should be consolidated there.
Consider consolidating markdown styling in VotingHistory component
The codebase already has a similar styled wrapper in web/src/pages/Cases/CaseDetails/Voting/VotingHistory.tsx that removes paragraph margins. Instead of creating a new styled component, consider reusing the StyledReactMarkDown from VotingHistory or consolidating both styling needs into that component.
web/src/pages/Cases/CaseDetails/Voting/VotingHistory.tsx: ContainsStyledReactMarkDownwithp { margin: 0; }web/src/components/DisputePreview/DisputeContext.tsx: Your new component withp:first-of-type { margin: 0; }
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if ReactMarkdown is used in other components
rg "ReactMarkdown" -l | grep -v "DisputeContext.tsx"
Length of output: 513
Script:
#!/bin/bash
# Check if similar styled wrappers for ReactMarkdown exist
rg -A 5 "styled.*ReactMarkdown|styled.*markdown" --ignore-case
# Check the shared ReactMarkdown component implementation
cat web/src/components/ReactMarkdown.tsx
Length of output: 4933
kemuru
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
web/src/components/Verdict/DisputeTimeline.tsx (1)
Line range hint
67-116: Consider extracting icon mapping logicThe timeline item generation logic handles multiple icon conditions. Consider extracting the icon mapping into a separate function or constant for better maintainability and readability.
Example approach:
const getTimelineIcon = ( isLastRound: boolean, isRuled: boolean, isOverridden: boolean, itemType: 'decision' | 'appeal' ) => { if (itemType === 'appeal') return StyledClosedCircle; if (isLastRound && isRuled && !isOverridden) return ClosedCaseIcon; return undefined; };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
web/src/components/StyledIcons/ClosedCircleIcon.tsx(2 hunks)web/src/components/Verdict/DisputeTimeline.tsx(3 hunks)web/src/pages/Courts/CourtDetails/StakePanel/index.tsx(2 hunks)web/src/pages/Courts/CourtDetails/index.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- web/src/pages/Courts/CourtDetails/index.tsx
- web/src/pages/Courts/CourtDetails/StakePanel/index.tsx
🔇 Additional comments (3)
web/src/components/StyledIcons/ClosedCircleIcon.tsx (1)
5-5: LGTM! Export change enhances component reusability
The export of StyledClosedCircle allows for consistent icon styling across components, which aligns with the PR's objective of maintaining visual consistency.
web/src/components/Verdict/DisputeTimeline.tsx (2)
4-5: LGTM! Import organization enhances code readability
The reorganization of imports and addition of StyledClosedCircle improves code structure while maintaining functionality.
Also applies to: 23-23
105-105: LGTM! Icon replacement maintains visual consistency
The replacement of the appeal status icon with StyledClosedCircle aligns with the PR's objective of maintaining consistent styling across components.
kemuru
changed the title
|
Code Climate has analyzed commit c65acd9 and detected 12 issues on this pull request. Here's the issue category breakdown:
View more on Code Climate. |
|
PR-Codex overview
This PR primarily focuses on enhancing the styling and layout of various components within the application, including adjustments to paddings, heights, and gaps. It also introduces new constants for landscape styles and modifies some components to improve responsiveness and visual consistency.
Detailed summary
height: 64pxtoDesktopHeaderandMobileHeader.paddinginHeaderContainer.gapinPNKLogoAndTitleand other components.MAX_WIDTH_LANDSCAPEconstant.StyledClosedCircleto be exported.max-widthto useMAX_WIDTH_LANDSCAPEin several components.getCourtIdmethod toIStatinterface and utilized it inExtraStatsDisplay.StyledLinkwithLinkinLogocomponent.DisputeContextwithExternalLinkand styled components.Summary by CodeRabbit
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Style
Chores