navbar component#320
Conversation
|
@ishanraj953 is attempting to deploy a commit to the santanu-atta03's projects Team on Vercel. A member of the Team first needs to authorize it. |
Thanks for creating a PR for your Issue!
|
|
Hi, please review my PR once and if you see any potential changes that need to be made. If you like the PR, then put the tag of OSCG contribution. |
There was a problem hiding this comment.
Pull request overview
This PR refactors duplicate navbar implementations across multiple pages into a centralized, reusable Navbar component. The refactoring addresses issue #249 by extracting common navigation logic from Dashboard.jsx and AboutUs.jsx (and other pages) into a shared component at Frontend/src/components/Navbar.jsx. The new component supports multiple visual tones (landing, skin, light) and variants (pill, simple) with configurable props for authentication buttons, theme toggles, and active link highlighting.
However, the PR also includes an undocumented React downgrade from version 19.1.1 to 18.3.1, which is a significant breaking change not mentioned in the PR description.
Changes:
- Created reusable Navbar component with three tone configurations and two variant styles
- Replaced inline navbar code in 9 pages with the new Navbar component
- Downgraded React from 19.1.1 to 18.3.1 (undocumented major change)
Reviewed changes
Copilot reviewed 11 out of 13 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| Frontend/src/components/Navbar.jsx | New reusable navbar component with configurable tones, variants, and behavior |
| Frontend/src/pages/Terms.jsx | Replaced inline navbar with Navbar component using tone="light" |
| Frontend/src/pages/Privacy.jsx | Replaced inline navbar with Navbar component using tone="light" |
| Frontend/src/pages/PricingPage.jsx | Replaced inline navbar with Navbar component using tone="light" |
| Frontend/src/pages/Landing.jsx | Replaced inline navbar with Navbar component using tone="landing" with scroll functionality |
| Frontend/src/pages/FAQ.jsx | Replaced custom navbar with Navbar component using variant="simple" |
| Frontend/src/pages/CookiePolicy.jsx | Replaced inline navbar with Navbar component using tone="light" |
| Frontend/src/pages/ContactUs.jsx | Replaced inline navbar with Navbar component using tone="skin" |
| Frontend/src/pages/Career.jsx | Replaced inline navbar with Navbar component using tone="light" |
| Frontend/src/pages/AboutUs.jsx | Replaced inline navbar with Navbar component using tone="skin" |
| Frontend/package.json | Downgraded React from 19.1.1 to 18.3.1 and react-dom accordingly |
| Frontend/package-lock.json | Updated lockfile to reflect React 18.3.1 and related dependency changes |
| package-lock.json | Added peer dependencies for React 19.2.4 (likely auto-generated for root dependencies) |
Files not reviewed (1)
- Frontend/package-lock.json: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "react": "^18.3.1", | ||
| "react-dom": "^18.3.1", |
There was a problem hiding this comment.
This PR downgrades React from version 19.1.1 to 18.3.1, which is a major version change, but this isn't mentioned in the PR description. The README.md indicates the project uses React 19 and even has special instructions for handling React 19 peer dependencies. This significant change should be documented and explained. Was this downgrade intentional? If so, please update the README.md to reflect this change and explain the rationale. If not, this may be an unintended change that should be reverted.
| <div className="flex items-center gap-2 sm:gap-3"> | ||
| <div className="relative"> | ||
| <div className="w-10 h-10 sm:w-12 sm:h-12 rounded-lg sm:rounded-xl flex items-center justify-center "> | ||
| <img src={logo} alt="logo" /> |
There was a problem hiding this comment.
The variable 'logo' is used but never imported or defined in this component. This will cause a runtime error when the Navbar is rendered with variant="simple" (as used in FAQ.jsx). You need to add an import statement at the top of the file, such as: import logo from "../assets/intervyologo.png"
| mobileMenu: "bg-white backdrop-blur-md rounded-2xl shadow-xl border border-gray-200", | ||
| mobileLink: "text-gray-600 hover:text-gray-900", | ||
| menuButton: "text-gray-900 hover:bg-gray-100", | ||
| logoPrimary: "text-white-800", |
There was a problem hiding this comment.
The class name 'text-white-800' is not a valid Tailwind CSS class. The standard Tailwind color classes use hyphens, so this should likely be 'text-gray-100' or 'text-white' depending on the desired color. If you intended to use a custom color, ensure it's defined in your Tailwind configuration.
| logoPrimary: "text-white-800", | |
| logoPrimary: "text-white", |
| </div> | ||
| )} | ||
| </nav> | ||
| <Navbar tone="light" activeKey="about" /> |
There was a problem hiding this comment.
The activeKey prop is set to "about" but this is the Terms and Conditions page, not the About page. This will incorrectly highlight the "About" link in the navbar when users are on the Terms page. Consider either removing the activeKey prop or setting it to a value that represents the current page, or adding a "terms" key to the navbar links if you want to highlight it.
| <Navbar tone="light" activeKey="about" /> | |
| <Navbar tone="light" /> |
| </div> | ||
| )} | ||
| </nav> | ||
| <Navbar tone="light" activeKey="about" /> |
There was a problem hiding this comment.
The activeKey prop is set to "about" but this is the Privacy Policy page, not the About page. This will incorrectly highlight the "About" link in the navbar when users are on the Privacy page. Consider either removing the activeKey prop or setting it to a value that represents the current page, or adding a "privacy" key to the navbar links if you want to highlight it.
| <Navbar tone="light" activeKey="about" /> | |
| <Navbar tone="light" /> |
| </div> | ||
| )} | ||
| </nav> | ||
| <Navbar tone="light" activeKey="about" /> |
There was a problem hiding this comment.
The activeKey prop is set to "about" but this is the Cookie Policy page, not the About page. This will incorrectly highlight the "About" link in the navbar when users are on the Cookie Policy page. Consider either removing the activeKey prop or setting it to a value that represents the current page, or adding a "cookie-policy" key to the navbar links if you want to highlight it.
| <Navbar tone="light" activeKey="about" /> | |
| <Navbar tone="light" /> |
| "react": "^18.3.1", | ||
| "react-dom": "^18.3.1", |
There was a problem hiding this comment.
React has been downgraded from ^19.1.1 to ^18.3.1. However, note that react-is at version 19.2.3 (line 35) should typically match the React version for compatibility. Consider downgrading react-is to a version compatible with React 18 (e.g., ^18.3.1) to avoid potential compatibility issues.
|
@ishanraj953 there are many conflicts, resolve all of them |
|
@santanu-atta03 Please check it once |
|
@santanu-atta03 Please check it once |
|
@santanu-atta03 Could please review my PR |
Pull Request — Intervyo
📌 Related Issue
Fixes #249
Description of Changes
I had made a new navbar component, which is the same as the previous one. Even though I had removed the home icon because it was of no use. I had attached the navbar to every page, which will give us the power to reuse it multiple times.
Type of Change
Testing
Describe the tests you ran to verify your changes.
📸 Screenshots / Video
Recording.2026-02-20.145657.1.mp4
✅ Checklist
💬 Additional Notes (Optional)
Add any context, dependencies, or questions for reviewers.