fix: resolve <a> inside <a> nesting issue in MobileNavMenu #3775
Conversation
KishanInnovates
requested review from
Mayaleeeee,
akshatnema,
anshgoyalevil,
asyncapi-bot-eve and
derberg
as code owners
|
Caution Review failedThe pull request is closed. WalkthroughThe changes in this pull request simplify the structure of the mobile navigation menu. In Changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
✅ Deploy Preview for asyncapi-website ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site configuration. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
components/navigation/MobileNavMenu.tsx (4)
88-90: Remove Nested<a>Tags for Docs Link
The revised Docs link now directly uses the<Link>component with thecursor-pointerclass, effectively eliminating the invalid nesting of<a>tags. This improves both the semantic structure and accessibility of the code. Additionally, please check for any extra spaces as flagged by ESLint and adjust accordingly.🧰 Tools
🪛 ESLint
[error] 88-88: Delete
··(prettier/prettier)
[error] 89-89: Delete
··(prettier/prettier)
[error] 90-90: Delete
··(prettier/prettier)
98-100: Simplify Tools Link Structure
The Tools section now uses<Link>with thecursor-pointerclass instead of wrapping it with an<a>tag. This change ensures valid HTML structure and a more streamlined component markup. Ensure that associated whitespace adheres to the ESLint/prettier guidelines.🧰 Tools
🪛 ESLint
[error] 99-99: Delete
··(prettier/prettier)
107-109: Eliminate Redundant<a>Tag in Community Link
By replacing the outer<a>container with a<Link>component directly, the Community link now avoids invalid nested anchors. This change aligns with best practices for accessibility and markup validity. Please verify that the styling (e.g., spacing) remains consistent and consider running a formatter to resolve the minor ESLint whitespace issues.🧰 Tools
🪛 ESLint
[error] 107-107: Delete
··(prettier/prettier)
[error] 108-108: Delete
··(prettier/prettier)
[error] 109-109: Replace
··</Link>··with</Link>(prettier/prettier)
[error] 109-109: Trailing spaces not allowed.
(no-trailing-spaces)
117-120: Consider Consistent Navigation Element for 'Others' Section
While the PR’s primary objective was to fix nested<a>tags, note that the 'Others' section still uses a plain<a>element (<a className='cursor-pointer'>Others</a>). If this element is intended to navigate or toggle submenus similar to the other sections, it might be worth considering a refactor to use the<Link>component for consistency.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
components/navigation/MobileNavMenu.tsx(1 hunks)
🧰 Additional context used
🪛 ESLint
components/navigation/MobileNavMenu.tsx
[error] 88-88: Delete ··
(prettier/prettier)
[error] 89-89: Delete ··
(prettier/prettier)
[error] 90-90: Delete ··
(prettier/prettier)
[error] 99-99: Delete ··
(prettier/prettier)
[error] 107-107: Delete ··
(prettier/prettier)
[error] 108-108: Delete ··
(prettier/prettier)
[error] 109-109: Replace ··</Link>·· with </Link>
(prettier/prettier)
[error] 109-109: Trailing spaces not allowed.
(no-trailing-spaces)
⏰ Context from checks skipped due to timeout of 180000ms (4)
- GitHub Check: Redirect rules - asyncapi-website
- GitHub Check: Header rules - asyncapi-website
- GitHub Check: Pages changed - asyncapi-website
- GitHub Check: Lighthouse CI
KishanInnovates
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
components/navigation/MobileNavMenu.tsx (5)
88-90: Great fix for the nested<a>tags issue!The removal of the unnecessary
<a>tag around the Link component is correct, as Next.js Link components already render as<a>elements in the DOM. This change helps maintain valid HTML structure and improves accessibility.However, there are some formatting issues flagged by the pipeline:
- Extra spaces at the beginning of lines 88-90
- Invalid Tailwind CSS classnames order on line 88 (should be
flex cursor-pointerinstead ofcursor-pointer flex)- <Link href='/docs' className='cursor-pointer flex'> + <Link href='/docs' className='flex cursor-pointer'>🧰 Tools
🪛 ESLint
[error] 88-88: Delete
··(prettier/prettier)
[error] 89-89: Delete
··(prettier/prettier)
[error] 90-90: Delete
··(prettier/prettier)
🪛 GitHub Actions: PR testing - if Node project
[error] 88-88: Delete
··prettier/prettier
[warning] 88-88: Invalid Tailwind CSS classnames order tailwindcss/classnames-order
[error] 89-89: Delete
··prettier/prettier
[error] 90-90: Delete
··prettier/prettier
98-100: Good fix for nested interactive elements!Removing the
<a>tag wrapper around the Link component fixes the HTML validity issue.But please fix the formatting issues:
- Extra spaces at the beginning of line 99
- Invalid Tailwind CSS classnames order on line 98 (should be
flex cursor-pointerinstead ofcursor-pointer flex)- <Link href='/tools' className='cursor-pointer flex'> - Tools - </Link> + <Link href='/tools' className='flex cursor-pointer'> + Tools + </Link>🧰 Tools
🪛 ESLint
[error] 99-99: Delete
··(prettier/prettier)
🪛 GitHub Actions: PR testing - if Node project
[warning] 98-98: Invalid Tailwind CSS classnames order tailwindcss/classnames-order
[error] 99-99: Delete
··prettier/prettier
107-109: Clean removal of nested anchor tag!This change correctly addresses the issue by removing the wrapping
<a>tag.Please fix these formatting issues:
- Extra spaces at the beginning of lines 107-108
- Trailing spaces on line 109
- Invalid Tailwind CSS classnames order on line 107 (should be
flex cursor-pointerinstead ofcursor-pointer flex)- <Link href='/community' className='cursor-pointer flex'> - Community - </Link> + <Link href='/community' className='flex cursor-pointer'> + Community + </Link>🧰 Tools
🪛 ESLint
[error] 107-107: Delete
··(prettier/prettier)
[error] 108-108: Delete
··(prettier/prettier)
[error] 109-109: Replace
··</Link>··with</Link>(prettier/prettier)
[error] 109-109: Trailing spaces not allowed.
(no-trailing-spaces)
🪛 GitHub Actions: PR testing - if Node project
[error] 107-107: Delete
··prettier/prettier
[warning] 107-107: Invalid Tailwind CSS classnames order tailwindcss/classnames-order
[error] 108-108: Delete
··prettier/prettier
[error] 109-109: Replace
··</Link>··with</Link>prettier/prettier
[error] 109-109: Trailing spaces not allowed. no-trailing-spaces
118-118: Consider updating the "Others" section for consistencyWhile you've fixed the nested
<a>issues in the Docs, Tools, and Community sections, consider applying the same pattern to the "Others" section where there's still a standalone<a>tag.Since this section also links to items (when expanded), you might want to replace this with a Link component like the other sections for consistency, or just remove the cursor-pointer class if it's not actually clickable.
141-143: Consider updating the "Language" section for consistencySimilar to the "Others" section, the "Language" section uses a standalone
<a>tag. For consistency across the mobile navigation menu, consider handling this section in the same way as the other sections.Since the language dropdown is controlled by the parent div's onClick event rather than the anchor tag's behavior, it would be clearer to just use a
<span>or<div>instead of an<a>tag here.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
components/navigation/MobileNavMenu.tsx(1 hunks)
🧰 Additional context used
🪛 ESLint
components/navigation/MobileNavMenu.tsx
[error] 88-88: Delete ··
(prettier/prettier)
[error] 89-89: Delete ··
(prettier/prettier)
[error] 90-90: Delete ··
(prettier/prettier)
[error] 99-99: Delete ··
(prettier/prettier)
[error] 107-107: Delete ··
(prettier/prettier)
[error] 108-108: Delete ··
(prettier/prettier)
[error] 109-109: Replace ··</Link>·· with </Link>
(prettier/prettier)
[error] 109-109: Trailing spaces not allowed.
(no-trailing-spaces)
🪛 GitHub Actions: PR testing - if Node project
components/navigation/MobileNavMenu.tsx
[error] 88-88: Delete ·· prettier/prettier
[warning] 88-88: Invalid Tailwind CSS classnames order tailwindcss/classnames-order
[error] 89-89: Delete ·· prettier/prettier
[error] 90-90: Delete ·· prettier/prettier
[warning] 98-98: Invalid Tailwind CSS classnames order tailwindcss/classnames-order
[error] 99-99: Delete ·· prettier/prettier
[error] 107-107: Delete ·· prettier/prettier
[warning] 107-107: Invalid Tailwind CSS classnames order tailwindcss/classnames-order
[error] 108-108: Delete ·· prettier/prettier
[error] 109-109: Replace ··</Link>·· with </Link> prettier/prettier
[error] 109-109: Trailing spaces not allowed. no-trailing-spaces
⏰ Context from checks skipped due to timeout of 180000ms (3)
- GitHub Check: Redirect rules - asyncapi-website
- GitHub Check: Header rules - asyncapi-website
- GitHub Check: Pages changed - asyncapi-website
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3775 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 21 21
Lines 667 667
Branches 113 113
=========================================
Hits 667 667 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
/rtm |
3 participants
fix: prevent nested
<a>tags in MobileNavMenuIssue Reference
Closes #3766
Changes Made
<a>tags insideLinkcomponents.How to Test
Summary by CodeRabbit
Summary by CodeRabbit