fix: resolved the navbar item link problem that leads to its respecti…#23
Open
akashmeapen wants to merge 1 commit intoAOSSIE-Org:mainfrom
Open
fix: resolved the navbar item link problem that leads to its respecti…#23akashmeapen wants to merge 1 commit intoAOSSIE-Org:mainfrom
akashmeapen wants to merge 1 commit intoAOSSIE-Org:mainfrom
Conversation
WalkthroughThe PR adds anchor-based navigation to the website by introducing section identifiers (id attributes) to key page sections and implementing smooth scroll-to-section functionality in the navbar. This enables users to navigate to Features, About, and Download sections within the page, and provides a scrollToTop function for returning to the top. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment Tip CodeRabbit can use oxc to improve the quality of JavaScript and TypeScript code reviews.Add a configuration file to your project to customize how CodeRabbit runs oxc. |
github-actions
bot
added
bug
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Pages/Landing` page/Navbar.tsx:
- Around line 153-156: The Navbar currently hardcodes user-visible labels in
NavLink components (e.g., the "Home", "Features", "About", "Download" strings
and similar labels later in the file at the other noted ranges); replace these
literals with i18n resource lookups (use the existing i18n pattern in the
project, e.g., obtain a t function via useTranslation or the app's i18n helper)
and reference new keys like navbar.home, navbar.features, navbar.about,
navbar.download; add corresponding entries to the i18n resource files and update
all NavLink occurrences and any other hardcoded labels in this component
(including the other ranges called out) to use t('...') instead of string
literals so localization is consistent.
- Around line 197-202: The mobile menu container currently only translates
offscreen (uses isOpen) which leaves links focusable; update the menu container
(the <div> using isOpen in Navbar) so that when isOpen is false it also sets
aria-hidden="true" and hidden (or applies the inert attribute/polyfill) to
remove it from accessibility tree, and when isOpen is true remove
aria-hidden/hidden and restore focusability; additionally ensure any focusable
children get tabIndex={isOpen ? 0 : -1} (or use an inert polyfill) so closed
menus are not keyboard-focusable; apply the same changes to the other
mobile-menu block referenced (the second menu block around the other menu
markup).
- Around line 212-217: The mobile close button currently uses an onClick handler
(onClick={() => setIsOpen(false)}) but lacks an explicit type, so add
type="button" to that <button> in the Navbar component to prevent it from
defaulting to submit when rendered inside a form; locate the button that
contains the class string with "text-gray-800 dark:text-gray-300" and the
onClick calling setIsOpen(false) and add the type attribute there.
- Around line 204-211: The Home logo Link ("PictoPy") doesn't close the mobile
nav when clicked; add an onClick handler to the Link that closes the mobile menu
(e.g., onClick={() => setIsOpen(false)} or call the existing closeMobileMenu() /
setMobileMenuOpen(false) function used elsewhere) so clicking the "PictoPy" Link
also collapses the mobile menu.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 099885f4-d966-427a-8692-320068b31676
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (3)
src/Pages/HowItWorks/HowItWorks.tsxsrc/Pages/Landing page/Navbar.tsxsrc/components/ui/Bouncy Card Features.tsx
Comment on lines
+153
to
+156
| <NavLink to="/" onClick={scrollToTop}>Home</NavLink> | ||
| <NavLink to="#features" isScrollLink={true}>Features</NavLink> | ||
| <NavLink to="#about" isScrollLink={true}>About</NavLink> | ||
| <NavLink to="#downloads-section" isScrollLink={true}>Download</NavLink> |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Externalize newly added navbar labels into i18n resources.
The added user-visible strings are hardcoded in JSX; move them to resource keys to keep localization consistent.
As per coding guidelines, Internationalization: User-visible strings should be externalized to resource files (i18n).
Also applies to: 206-210, 224-238
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Pages/Landing` page/Navbar.tsx around lines 153 - 156, The Navbar
currently hardcodes user-visible labels in NavLink components (e.g., the "Home",
"Features", "About", "Download" strings and similar labels later in the file at
the other noted ranges); replace these literals with i18n resource lookups (use
the existing i18n pattern in the project, e.g., obtain a t function via
useTranslation or the app's i18n helper) and reference new keys like
navbar.home, navbar.features, navbar.about, navbar.download; add corresponding
entries to the i18n resource files and update all NavLink occurrences and any
other hardcoded labels in this component (including the other ranges called out)
to use t('...') instead of string literals so localization is consistent.
Comment on lines
+197
to
+202
| <div | ||
| className={`md:hidden fixed inset-0 z-[60] | ||
| bg-white dark:bg-black | ||
| transform transition-transform duration-300 ease-in-out | ||
| ${isOpen ? "translate-x-0" : "-translate-x-full"}`} | ||
| > | ||
| <div className="pt-16 pb-6 px-4 space-y-6"> | ||
| <div className="space-y-4 flex flex-col items-start"> | ||
| <NavLink to="/" onClick={() => setIsOpen(false)}> | ||
| Home | ||
| </NavLink> | ||
| <NavLink to="#features" isScrollLink={true} onClick={() => setIsOpen(false)}> | ||
| Feature | ||
| </NavLink> | ||
| <NavLink to="#about" isScrollLink={true} onClick={() => setIsOpen(false)}> | ||
| About | ||
| </NavLink> | ||
| <Button | ||
| className="w-full bg-gray-800 dark:bg-black | ||
| text-white | ||
| hover:bg-green-700 dark:hover:bg-green-800 | ||
| transition-colors duration-300 mt-4" | ||
| onClick={() => setIsOpen(false)} | ||
| > | ||
| Download | ||
| </Button> | ||
| </div> | ||
| > |
There was a problem hiding this comment.
Hidden mobile menu remains focusable when closed.
Using only translate-offscreen keeps interactive links in the DOM flow for accessibility tools/keyboard navigation. Make the closed state non-interactive and hidden.
Suggested fix
- <div
- className={`md:hidden fixed inset-0 z-[60]
+ <div
+ aria-hidden={!isOpen}
+ className={`md:hidden fixed inset-0 z-[60]
bg-white dark:bg-black
transform transition-transform duration-300 ease-in-out
- ${isOpen ? "translate-x-0" : "-translate-x-full"}`}
+ ${isOpen ? "translate-x-0 visible" : "-translate-x-full invisible pointer-events-none"}`}
>Also applies to: 222-238
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Pages/Landing` page/Navbar.tsx around lines 197 - 202, The mobile menu
container currently only translates offscreen (uses isOpen) which leaves links
focusable; update the menu container (the <div> using isOpen in Navbar) so that
when isOpen is false it also sets aria-hidden="true" and hidden (or applies the
inert attribute/polyfill) to remove it from accessibility tree, and when isOpen
is true remove aria-hidden/hidden and restore focusability; additionally ensure
any focusable children get tabIndex={isOpen ? 0 : -1} (or use an inert polyfill)
so closed menus are not keyboard-focusable; apply the same changes to the other
mobile-menu block referenced (the second menu block around the other menu
markup).
Comment on lines
+204
to
+211
| <Link | ||
| to="/" | ||
| className="text-2xl font-bold | ||
| bg-gradient-to-r from-yellow-500 to-green-500 | ||
| bg-clip-text text-transparent" | ||
| > | ||
| PictoPy | ||
| </Link> |
There was a problem hiding this comment.
Home logo click should also close the mobile menu.
Currently the menu state stays open after clicking the logo link.
Suggested fix
<Link
to="/"
+ onClick={() => {
+ scrollToTop();
+ setIsOpen(false);
+ }}
className="text-2xl font-bold
bg-gradient-to-r from-yellow-500 to-green-500
bg-clip-text text-transparent"
>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Pages/Landing` page/Navbar.tsx around lines 204 - 211, The Home logo Link
("PictoPy") doesn't close the mobile nav when clicked; add an onClick handler to
the Link that closes the mobile menu (e.g., onClick={() => setIsOpen(false)} or
call the existing closeMobileMenu() / setMobileMenuOpen(false) function used
elsewhere) so clicking the "PictoPy" Link also collapses the mobile menu.
Comment on lines
+212
to
+217
| <button | ||
| onClick={() => setIsOpen(false)} | ||
| className="text-gray-800 dark:text-gray-300 | ||
| hover:text-black dark:hover:text-white | ||
| transition-colors duration-300" | ||
| > |
There was a problem hiding this comment.
Add explicit button type on the mobile close button.
Without type="button", it defaults to submit, which is fragile if this component is ever rendered inside a form.
Suggested fix
- <button
+ <button type="button"
onClick={() => setIsOpen(false)}
className="text-gray-800 dark:text-gray-300
hover:text-black dark:hover:text-white
transition-colors duration-300"
>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <button | |
| onClick={() => setIsOpen(false)} | |
| className="text-gray-800 dark:text-gray-300 | |
| hover:text-black dark:hover:text-white | |
| transition-colors duration-300" | |
| > | |
| <button type="button" | |
| onClick={() => setIsOpen(false)} | |
| className="text-gray-800 dark:text-gray-300 | |
| hover:text-black dark:hover:text-white | |
| transition-colors duration-300" | |
| > |
🧰 Tools
🪛 Biome (2.4.6)
[error] 212-217: Provide an explicit type prop for the button element.
(lint/a11y/useButtonType)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Pages/Landing` page/Navbar.tsx around lines 212 - 217, The mobile close
button currently uses an onClick handler (onClick={() => setIsOpen(false)}) but
lacks an explicit type, so add type="button" to that <button> in the Navbar
component to prevent it from defaulting to submit when rendered inside a form;
locate the button that contains the class string with "text-gray-800
dark:text-gray-300" and the onClick calling setIsOpen(false) and add the type
attribute there.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
##Description
Addressed Issues:
Closes #22
Testing
npm installandnpm run devChecklist
We encourage contributors to use AI tools responsibly when creating Pull Requests. While AI can be a valuable aid, it is essential to ensure that your contributions meet the task requirements, build successfully, include relevant tests, and pass all linters. Submissions that do not meet these standards may be closed without warning to maintain the quality and integrity of the project. Please take the time to understand the changes you are proposing and their impact.
Summary by CodeRabbit