-
Notifications
You must be signed in to change notification settings - Fork 47
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
feat: improved overview page, evidence page, voting history page, rem… #1779
Conversation
…oved notif icon etc
WalkthroughThe pull request introduces multiple modifications across various components, primarily focusing on styling updates and structural enhancements. Key changes include the addition of new styled components, renaming existing ones, and adjustments to layout properties to improve visual consistency. Notable components affected are Changes
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
🪧 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-testnet ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ 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. |
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 (10)
web/src/components/Verdict/Answer.tsx (1)
31-33
: Consider handling edge casesWhile the current ruling display looks good, consider adding a data-testid attribute for testing and handling potential XSS for the dynamic currentRuling value.
- {currentRuling !== 0 ? <small>Answer 0x{currentRuling}</small> : <small>Refuse to Arbitrate</small>} + {currentRuling !== 0 ? ( + <small data-testid="ruling-answer">Answer 0x{currentRuling.toString()}</small> + ) : ( + <small data-testid="ruling-refuse">Refuse to Arbitrate</small> + )}web/src/components/DisputePreview/DisputeContext.tsx (1)
59-67
: Enhanced typography consistency with styled componentsGood addition of
StyledSmall
andStyledLabel
components. This improves maintainability and ensures consistent styling across voting options.Consider extracting these styled components to a shared styles file if they might be reused across other components.
web/src/pages/Cases/CaseDetails/Voting/index.tsx (1)
78-84
: Improved loading state handlingGood implementation of loading state with Skeleton component. This provides better user feedback during data fetching.
Consider extracting the Skeleton dimensions into theme constants if they're used across multiple components:
const SKELETON_DIMENSIONS = { width: 300, height: 20 } as const;web/src/components/Verdict/FinalDecision.tsx (1)
62-69
: Consider consolidating arrow link stylesWhile the ReStyledArrowLink provides good customization, having multiple styled versions of the same component might lead to inconsistency.
Consider making StyledArrowLink more configurable through props:
const StyledArrowLink = styled(Link)<{ size?: 'small' | 'medium' | 'large' }>` font-size: ${({ size = 'medium' }) => ({ small: '14px', medium: '16px', large: '18px' }[size])}; > svg { height: ${({ size = 'medium' }) => ({ small: '15px', medium: '18px', large: '21px' }[size])}; width: auto; } `;web/src/pages/Cases/CaseDetails/Voting/VotesDetails/index.tsx (1)
24-40
: Improve accordion styling consistencyThe styling changes to the accordion improve visual consistency and spacing. However, consider extracting magic numbers into theme constants for better maintainability.
Consider refactoring the hardcoded values:
- padding: 16px ${responsiveSize(8, 18)} !important; + padding: ${({ theme }) => theme.spacing.medium} ${({ theme }) => responsiveSize(theme.spacing.small, theme.spacing.large)} !important;web/src/pages/Cases/CaseDetails/Voting/VotingHistory.tsx (1)
110-131
: Consider extracting TabsContainer content into a separate componentThe TabsContainer section contains multiple related components and could benefit from being extracted into its own component for better maintainability and reusability.
Consider creating a new component:
interface VotingTabsProps { currentTab: number; rounds: Array<any>; // Replace with proper type localRounds: Array<any>; // Replace with proper type disputeData: any; // Replace with proper type answers: Array<any>; // Replace with proper type drawnJurors: Array<any>; // Replace with proper type } const VotingTabs: React.FC<VotingTabsProps> = ({ currentTab, rounds, localRounds, disputeData, answers, drawnJurors, }) => ( <TabsContainer> <StyledTabs currentValue={currentTab} items={rounds.map((_, i) => ({ text: `Round ${i + 1}`, value: i, }))} callback={(i: number) => setCurrentTab(i)} /> <PendingVotesBox current={localRounds.at(currentTab)?.totalVoted} total={rounds.at(currentTab)?.nbVotes} court={rounds.at(currentTab)?.court.name ?? ""} /> <VotesAccordion drawnJurors={drawnJurors} period={disputeData?.dispute?.period} answers={answers} isActiveRound={localRounds?.length - 1 === currentTab} hiddenVotes={Boolean(disputeData?.dispute?.court.hiddenVotes)} /> </TabsContainer> );web/src/components/EvidenceCard.tsx (2)
132-138
: Consider consolidating hover stylesThe hover styles are duplicated within the same block. Consider consolidating them for better maintainability.
const HoverStyle = css` - :hover { - text-decoration: underline; - color: ${({ theme }) => theme.primaryBlue}; - cursor: pointer; - } :hover { + &, + label { text-decoration: underline; color: ${({ theme }) => theme.primaryBlue}; cursor: pointer; } - label { - text-decoration: underline; - color: ${({ theme }) => theme.primaryBlue}; - cursor: pointer; - } } `;
233-235
: Consider accessibility improvementsThe label element is being used without an associated form control. Consider using a more semantic element for the timestamp.
- <label>{formatDate(Number(timestamp), true)}</label> + <span className="timestamp">{formatDate(Number(timestamp), true)}</span>web/src/pages/Courts/CourtDetails/StakePanel/Simulator/index.tsx (2)
Line range hint
235-265
: LGTM! Consider adding ARIA labels for better accessibility.The code changes look good. Removing the unused
index
parameter from the map function improves code cleanliness. The component's structure and functionality are well-maintained.Consider adding ARIA labels to improve accessibility, particularly for the arrow icon that indicates staking direction. Here's a suggested improvement:
- <StyledArrowRightIcon {...{ isStaking }} /> + <StyledArrowRightIcon + {...{ isStaking }} + aria-label={`Arrow indicating ${isStaking ? 'increase' : 'decrease'} in ${item.title.toLowerCase()}`} + />
Line range hint
1-265
: Consider architectural improvements for better maintainability.While not directly related to the current changes, here are some suggestions to improve the code architecture:
- Move calculation functions like
calculateJurorOdds
to a separate utility file to improve modularity.- Consider using
useMemo
for complex calculations in thesimulatorItems
array to optimize performance.- Simplify undefined checks using optional chaining where applicable.
Example of moving calculations to a utility file:
// utils/staking-calculations.ts export const calculateJurorOdds = (newStake: number, totalStake: number): string => { const odds = totalStake !== 0 ? (newStake * 100) / totalStake : 0; return `${odds.toFixed(2)}%`; }; export const calculateExpectedVotes = ( effectiveStake: number | undefined, treeVotesPerPnk: number | undefined ): string | undefined => { return effectiveStake !== undefined && treeVotesPerPnk !== undefined ? beautifyStatNumber(effectiveStake * treeVotesPerPnk) : undefined; };Example of memoizing simulatorItems:
const simulatorItems = useMemo( () => [ { title: "Votes", icon: <GavelIcon />, currentValue: currentExpectedVotes, futureValue: futureExpectedVotes, }, // ... other items ], [currentExpectedVotes, futureExpectedVotes, currentDrawingOdds, futureDrawingOdds, currentExpectedRewardsUSD, futureExpectedRewardsUSD] );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (13)
web/src/components/DisputePreview/DisputeContext.tsx
(3 hunks)web/src/components/EvidenceCard.tsx
(5 hunks)web/src/components/Verdict/Answer.tsx
(1 hunks)web/src/components/Verdict/DisputeTimeline.tsx
(0 hunks)web/src/components/Verdict/FinalDecision.tsx
(3 hunks)web/src/components/Verdict/VerdictBanner.tsx
(1 hunks)web/src/layout/Header/navbar/Menu/index.tsx
(2 hunks)web/src/pages/Cases/CaseDetails/Evidence/index.tsx
(0 hunks)web/src/pages/Cases/CaseDetails/Voting/VotesDetails/index.tsx
(1 hunks)web/src/pages/Cases/CaseDetails/Voting/VotingHistory.tsx
(4 hunks)web/src/pages/Cases/CaseDetails/Voting/index.tsx
(2 hunks)web/src/pages/Courts/CourtDetails/StakePanel/Simulator/index.tsx
(1 hunks)web/src/utils/beautifyStatNumber.ts
(1 hunks)
💤 Files with no reviewable changes (2)
- web/src/pages/Cases/CaseDetails/Evidence/index.tsx
- web/src/components/Verdict/DisputeTimeline.tsx
✅ Files skipped from review due to trivial changes (1)
- web/src/layout/Header/navbar/Menu/index.tsx
🔇 Additional comments (12)
web/src/components/Verdict/Answer.tsx (1)
24-28
: LGTM! Semantic HTML improvements
The switch to <small>
tags improves semantic markup while maintaining the visual hierarchy. The conditional rendering for the description separator is well-handled.
web/src/utils/beautifyStatNumber.ts (1)
21-21
: Verify impact of decimal place change
Changing from toFixed(0)
to toFixed(2)
for numbers ≤ 0 affects display consistency. While this standardizes decimal places, it may impact readability for whole numbers.
web/src/components/Verdict/VerdictBanner.tsx (1)
10-10
: LGTM! Improved vertical alignment
Adding align-items: center
improves the visual alignment between the icon and text in the banner.
web/src/components/DisputePreview/DisputeContext.tsx (2)
29-29
: Improved readability with lighter font weight
Reducing the font weight from 600 to 400 for the description text improves readability, especially for longer content.
101-105
: Improved voting options layout
The refactored voting options structure with styled components provides better visual hierarchy and semantic HTML.
web/src/pages/Cases/CaseDetails/Voting/index.tsx (1)
34-37
: Enhanced layout structure with InfoCardContainer
Good addition of a dedicated container for the info card with proper spacing and flex layout.
web/src/components/Verdict/FinalDecision.tsx (3)
44-50
: Improved verdict layout with flex container
Good addition of VerdictContainer with proper spacing and alignment. The flex layout ensures consistent spacing between elements.
120-122
: Consistent link styling
Good application of the custom styled link component with proper icon alignment.
101-115
: Enhanced verdict and jury decision layout
Good restructuring of the verdict and jury decision components with proper containment and conditional rendering.
Let's verify the conditional rendering logic:
✅ Verification successful
Let me gather more context about the component's logic and dependencies.
Let me check one more aspect to understand the complete flow of the ruling states.
Based on the verification results, I can now provide the final response:
Conditional rendering logic is correctly implemented
The verdict and jury decision components' conditional rendering is properly structured:
- When ruled: Shows final decision with "The jury decided in favor of"
- When not ruled but voting in progress (periodIndex > 1 and votes cast): Shows current winning option with "This option is winning"
- Base container and banner are always rendered
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for potential edge cases in the ruling conditions
ast-grep --pattern 'ruled && (
$$$
)'
# Check for potential missing ruling states
ast-grep --pattern 'periodIndex > 1 && localRounds?.at(localRounds.length - 1)?.totalVoted > 0 && (
$$$
)'
Length of output: 668
Script:
#!/bin/bash
# Check the component's imports and type definitions
rg -A 5 "interface|type" web/src/components/Verdict/FinalDecision.tsx
# Look for other usages of ruling conditions
rg "ruled|periodIndex|localRounds" web/src/components/Verdict/FinalDecision.tsx
# Check VerdictBanner component
ast-grep --pattern 'const VerdictBanner = ({ ruled }: { ruled: boolean }) => {
$$$
}'
Length of output: 778
Script:
#!/bin/bash
# Check for Periods enum and its usage
rg -A 5 "enum Periods|type Periods"
# Check for dispute type definitions
rg -A 5 "interface.*Dispute|type.*Dispute"
# Look for AnswerDisplay component usage
ast-grep --pattern 'AnswerDisplay {...{ answer, currentRuling }}'
Length of output: 70170
web/src/pages/Cases/CaseDetails/Voting/VotesDetails/index.tsx (1)
46-47
: LGTM: Card styling improvements
The added padding and border enhance the visual hierarchy and separation of cards.
web/src/pages/Cases/CaseDetails/Voting/VotingHistory.tsx (1)
28-32
: LGTM: Improved layout structure
The new flex container and TabsContainer components improve the layout organization and spacing consistency.
Also applies to: 59-62
web/src/components/EvidenceCard.tsx (1)
29-47
: LGTM: Improved content organization
The restructured TopContent and new IndexAndName components improve the visual hierarchy and maintainability of the card layout.
Also applies to: 49-54
❌ Deploy Preview for kleros-v2-testnet-devtools failed. Why did it fail? →
|
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 (4)
web/src/pages/Resolver/Parameters/Jurors.tsx (2)
9-9
: Consider making chain selection more flexibleThe component now uses a hardcoded
DEFAULT_CHAIN
for arbitration cost queries. This might limit flexibility in multi-chain scenarios.Consider:
- Making the chain configurable through props or context
- Adding error handling for unsupported chains
- Using the user's connected chain instead of a default value
Example approach:
- import { DEFAULT_CHAIN } from "consts/chains"; + import { useNetwork } from 'wagmi' // or your preferred web3 library const Jurors: React.FC = () => { + const { chain } = useNetwork() // ... const { data } = useReadKlerosCoreArbitrationCost({ // ... - chainId: DEFAULT_CHAIN, + chainId: chain?.id, // ... });Also applies to: 62-62
Line range hint
41-47
: Add chain information to the arbitration cost displayThe arbitration cost is displayed without indicating which chain it's for, which could be confusing in a multi-chain context.
Consider updating the StyledDisplay component to show the chain:
const StyledDisplay = styled(DisplaySmall)` width: 290px; margin-bottom: ${responsiveSize(20, 48)}; h2::after { - content: "ETH"; + content: "${props => `ETH (${props.chainName})`}"; margin-left: 4px; }web/src/components/EvidenceCard.tsx (2)
29-47
: LGTM! Consider documenting the styling constants.The renamed
TopContent
component with improved spacing and margin handling looks good. However, consider extracting the magic numbers (8, 24, 4) into styled constants for better maintainability.+const PADDING = { + mobile: 8, + desktop: 24 +}; +const GAP = { + mobile: 4, + desktop: 8 +}; const TopContent = styled.div` display: flex; flex-direction: column; - padding: ${responsiveSize(8, 24)}; - gap: ${responsiveSize(4, 8)}; + padding: ${responsiveSize(PADDING.mobile, PADDING.desktop)}; + gap: ${responsiveSize(GAP.mobile, GAP.desktop)};
132-138
: Consider consolidating duplicate hover styles.The hover styles contain duplicate blocks. Consider consolidating them for better maintainability.
const HoverStyle = css` :hover { text-decoration: underline; color: ${({ theme }) => theme.primaryBlue}; cursor: pointer; - } - :hover { label { text-decoration: underline; color: ${({ theme }) => theme.primaryBlue}; cursor: pointer; } } `;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
web/src/components/EvidenceCard.tsx
(5 hunks)web/src/components/Verdict/FinalDecision.tsx
(5 hunks)web/src/pages/Resolver/Parameters/Jurors.tsx
(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- web/src/components/Verdict/FinalDecision.tsx
🔇 Additional comments (6)
web/src/components/EvidenceCard.tsx (6)
49-54
: LGTM! Clean and focused styling.
The new IndexAndName
component provides a clear structure for displaying the index and name elements.
67-69
: LGTM! Consistent with other components.
The margin reset for paragraphs aligns with similar updates in other components like VotingHistory
.
Line range hint 86-107
: LGTM! Well-structured responsive styling.
The BottomLeftContent
component effectively handles both mobile and desktop layouts using the landscapeStyle utility.
108-124
: LGTM! Comprehensive account display styling.
The AccountContainer
component provides clear styling for the account section, including proper handling of the Identicon canvas size.
146-148
: LGTM! Good component reusability.
The StyledExternalLink
component effectively reuses the HoverStyle
CSS.
218-236
: LGTM! Improved component structure.
The updated JSX structure with the new styled components provides better organization and semantic markup.
@@ -41,13 +45,13 @@ const StyledCard = styled(Card)` | |||
width: 100%; | |||
height: auto; | |||
padding: 11.5px ${responsiveSize(8, 18)}; | |||
margin: 8px 0; | |||
border: 1px solid ${({ theme }) => theme.stroke}; |
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.
Code Climate has analyzed commit a1f57bf and detected 4 issues on this pull request. Here's the issue category breakdown:
View more on Code Climate. |
|
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.
lgtm
…oved notif icon etc
PR-Codex overview
This PR focuses on improving the UI components and styling across various files, enhancing layout consistency, and updating some logic for better functionality. It also introduces new styled components and refactors existing ones for clarity and maintainability.
Detailed summary
DisputeTimeline.tsx
,PendingVotesBox.tsx
, andVerdictBanner.tsx
.beautifyStatNumber.ts
.AnswerDisplay
inAnswer.tsx
to usesmall
tags.StyledSmall
andStyledLabel
inDisputeContext.tsx
.InfoCardContainer
inVoting.tsx
for layout.VotingHistory.tsx
to includeRulingAndRewardsIndicators
conditionally.EvidenceCard.tsx
to improve structure and styling consistency.VerdictContainer
inFinalDecision.tsx
for better layout management.NotificationsIcon
inMenu/index.tsx
.Summary by CodeRabbit
Release Notes
New Features
DisputeContext
,EvidenceCard
, andFinalDecision
.VotingHistory
component layout with a newTabsContainer
for better organization.InfoCardContainer
to improve the layout of the voting information display.Bug Fixes
DisputeTimeline
to accurately reflect dispute statuses.Styling Improvements
BannerContainer
,ScrollButton
, andStyledAccordion
, to enhance visual consistency and user experience.AnswerDisplay
component by replacing styled components with standard HTML elements.VotesAccordion
andVotingHistory
components for better readability.DisputeContext
component for improved text presentation.StyledBox
inPendingVotesBox
to adjust layout margins.Chores
Simulator
component by removing unnecessary parameters from the mapping function.beautifyStatNumber
function to improve number formatting consistency.Jurors
component to utilize a newDEFAULT_CHAIN
constant for improved functionality.