-
Notifications
You must be signed in to change notification settings - Fork 49
refactor(web): case details overview file too big #1340
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
Conversation
✅ Deploy Preview for kleros-v2 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
const QuestionAndDescription = styled.div` | ||
display: flex; | ||
flex-direction: column; | ||
> * { | ||
margin: 0px; | ||
} | ||
`; |
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.
let's take the margin out and make it more specific using styled components
const QuestionAndDescription = styled.div` | |
display: flex; | |
flex-direction: column; | |
> * { | |
margin: 0px; | |
} | |
`; | |
const QuestionAndDescription = styled.div` | |
display: flex; | |
flex-direction: column; | |
`; | |
const StyledReactMarkDown = styled(ReactMarkDown)` | |
margin: 0px; | |
` |
const Answers = styled.div` | ||
display: flex; | ||
flex-direction: column; | ||
|
||
span { | ||
margin: 0px; | ||
display: flex; | ||
gap: 8px; | ||
} | ||
`; |
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.
same here:
const Answers = styled.div` | |
display: flex; | |
flex-direction: column; | |
span { | |
margin: 0px; | |
display: flex; | |
gap: 8px; | |
} | |
`; | |
const AnswersContainer = styled.div` | |
display: flex; | |
flex-direction: column; | |
`; | |
const Answer = styled.span` | |
margin: 0px; | |
display: flex; | |
gap: 8px; | |
` |
{disputeTemplate?.frontendUrl && ( | ||
<a href={disputeTemplate?.frontendUrl} target="_blank" rel="noreferrer"> | ||
Go to arbitrable | ||
</a> | ||
)} |
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.
I have been trying to favor the following syntax on these situations (make sure to run it through the linter 😅 ):
{disputeTemplate?.frontendUrl && ( | |
<a href={disputeTemplate?.frontendUrl} target="_blank" rel="noreferrer"> | |
Go to arbitrable | |
</a> | |
)} | |
{isUndefined(disputeTemplate?.frontendUrl) | |
? null | |
: ( | |
<a href={disputeTemplate?.frontendUrl} target="_blank" rel="noreferrer"> | |
Go to arbitrable | |
</a> | |
) | |
} |
</a> | ||
)} | ||
<VotingOptions> | ||
{disputeTemplate && <h3>Voting Options</h3>} |
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.
same as with the previous comment, let's try the other syntax
> svg { | ||
width: 16px; | ||
fill: ${({ theme }) => theme.primaryBlue}; | ||
} |
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.
let's take this out into its own styled component
> h1 { | ||
margin: 0; | ||
} |
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.
which component is this style for?
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.
This was for the title, moved it to DisputeContext now
{...{ rewards, category }} | ||
/> | ||
</Container> | ||
<Policies disputePolicyURI={disputeTemplate?.policyURI} courtId={courtPolicy && court?.id} /> |
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.
check if the courtID
being passed as props (line 48) can be used here instead of courtPolicy && court?.id
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.
Yes it works , passing that to the Policies component now
const localRounds = getLocalRounds(votingHistory?.dispute?.disputeKitDispute); | ||
const courtName = courtPolicy?.name; | ||
const court = disputeDetails?.dispute?.court; | ||
const rewards = court ? `≥ ${formatEther(court.feeForJuror)} ETH` : undefined; |
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.
let's use useMemo
here
return ( | ||
<> | ||
<Container> | ||
<DisputeBasicInfo disputeTemplate={disputeTemplate} /> |
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.
maybe change the name of this component so that it doesn't get mixed up with DisputeInfo
🤔 something like DisputeQuestion
or DisputeContext
seems fine I think 🤔
Code Climate has analyzed commit a0a583c and detected 17 issues on this pull request. Here's the issue category breakdown:
View more on Code Climate. |
Kudos, SonarCloud Quality Gate passed!
|
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
Refactored pages/Case/CaseDetails/Overview file into smaller components.
PR-Codex overview
Detailed summary
Policies
in theOverview
folder.DisputeContext
in theOverview
folder.Overview
component to use the new components and display dispute information.Overview.tsx
andPolicies.tsx
.