-
Notifications
You must be signed in to change notification settings - Fork 0
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: missing event details #1230
Conversation
Reviewer's Guide by SourceryThis pull request adds support for displaying details for Updated class diagram for EventDetailSheetclassDiagram
class EventDetailSheet {
+event: any
+onClose: () => void
}
EventDetailSheet *-- UserAllowedDetails : Uses
EventDetailSheet *-- UserDisallowedDetails : Uses
class UserAllowedDetails {
+details: UserAllowedEvent
}
class UserDisallowedDetails {
+details: UserDisallowedEvent
}
class DetailsCard {
+details: any[]
}
UserAllowedDetails *-- DetailsCard : Uses
UserDisallowedDetails *-- DetailsCard : Uses
class EvmAddress {
+address: string
}
class EvmAddressBalances {
+address: string
}
DetailsCard *-- EvmAddress : Uses
EvmAddress *-- EvmAddressBalances : Uses
note for EventDetailSheet "Renders details for various event types, including UserAllowedEvent and UserDisallowedEvent."
note for UserAllowedDetails "Displays details for UserAllowedEvent."
note for UserDisallowedDetails "Displays details for UserDisallowedEvent."
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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.
Hey @roderik - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider extracting the common logic in
UserAllowedDetails
andUserDisallowedDetails
into a shared component or helper function to reduce duplication.
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟢 Review instructions: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
details: UserAllowedEvent; | ||
} | ||
|
||
export function UserAllowedDetails({ details }: UserAllowedDetailsProps) { |
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.
suggestion: Consider reducing duplicate logic between user event components.
Both UserAllowedDetails and UserDisallowedDetails render an EvmAddress with balances using almost identical logic. Abstracting this repeated pattern into a shared component or utility function could simplify maintenance as additional user event types are added.
details: UserDisallowedEvent; | ||
} | ||
|
||
export function UserDisallowedDetails({ details }: UserDisallowedDetailsProps) { |
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.
issue (complexity): Consider creating a generic details component that accepts event details and labels as props to avoid repeating the same logic in multiple components, such as the allowed and disallowed event components, and then updating the allowed/disallowed components to use the new generic component..
You can reduce complexity by abstracting the repeated details logic into a single parameterized component. Instead of having nearly identical components for allowed and disallowed events, create a generic details component that accepts event details and any labels or other minor variations as props. For example:
interface EventDetailsProps<T> {
details: T;
userLabel: string;
}
export function EventDetails<T extends { user: { id: string } }>({
details,
userLabel,
}: EventDetailsProps<T>) {
const detailItems = [
{
key: "user",
label: userLabel,
value: (
<EvmAddress address={details.user.id}>
<EvmAddressBalances address={details.user.id} />
</EvmAddress>
),
},
];
return <DetailsCard details={detailItems} />;
}
Then update your allowed/disallowed components to use the new generic component. For example:
import { useTranslations } from "next-intl";
interface UserDisallowedDetailsProps {
details: UserDisallowedEvent;
}
export function UserDisallowedDetails({ details }: UserDisallowedDetailsProps) {
const t = useTranslations("components.asset-events-table.details");
return <EventDetails details={details} userLabel={t("user")} />;
}
This refactoring keeps all functionality intact while reducing redundant code and simplifying future maintenance.
…rice * main: chore(deps): update dependency @types/node to v22.13.13 (#1244) chore(deps): update github/codeql-action digest to 1b549b9 (#1241) feat: add input suggestions based on history (#1240) feat: add issue button to all table tabes (#1239) fix: type the validated fields array (#1238) fix: user balances in popup (#1237) fix: page titles and graphs (#1235) fix: rtl issues (#1233) feat: convert last zod usages to typebox (#1232) fix: collateral proof validity (#1231) fix: translated errors (#1229) fix: missing event details (#1230) fix: formfield postfix ui (#1228) fix: asset details and actions (#1227)
Summary by Sourcery
New Features: