Skip to content

Refactor GitHub link handling in Header component for improved structure and mobile responsiveness #293

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.

Sign up for GitHub

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

Closed
sarans-h wants to merge 3 commits into from
Closed

Refactor GitHub link handling in Header component for improved structure and mobile responsiveness #293

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a54b5c8
Refactor GitHub link handling in Header component for improved struct…
sarans-h Oct 7, 2025
3a1c4de
Merge branch 'devvsakib:main' into issue-268
sarans-h Oct 8, 2025
ffea295
Enhance Header component with mobile menu functionality and dynamic s…
sarans-h Oct 8, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
96 changes: 58 additions & 38 deletions src/components/Header/Header.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,14 +46,15 @@ function Header({notice }) {
link: "/resources",
icon: <MdStore size="1.2rem" />,
},
{
name: "Github",
link: "https://github.com/devvsakib/github-error-solve",
icon: <AiFillGithub size="1.2rem" />,
isExternalURL: true,
},
];

const githubLink = {
name: "Github",
link: "https://github.com/devvsakib/github-error-solve",
icon: <AiFillGithub size="1.2rem" />,
isExternalURL: true,
};

return (
<header className="p-4 shadow-lg backdrop-blur-sm z-50">
<div className="w-full md:w-5/6 mx-auto flex flex-row md:flex-row justify-between items-center">
Expand All @@ -66,12 +67,32 @@ function Header({notice }) {
/>
</Link>

{/* Menu icon */}
<div
onClick={() => setOpen((val) => !val)}
className="cursor-pointer md:hidden"
>
{open ? <MdClose size="1.2rem" /> : <MdMenu size="1.2rem" />}
{/* GitHub, theme toggle and menu icon for mobile */}
<div className="flex items-center gap-3 md:hidden">
<a target="_blank" href={githubLink.link}>
<div className="bg-blue-600/50 shadow font-semibold flex gap-1 p-1 px-2 items-center rounded-full text-sm">
<span className="githubBtn">
{githubLink.icon}
</span>
{
countStar && (
<div className="flex items-center gap-1">
{countStar}
</div>
)
}
</div>
</a>
Comment on lines +105 to +118
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Oct 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Add rel="noopener noreferrer" to external link.

Opening links with target="_blank" without rel="noopener noreferrer" exposes users to potential security risks (tabnabbing attacks) and performance issues. The opened page can access window.opener and potentially redirect the original page to a malicious site.

Apply this diff to fix the security issue:

-<a target="_blank" href={githubLink.link}>
+<a target="_blank" rel="noopener noreferrer" href={githubLink.link}>
📝 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
<a target="_blank" href={githubLink.link}>
<div className="bg-blue-600/50 shadow font-semibold flex gap-1 p-1 px-2 items-center rounded-full text-sm">
<span className="githubBtn">
{githubLink.icon}
</span>
{
countStar && (
<div className="flex items-center gap-1">
{countStar}
</div>
)
}
</div>
</a>
<a target="_blank" rel="noopener noreferrer" href={githubLink.link}>
<div className="bg-blue-600/50 shadow font-semibold flex gap-1 p-1 px-2 items-center rounded-full text-sm">
<span className="githubBtn">
{githubLink.icon}
</span>
{
countStar && (
<div className="flex items-center gap-1">
{countStar}
</div>
)
}
</div>
</a>
🤖 Prompt for AI Agents 
In src/components/Header/Header.jsx around lines 72 to 85, the external anchor
uses target="_blank" without rel="noopener noreferrer"; update the <a> element
to include rel="noopener noreferrer" (i.e., set rel to "noopener noreferrer") so
the opened page cannot access window.opener and to mitigate tabnabbing and
performance/security issues.

⚠️ Potential issue | 🟡 Minor

Add accessible label to GitHub link.

The GitHub link lacks an aria-label, which impacts screen reader users who may not understand the purpose of the link from the icon alone.

Apply this diff to improve accessibility:

-<a target="_blank" href={githubLink.link}>
+<a target="_blank" rel="noopener noreferrer" href={githubLink.link} aria-label="Star us on GitHub">
📝 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
<a target="_blank" href={githubLink.link}>
<div className="bg-blue-600/50 shadow font-semibold flex gap-1 p-1 px-2 items-center rounded-full text-sm">
<span className="githubBtn">
{githubLink.icon}
</span>
{
countStar && (
<div className="flex items-center gap-1">
{countStar}
</div>
)
}
</div>
</a>
<a target="_blank" rel="noopener noreferrer" href={githubLink.link} aria-label="Star us on GitHub">
<div className="bg-blue-600/50 shadow font-semibold flex gap-1 p-1 px-2 items-center rounded-full text-sm">
<span className="githubBtn">
{githubLink.icon}
</span>
{
countStar && (
<div className="flex items-center gap-1">
{countStar}
</div>
)
}
</div>
</a>
🤖 Prompt for AI Agents 
In src/components/Header/Header.jsx around lines 72 to 85, the anchor for the
GitHub link lacks an aria-label which makes it inaccessible to screen readers;
add an aria-label attribute to the <a> element with a meaningful descriptive
string (e.g., use githubLink.label if available or fallback to "View on GitHub")
so screen readers can convey the link purpose, and while updating the anchor
also include rel="noopener noreferrer" when using target="_blank" for security.

<div className="text-lg cursor-pointer" onClick={toggleTheme}>
<HiMoon className="dark:hidden" />
<HiSun className="hidden dark:inline" />
</div>
Comment on lines +119 to +122
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Oct 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Add accessible label to theme toggle button.

The theme toggle lacks semantic button markup and an aria-label, making it difficult for screen reader users to understand its purpose.

Apply this diff to improve accessibility:

-<div className="text-lg cursor-pointer" onClick={toggleTheme}>
+<button className="text-lg cursor-pointer bg-transparent border-0" onClick={toggleTheme} aria-label="Toggle theme">
   <HiMoon className="dark:hidden" />
   <HiSun className="hidden dark:inline" />
-</div>
+</button>
📝 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
<div className="text-lg cursor-pointer" onClick={toggleTheme}>
<HiMoon className="dark:hidden" />
<HiSun className="hidden dark:inline" />
</div>
<button className="text-lg cursor-pointer bg-transparent border-0" onClick={toggleTheme} aria-label="Toggle theme">
<HiMoon className="dark:hidden" />
<HiSun className="hidden dark:inline" />
</button>
🤖 Prompt for AI Agents 
In src/components/Header/Header.jsx around lines 86 to 89, the theme toggle is a
non-semantic div without an aria-label; replace it with a <button type="button">
preserving the className and onClick (toggleTheme), add an appropriate
aria-label (preferably dynamic: "Switch to dark mode" or "Switch to light mode"
based on current theme state) and consider adding aria-pressed or visually
hidden text if you want screen readers to know the current state; keep the
HiMoon and HiSun icons as children so keyboard and screen-reader users can
activate the control.

<div
onClick={() => setOpen((val) => !val)}
className="cursor-pointer"
>
{open ? <MdClose size="1.2rem" /> : <MdMenu size="1.2rem" />}
</div>
Comment on lines +123 to +128
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Oct 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Add accessible label to menu toggle button.

The menu toggle lacks semantic button markup and an aria-label. Additionally, it should use aria-expanded to communicate the menu's open/closed state to assistive technologies.

Apply this diff to improve accessibility:

-<div
-  onClick={() => setOpen((val) => !val)}
-  className="cursor-pointer"
->
+<button
+  onClick={() => setOpen((val) => !val)}
+  className="cursor-pointer bg-transparent border-0"
+  aria-label="Toggle menu"
+  aria-expanded={open}
+>
   {open ? <MdClose size="1.2rem" /> : <MdMenu size="1.2rem" />}
-</div>
+</button>
📝 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
<div
onClick={() => setOpen((val) => !val)}
className="cursor-pointer"
>
{open ? <MdClose size="1.2rem" /> : <MdMenu size="1.2rem" />}
</div>
<button
onClick={() => setOpen((val) => !val)}
className="cursor-pointer bg-transparent border-0"
aria-label="Toggle menu"
aria-expanded={open}
>
{open ? <MdClose size="1.2rem" /> : <MdMenu size="1.2rem" />}
</button>
🤖 Prompt for AI Agents 
In src/components/Header/Header.jsx around lines 90 to 95, the menu toggle is a
non-semantic div without ARIA attributes; replace the div with a semantic
<button> (type="button") that preserves the onClick and className, add an
appropriate aria-label (e.g., "Toggle menu"), and include aria-expanded={open}
to reflect the open/closed state so assistive tech can announce it; keep the
same icons and sizing and ensure keyboard activation works naturally via the
button element.

</div>

{/* Nav link items */}
Expand All @@ -88,34 +109,33 @@ function Header({notice }) {
${open ? (theme === "dark" ? "bg-dark/90" : "bg-white/90") : ""}
${open ? "top-14" : "top-[-490px]"}`}
>
{navLink.map((link, index) => {
return (
<div key={`${link.name}-${index}`}>
{link?.isExternalURL ? (
<a target="_blank" href={link.link}>
<div className="bg-blue-600/50 shadow font-semibold flex gap-1 p-1 px-2 items-center rounded-full">
<span className="githubBtn">
{link.icon}
</span>
{
countStar && (
<div className="flex items-center gap-1">
{countStar}
</div>
)
}
{navLink.map((link, index) => (
<div key={`${link.name}-${index}`}>
<Link className="flex items-center gap-1" to={link.link}>
{/* {link.icon} */}
{link.name}
</Link>
</div>
))}
{/* GitHub link for desktop */}
<div className="hidden md:block">
<a target="_blank" href={githubLink.link}>
<div className="bg-blue-600/50 shadow font-semibold flex gap-1 p-1 px-2 items-center rounded-full">
<span className="githubBtn">
{githubLink.icon}
</span>
{
countStar && (
<div className="flex items-center gap-1">
{countStar}
</div>
</a>
) : (
<Link className="flex items-center gap-1" to={link.link}>
{/* {link.icon} */}
{link.name}
</Link>
)}
)
}
</div>
);
})}
<div className="text-lg cursor-pointer" onClick={toggleTheme}>
</a>
Comment on lines +151 to +158
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Oct 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Add rel="noopener noreferrer" to external link.

Same security issue as the mobile GitHub link (Lines 72-85). Opening links with target="_blank" without rel="noopener noreferrer" exposes users to potential security risks.

Apply this diff to fix the security issue:

-<a target="_blank" href={githubLink.link}>
+<a target="_blank" rel="noopener noreferrer" href={githubLink.link} aria-label="Star us on GitHub">
🤖 Prompt for AI Agents 
In src/components/Header/Header.jsx around lines 122 to 135, the anchor uses
target="_blank" without rel attributes which exposes the app to reverse
tabnabbing; update the anchor to include rel="noopener noreferrer" (i.e., add
rel="noopener noreferrer" to the <a> tag) so external links opened in new tabs
are safe.

</div>
{/* Theme toggle for desktop */}
<div className="text-lg cursor-pointer hidden md:block" onClick={toggleTheme}>
<HiMoon className="dark:hidden" />
<HiSun className="hidden dark:inline" />
</div>
Comment on lines +162 to 165
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Oct 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Add accessible label to theme toggle button.

Same accessibility issue as the mobile theme toggle (Lines 86-89). The theme toggle should use semantic button markup with an aria-label.

Apply this diff to improve accessibility:

-<div className="text-lg cursor-pointer hidden md:block" onClick={toggleTheme}>
+<button className="text-lg cursor-pointer bg-transparent border-0 hidden md:block" onClick={toggleTheme} aria-label="Toggle theme">
   <HiMoon className="dark:hidden" />
   <HiSun className="hidden dark:inline" />
-</div>
+</button>
📝 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
<div className="text-lg cursor-pointer hidden md:block" onClick={toggleTheme}>
<HiMoon className="dark:hidden" />
<HiSun className="hidden dark:inline" />
</div>
<button
className="text-lg cursor-pointer bg-transparent border-0 hidden md:block"
onClick={toggleTheme}
aria-label="Toggle theme"
>
<HiMoon className="dark:hidden" />
<HiSun className="hidden dark:inline" />
</button>
🤖 Prompt for AI Agents 
In src/components/Header/Header.jsx around lines 138 to 141, the theme toggle is
a non-semantic div without an accessible name; change it to a <button> element
(preserve the existing classes and onClick handler), add type="button" and an
appropriate aria-label (e.g., "Toggle theme" or "Switch to dark/light mode"),
and mark the decorative icon elements as aria-hidden="true" so screen readers
don't announce them; ensure keyboard focus styles remain and functionality is
unchanged.

Expand Down