-
Notifications
You must be signed in to change notification settings - Fork 3
Add Cancellation & Refunds Terms and Conditions Shipping Privacy Contact Us #21
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: main
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for open-website-2 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
shubhojit-mitra-dev
left a comment
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.
Please fix all these issues
package-lock.json
Outdated
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.
Why package-lock when we are using pnpm?
package.json
Outdated
| "@react-three/fiber": "^9.3.0", | ||
| "class-variance-authority": "^0.7.1", | ||
| "clsx": "^2.1.1", | ||
| "lucide-react": "^0.542.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.
we are using react icons, no need for additional library.
package.json
Outdated
| "lucide-react": "^0.542.0", | ||
| "motion": "^12.23.12", | ||
| "next": "15.5.2", | ||
| "next": "^15.5.1-canary.26", |
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.
why nextjs canary? what is the need, please explain it.
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.
automatically added not done by me
src/components/DarkVeil.tsx
Outdated
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.
Not required, delete this component!!!
package.json
Outdated
| "next": "15.5.2", | ||
| "next": "^15.5.1-canary.26", | ||
| "next-themes": "^0.4.6", | ||
| "ogl": "^1.0.11", |
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.
not required!
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.
using it in LightRays.tsx
package.json
Outdated
| "react": "19.1.0", | ||
| "react-dom": "19.1.0", | ||
| "react-icons": "^5.5.0", | ||
| "react-router-dom": "^7.8.2", |
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.
why rrd in Nextjs? Please go through the documentation carefully!!!
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 asked for a separate constants.ts file for all the static content
src/app/contact-us/page.tsx
Outdated
| <ComingSoon /> | ||
| <div className="relative w-full min-h-[600px]"> | ||
| <div className='absolute inset-0 -z-10 overflow-hidden'> | ||
| <LightRays /> |
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.
Not required!!!!
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.
that's the background lighting that component is from reactbit
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.
Was this necessary?
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.
in latest nextjs it is necessary, it automatically gets created
shubhojit-mitra-dev
left a comment
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.
The code is not up to the mark, please follow best software development principles. If you are not able to understand please contact me
src/components/Contact.tsx
Outdated
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 is not a shared component, use file colocation for feature specific components
src/components/Contact.tsx
Outdated
| import { Card, CardContent, CardHeader, CardTitle } from "@/components/ui/card"; | ||
| import { MdEmail, MdPhone, MdLocationOn, MdSend } from "react-icons/md";import { toast } from "sonner"; | ||
|
|
||
| const ContactForm = () => { |
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.
separate the client logic from the server logic. This code is not maintainable in the long run
src/components/Contact.tsx
Outdated
| setFormData({ name: "", email: "", subject: "", message: "" }); | ||
| }; | ||
|
|
||
| const handleChange = (e: React.ChangeEvent<HTMLInputElement | HTMLTextAreaElement>) => { |
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.
Form handling should be done with react-hook-forms and zod for proper validation. The client-side should only send sanitized data to the server. These type of mistakes leads to XSS attacks
src/app/terms_condition/page.tsx
Outdated
| <section> | ||
| <h2 className="text-2xl font-semibold mb-4 text-foreground">1. Acceptance of Terms</h2> | ||
| <p className="text-muted-foreground leading-relaxed"> | ||
| By accessing and using this website, you accept and agree to be bound by the terms and provision of this agreement. If you do not agree to abide by the above, please do not use this service. |
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 asked for a separate constants.ts file. How many times do I need to tell? I even showed you in the meeting how I want the files to be. This type of code is not at all maintainable
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.
Every single file have the same problem of not having a constants file for content data. Please create a single shared template, since all these pages look the same. Please use software principles like KISS and DRY for these kind of things.
src/components/LightRays.tsx
Outdated
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 component is completely useless, since it is only used in one place, remove it and it's dependency.
shubhojit-mitra-dev
left a comment
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 told you in the previous code review, that since all of these pages look the same, create a single template and the constants file should be with their respective pages. Please follow at least basic software design principles. I told you to ask me if you are not clear.
src/app/contact-us/page.tsx
Outdated
| import { toast } from "sonner"; | ||
|
|
||
| // ✅ Zod schema for validation + sanitization | ||
| const ContactSchema = z.object({ |
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 already told in the meet itself, The UI and the submission and validation logic should be separate. In react we are allowed to build custom hooks to separate individual logic.
src/app/contact-us/page.tsx
Outdated
| @@ -1,10 +1,239 @@ | |||
| import ComingSoon from '@/components/ComingSoon'; | |||
| 'use client'; | |||
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 entire form is not needed as a client component. Separate the logic for client and server components.
src/app/contact-us/page.tsx
Outdated
| }); | ||
|
|
||
| // ✅ Sanitize + submit | ||
| const onSubmit = async (data: ContactFormData) => { |
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 demonstrated in the meet itself, how to use the lib/apiClient.ts file for type-safe api calls.
src/components/constants.ts
Outdated
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.
why is the constants file in components folder? This is not how you structure an application. Constants for each page should be with their respective page.tsx and in each page.tsx their should be used a single template that takes the constants as it's props.
No description provided.