-
Notifications
You must be signed in to change notification settings - Fork 3
[FEAT] - my events page detail #105
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
base: development
Are you sure you want to change the base?
Conversation
mpsalunggg
commented
Jan 8, 2026
| @@ -0,0 +1,59 @@ | |||
| "use client"; | |||
|
|
|||
| import { useParams, useRouter } from "next/navigation"; | |||
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.
There are inconsistency here, few files using useRouter from @/lib/navigation, but this is using next/navigation.
Is this intended? Or auto impott problem?
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 think @/lib/navigation optimized for i18n
| return ( | ||
| <div className="container mx-auto max-w-3xl px-4 py-8"> | ||
| <div className="text-center"> | ||
| <h1 className="mb-4 text-2xl font-bold">Transaction Not Found</h1> |
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.
There are hardcoded strings in english, better to use i18n
| const { data: paymentData, isLoading } = useGetPaymentDetail(transactionId); | ||
|
|
||
| if (isLoading) { | ||
| return ( | ||
| <div className="flex min-h-screen items-center justify-center"> | ||
| <Loader /> | ||
| </div> | ||
| ); | ||
| } |
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.
There is no handled for isError or error
| id?: number; | ||
| order_no?: string; | ||
| event_id?: number; | ||
| user_id?: string; |
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.
Is there any reason why id or no like these marked as optional? I belive event should has these value
| const handleViewDetails = () => { | ||
| openDialog({ | ||
| title: "Event Details", | ||
| content: <EventDetailModal event={event} />, | ||
| size: "xl", | ||
| className: "h-[80%]", | ||
| }); | ||
| router.push(`/my-events/${event.order_no}`); | ||
| }; |
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.
order_no is optional, so it's probalby empty, better to put condition to protect this router's push if this commented code intentioanl #105 (comment)
| mutationFn: ({ event_id }: { event_id: number }) => eventsService.registerEvent(event_id), | ||
| onSuccess: (data) => { | ||
| toast.success(data?.message); | ||
| router.push(`/my-events/${data.data.order_no}`); |
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 comment with this
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.
Can refactor be like:
onSuccess: (data) => {
toast.success(data?.message);
const orderNo = data?.data?.order_no;
if (orderNo) {
router.push(`/my-events/${orderNo}`);
} else {
router.push("/my-events");
}
},So will push user to main event page when error
| const params = useParams(); | ||
|
|
||
| const transactionId = params?.transactionId as string; | ||
| const { data: paymentData, isLoading } = useGetPaymentDetail(transactionId); |
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.
useGetPaymentDetail function parameter is order_no, its better to make it consistent between order_no or transactionId
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.
Including the nested routing also using transactionId
params?.transactionId
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.
| </Button> | ||
| </div> | ||
|
|
||
| <div className="rounded-lg border border-gray-200 bg-white p-6 dark:border-gray-800 dark:bg-gray-900"> |
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 believe the current css theme can adjust it's own opposite color when activating the dark mode, can remove classnames that started with dark:
| router.push(`/my-events/${data.data.order_no}`); | ||
| }, | ||
| onError: (error) => { | ||
| toast.error(error?.message); |
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.
Depends solely on backend can be confusing if the error is not what is expected. Can put handle like this
onError: (error) => {
const message = error?.message || "Failed to xxxx";
toast.error(message);
},And use i18n to store that Failed to xxxx
