-
Notifications
You must be signed in to change notification settings - Fork 0
Issue 18 create navbar component #80
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
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.
Pull request overview
This PR implements a responsive navbar component and adds several supporting UI components and dependencies for a Django + Next.js application. The changes include both server-side infrastructure for file storage and API documentation, as well as client-side UI components for building the user interface.
Key Changes
- Added responsive navbar component with mobile hamburger menu and desktop layout
- Integrated AWS S3 storage configuration with fallback to local storage
- Added drf-spectacular for Django REST framework API documentation
- Added multiple shadcn/ui-based components (Dialog, Alert, Accordion, Spinner, ReCaptcha)
- Updated dependencies in both client and server
Reviewed changes
Copilot reviewed 21 out of 24 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| server/pyproject.toml | Added dependencies for Pygments, drf-spectacular, boto3, django-storages, and Pillow |
| server/poetry.lock | Updated lock file with new dependency versions |
| server/api/settings.py | Configured S3 storage backend with environment-based fallback |
| server/api/urls.py | Added drf-spectacular API schema and documentation endpoints |
| server/.env.example | Added AWS configuration variables |
| client/src/components/ui/navbar.tsx | Implemented responsive navbar with hamburger menu |
| client/src/components/ui/dialog.tsx | Added Dialog component wrapper |
| client/src/components/ui/alert.tsx | Created AlertDialog with loading states |
| client/src/components/ui/accordion.tsx | Implemented Accordion component |
| client/src/components/ui/spinner.tsx | Added Spinner loading component |
| client/src/components/ui/recaptchaV2.tsx | Implemented Google reCAPTCHA v2 integration |
| client/src/components/ui/button.tsx | Updated button variants with custom styles |
| client/src/app/api/verify-recaptcha/route.ts | Added server-side reCAPTCHA verification endpoint |
| client/package.json | Added UI library dependencies and updated Next.js version |
| client/tailwind.config.ts | Extended color palette with bloom project colors |
| client/src/styles/globals.css | Added custom CSS utilities and color variables |
Files not reviewed (1)
- client/package-lock.json: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| psycopg = {extras = ["binary", "pool"], version = "^3.2.1"} | ||
| freezegun = "^1.5.1" | ||
| gunicorn = "^23.0.0" | ||
| pygments = "^2.7.2" |
Copilot
AI
Dec 13, 2025
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 version constraint for pygments (^2.7.2) is outdated. This package was added in 2020, and there are newer major versions available. Consider updating to a more recent version to benefit from security patches and improvements.
| pygments = "^2.7.2" | |
| pygments = "^3.0.0" |
| AWS_ACCESS_KEY_ID = os.getenv("AWS_ACCESS_KEY_ID") | ||
| AWS_SECRET_ACCESS_KEY = os.getenv("AWS_SECRET_ACCESS_KEY") | ||
| AWS_STORAGE_BUCKET_NAME = os.getenv("AWS_STORAGE_BUCKET_NAME") | ||
| AWS_REGION_NAME = os.getenv("AWS_REGION_NAME") |
Copilot
AI
Dec 13, 2025
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.
AWS credentials are being loaded from environment variables without validation. If these credentials are invalid or expired, errors will occur at runtime during file operations rather than at startup. Consider adding validation logic to verify AWS credentials on application startup when USE_S3 is True.
| <button | ||
| className="flex flex-col items-center justify-center space-y-1 md:hidden" | ||
| onClick={() => setOpen(!open)} | ||
| > | ||
| <span | ||
| className={cn( | ||
| "block h-0.5 w-6 bg-black transition-all", | ||
| open && "translate-y-1.5 rotate-45", | ||
| )} | ||
| /> | ||
| <span | ||
| className={cn( | ||
| "block h-0.5 w-6 bg-black transition-all", | ||
| open && "opacity-0", | ||
| )} | ||
| /> | ||
| <span | ||
| className={cn( | ||
| "block h-0.5 w-6 bg-black transition-all", | ||
| open && "-translate-y-1.5 -rotate-45", | ||
| )} | ||
| /> | ||
| </button> |
Copilot
AI
Dec 13, 2025
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 hamburger menu button lacks an accessible label. Add an aria-label attribute to describe the button's purpose for screen reader users.
| status: 400, | ||
| }); | ||
| } | ||
| } catch (error) { |
Copilot
AI
Dec 13, 2025
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 error handling swallows the actual error information. The catch block should log the error for debugging purposes and potentially return different status codes based on the error type (network errors vs validation failures).
| } catch (error) { | |
| } catch (error) { | |
| // Log the error for debugging purposes | |
| console.error("Error verifying reCAPTCHA:", error); | |
| // Check if this is an Axios error (network or HTTP error) | |
| if (axios.isAxiosError && axios.isAxiosError(error)) { | |
| return new Response(JSON.stringify({ message: "Error communicating with reCAPTCHA service" }), { | |
| status: 502, // Bad Gateway | |
| }); | |
| } | |
| // For all other errors, return a generic 500 |
| } catch (e) { | ||
| console.error(e); | ||
| alert("Unable to verify Captcha. Please try again."); | ||
| setVerified(false); |
Copilot
AI
Dec 13, 2025
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 console.error followed by an alert is not ideal for production. Consider implementing a more user-friendly error notification system (like toast notifications) instead of browser alerts.
| {/* To remove in future: demo button to simulate async work */} | ||
| <Button | ||
| variant="ghost" | ||
| onClick={() => { | ||
| setIsPending(true); | ||
| // simulate async work | ||
| setTimeout(() => setIsPending(false), 1200); | ||
| }} | ||
| disabled={isPending} | ||
| > | ||
| Demo | ||
| </Button> |
Copilot
AI
Dec 13, 2025
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 demo button should be removed before merging to production. It's clearly marked for removal but is still present in the code.
| {/* To remove in future: demo button to simulate async work */} | |
| <Button | |
| variant="ghost" | |
| onClick={() => { | |
| setIsPending(true); | |
| // simulate async work | |
| setTimeout(() => setIsPending(false), 1200); | |
| }} | |
| disabled={isPending} | |
| > | |
| Demo | |
| </Button> |
client/package.json
Outdated
| "is-inside-container": "^1.0.0", | ||
| "lucide-react": "^0.516.0", | ||
| "next": "15.4.7", | ||
| "next": "^15.5.9", |
Copilot
AI
Dec 13, 2025
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 Next.js version has been changed from a pinned version (15.4.7) to a caret range (^15.5.9). This could introduce unexpected breaking changes if minor/patch versions are automatically updated. Consider keeping the exact version pinned for stability, especially in production.
| "next": "^15.5.9", | |
| "next": "15.5.9", |
Fozzyack
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.
Good work Mel! Some minor changes required.
Let me know if you need any help.
Fozzyack
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.
Good work mel. However, the buttons on the mobile navbar should be centered. Also the mobile navbar does not take up the height of the screen. Note: you may have to use a different positioning property for the mobile nav.
| Admin login | ||
| </Button> | ||
| <div className="absolute left-0 top-full max-h-screen w-full overflow-y-auto bg-white md:hidden"> | ||
| <div className="flex h-full flex-col items-end gap-6 px-8 py-6"> |
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.
items-end can be changed top items-center
Change Summary
Implemented responsive navbar component with mobile and desktop layouts. Includes alignment adjustments for logo and buttons, a mobile hamburger menu with vertically stacked buttons, and consistent styling.
Change Form
Fill this up (NA if not available). If a certain criteria is not met, can you please give a reason.
Other Information
[Is there anything in particular in the review that I should be aware of?]