Fix/button props and type - #71
Open
raj-aryan-official wants to merge 14 commits into
Open
Conversation
…JSDoc to components for doc coverage
📝 WalkthroughWalkthroughThis PR introduces comprehensive light/dark theme support across the landing page while enhancing component accessibility. Changes include adding a new MIT License file, implementing a Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/app/layout.tsx (1)
47-58:⚠️ Potential issue | 🟠 MajorDark mode will still flash on first paint.
<body>defaults to light styles, butThemeToggleonly adds thedarkclass after hydration. Users with a saved or system dark preference will see a light-theme flash before the page becomes interactive. Add an inline script in the root layout that runs before React hydrates to seed the theme class on<html>based onlocalStorageorprefers-color-scheme.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/layout.tsx` around lines 47 - 58, Add a small synchronous inline script in the root layout (in src/app/layout.tsx near the <html> element, before the <body>) that seeds the theme class on the documentElement so dark-mode doesn't flash: implement logic to read localStorage.getItem('theme') (e.g. 'dark'|'light') and, if absent, use window.matchMedia('(prefers-color-scheme: dark)').matches; then immediately add or remove the 'dark' class on document.documentElement accordingly. Insert it as a raw script node (using dangerouslySetInnerHTML) so it runs before React hydration and keep your existing twMerge usage (inter.variable, fraunces.variable, caudex.variable) intact; this ensures ThemeToggle can hydrate without causing a flash of the wrong theme.
🧹 Nitpick comments (1)
src/sections/LogoTicker.tsx (1)
37-55: Deduplicate the ticker item markup.The two
mapblocks render the same element tree. Building the loop from a singleitems.concat(items)array or extracting a tiny render helper will keep future logo/text class changes from drifting.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/sections/LogoTicker.tsx` around lines 37 - 55, The markup for each ticker entry is duplicated in two map blocks; consolidate by mapping over a doubled array (e.g., items.concat(items)) or extracting a small render helper (e.g., renderTickerItem(item, index, repeatFlag)) and use that single mapper to produce the same <div> structure with LogoComp and label; update the key logic to remain unique (like `${index}` for first pass and `${index}-repeat` for the second) while keeping the same class names and props on LogoComp so future style changes only need one edit.
🤖 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/components/ThemeToggle.tsx`:
- Around line 12-36: The ThemeToggle component currently keeps theme in local
state causing desync when multiple instances mount; lift the single source of
truth into shared state (e.g., a ThemeContext or a top-level provider) and have
ThemeToggle read theme and call setTheme from that context instead of using its
own useState. Initialize the shared provider once (on app/root mount) to read
localStorage ('theme'), apply/remove the 'dark' class on
document.documentElement, and persist changes to localStorage; update
ThemeToggle to use the provider's theme and the existing toggleTheme/setTheme
function (replace local setTheme, mounted logic and the effect that writes to
document.documentElement/localStorage). Ensure both Header-mounted ThemeToggle
instances consume the same context so icons/knobs stay in sync.
- Around line 46-54: The <ThemeToggle> button currently lacks explicit type and
ARIA state; update the <button> element used with onClick={toggleTheme} to
include type="button" and aria-pressed={isDark} (or aria-pressed={!!isDark}) so
it won't implicitly submit forms and assistive tech can read the current
dark-mode state; keep the existing aria-label and other props unchanged.
In `@src/sections/Footer.tsx`:
- Around line 50-58: The YouTube anchor in the Footer component uses a
placeholder href ("https://www.youtube.com"); locate the anchor element that
contains aria-label="Djed Alliance on YouTube" (the one rendering <YTSocial
.../>) and replace the href value with the actual Djed Alliance YouTube channel
URL (ensuring target="_blank" and rel="noreferrer" are preserved) before
release.
In `@src/sections/Header.tsx`:
- Around line 40-46: The mobile menu trigger/button (onClick={() =>
setOpen(true)} with MenuIcon) and corresponding drawer must be treated as a
modal dialog: add aria-expanded and aria-controls to the trigger (toggle with
setOpen), give the drawer dialog semantics (role="dialog" and aria-modal="true"
and an id referenced by aria-controls), trap keyboard focus inside the drawer
and set initial focus to the first focusable element when it opens, restore
focus to the trigger when it closes, handle Escape to close (listen for Escape
and call setOpen(false)), and hide/inert background content (or set aria-hidden
on background regions) while open so screen readers and keyboard users cannot
reach behind the modal. Ensure these behaviors are applied to the same mobile
drawer used in the later block (lines around 52-73).
- Around line 29-33: Desktop nav anchors use placeholder href="#" while the
drawer uses real routes; update the desktop anchor hrefs to the correct paths
(e.g., "/docs" and "/integration") and refactor both desktop and drawer
navigation to consume a single shared links array (e.g., navItems or NAV_LINKS)
so targets remain consistent; locate the anchors in Header.tsx (the desktop
links around "Developers Guide" and "Integration Docs" and the drawer link
rendering) and replace static hrefs with link.href from the shared data, mapping
that array in both places.
---
Outside diff comments:
In `@src/app/layout.tsx`:
- Around line 47-58: Add a small synchronous inline script in the root layout
(in src/app/layout.tsx near the <html> element, before the <body>) that seeds
the theme class on the documentElement so dark-mode doesn't flash: implement
logic to read localStorage.getItem('theme') (e.g. 'dark'|'light') and, if
absent, use window.matchMedia('(prefers-color-scheme: dark)').matches; then
immediately add or remove the 'dark' class on document.documentElement
accordingly. Insert it as a raw script node (using dangerouslySetInnerHTML) so
it runs before React hydration and keep your existing twMerge usage
(inter.variable, fraunces.variable, caudex.variable) intact; this ensures
ThemeToggle can hydrate without causing a flash of the wrong theme.
---
Nitpick comments:
In `@src/sections/LogoTicker.tsx`:
- Around line 37-55: The markup for each ticker entry is duplicated in two map
blocks; consolidate by mapping over a doubled array (e.g., items.concat(items))
or extracting a small render helper (e.g., renderTickerItem(item, index,
repeatFlag)) and use that single mapper to produce the same <div> structure with
LogoComp and label; update the key logic to remain unique (like `${index}` for
first pass and `${index}-repeat` for the second) while keeping the same class
names and props on LogoComp so future style changes only need one edit.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 00c002aa-ec97-4a93-ab5c-88b60a7f0a65
⛔ Files ignored due to path filters (5)
src/app/apple-touch-icon.svgis excluded by!**/*.svgsrc/app/favicon.svgis excluded by!**/*.svgsrc/assets/djed-alliance.svgis excluded by!**/*.svgsrc/assets/stability-nexus.svgis excluded by!**/*.svgsrc/assets/svg/logo.svgis excluded by!**/*.svg
📒 Files selected for processing (12)
License.mdsrc/app/layout.tsxsrc/app/page.tsxsrc/components/Button.tsxsrc/components/ThemeToggle.tsxsrc/sections/CallToAction.tsxsrc/sections/Features.tsxsrc/sections/Footer.tsxsrc/sections/Header.tsxsrc/sections/Hero.tsxsrc/sections/LogoTicker.tsxtailwind.config.ts
Comment on lines
+12
to
+36
| const [theme, setTheme] = useState<'light' | 'dark'>('dark'); | ||
| const [mounted, setMounted] = useState(false); | ||
|
|
||
| // Read initial theme and apply classes | ||
| useEffect(() => { | ||
| setMounted(true); | ||
| const storedTheme = localStorage.getItem('theme') as 'light' | 'dark' | null; | ||
| const initialTheme = storedTheme || (window.matchMedia('(prefers-color-scheme: dark)').matches ? 'dark' : 'light'); | ||
| setTheme(initialTheme); | ||
| }, []); | ||
|
|
||
| useEffect(() => { | ||
| if (!mounted) return; | ||
| const root = document.documentElement; | ||
| if (theme === 'dark') { | ||
| root.classList.add('dark'); | ||
| } else { | ||
| root.classList.remove('dark'); | ||
| } | ||
| localStorage.setItem('theme', theme); | ||
| }, [theme, mounted]); | ||
|
|
||
| const toggleTheme = () => { | ||
| setTheme(prev => (prev === 'dark' ? 'light' : 'dark')); | ||
| }; |
There was a problem hiding this comment.
Avoid storing the active theme in each toggle instance.
theme is component-local state and is only mirrored to document.documentElement/localStorage. src/sections/Header.tsx now mounts two ThemeToggles, so toggling the drawer copy leaves the header copy with stale icon/knob state. Move the theme source of truth into shared state and let each toggle render from that single value.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/ThemeToggle.tsx` around lines 12 - 36, The ThemeToggle
component currently keeps theme in local state causing desync when multiple
instances mount; lift the single source of truth into shared state (e.g., a
ThemeContext or a top-level provider) and have ThemeToggle read theme and call
setTheme from that context instead of using its own useState. Initialize the
shared provider once (on app/root mount) to read localStorage ('theme'),
apply/remove the 'dark' class on document.documentElement, and persist changes
to localStorage; update ThemeToggle to use the provider's theme and the existing
toggleTheme/setTheme function (replace local setTheme, mounted logic and the
effect that writes to document.documentElement/localStorage). Ensure both
Header-mounted ThemeToggle instances consume the same context so icons/knobs
stay in sync.
Comment on lines
+46
to
+54
| <button | ||
| onClick={toggleTheme} | ||
| className={`relative flex items-center w-[60px] h-[32px] rounded-full p-1 transition-colors duration-500 ease-in-out border border-white/10 | ||
| ${isDark ? 'bg-white/10 shadow-[inset_0px_0px_10px_rgba(255,255,255,0.1)]' : 'bg-black/5 shadow-[inset_0px_0px_10px_rgba(0,0,0,0.05)]'}`} | ||
| aria-label="Toggle Theme" | ||
| style={{ | ||
| WebkitTapHighlightColor: 'transparent', | ||
| }} | ||
| > |
There was a problem hiding this comment.
Expose this as a real toggle button.
The control is missing type="button" and aria-pressed, so it will submit if reused inside a form and assistive tech cannot tell whether dark mode is currently active.
🔧 Minimal fix
<button
+ type="button"
onClick={toggleTheme}
+ aria-pressed={isDark}
className={`relative flex items-center w-[60px] h-[32px] rounded-full p-1 transition-colors duration-500 ease-in-out border border-white/10
${isDark ? 'bg-white/10 shadow-[inset_0px_0px_10px_rgba(255,255,255,0.1)]' : 'bg-black/5 shadow-[inset_0px_0px_10px_rgba(0,0,0,0.05)]'}`}
- aria-label="Toggle Theme"
+ aria-label={isDark ? 'Switch to light theme' : 'Switch to dark theme'}
style={{
WebkitTapHighlightColor: 'transparent',
}}📝 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={toggleTheme} | |
| className={`relative flex items-center w-[60px] h-[32px] rounded-full p-1 transition-colors duration-500 ease-in-out border border-white/10 | |
| ${isDark ? 'bg-white/10 shadow-[inset_0px_0px_10px_rgba(255,255,255,0.1)]' : 'bg-black/5 shadow-[inset_0px_0px_10px_rgba(0,0,0,0.05)]'}`} | |
| aria-label="Toggle Theme" | |
| style={{ | |
| WebkitTapHighlightColor: 'transparent', | |
| }} | |
| > | |
| <button | |
| type="button" | |
| onClick={toggleTheme} | |
| aria-pressed={isDark} | |
| className={`relative flex items-center w-[60px] h-[32px] rounded-full p-1 transition-colors duration-500 ease-in-out border border-white/10 | |
| ${isDark ? 'bg-white/10 shadow-[inset_0px_0px_10px_rgba(255,255,255,0.1)]' : 'bg-black/5 shadow-[inset_0px_0px_10px_rgba(0,0,0,0.05)]'}`} | |
| aria-label={isDark ? 'Switch to light theme' : 'Switch to dark theme'} | |
| style={{ | |
| WebkitTapHighlightColor: 'transparent', | |
| }} | |
| > |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/ThemeToggle.tsx` around lines 46 - 54, The <ThemeToggle>
button currently lacks explicit type and ARIA state; update the <button> element
used with onClick={toggleTheme} to include type="button" and
aria-pressed={isDark} (or aria-pressed={!!isDark}) so it won't implicitly submit
forms and assistive tech can read the current dark-mode state; keep the existing
aria-label and other props unchanged.
Comment on lines
+50
to
+58
| <a | ||
| href="https://www.youtube.com" | ||
| target="_blank" | ||
| rel="noreferrer" | ||
| aria-label="Djed Alliance on YouTube" | ||
| className="text-black/40 hover:text-black dark:text-white/40 dark:hover:text-white transition-colors duration-300" | ||
| > | ||
| <YTSocial className="w-5 h-5 sm:w-6 sm:h-6" /> | ||
| </a> |
There was a problem hiding this comment.
YouTube URL appears to be a placeholder.
The YouTube link points to https://www.youtube.com instead of the actual Djed Alliance YouTube channel. This should be updated to the correct channel URL before release.
Suggested fix
<a
- href="https://www.youtube.com"
+ href="https://www.youtube.com/@DjedAlliance"
target="_blank"
rel="noreferrer"
aria-label="Djed Alliance on YouTube"Replace with the actual YouTube channel URL.
📝 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 | |
| href="https://www.youtube.com" | |
| target="_blank" | |
| rel="noreferrer" | |
| aria-label="Djed Alliance on YouTube" | |
| className="text-black/40 hover:text-black dark:text-white/40 dark:hover:text-white transition-colors duration-300" | |
| > | |
| <YTSocial className="w-5 h-5 sm:w-6 sm:h-6" /> | |
| </a> | |
| <a | |
| href="https://www.youtube.com/@DjedAlliance" | |
| target="_blank" | |
| rel="noreferrer" | |
| aria-label="Djed Alliance on YouTube" | |
| className="text-black/40 hover:text-black dark:text-white/40 dark:hover:text-white transition-colors duration-300" | |
| > | |
| <YTSocial className="w-5 h-5 sm:w-6 sm:h-6" /> | |
| </a> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/sections/Footer.tsx` around lines 50 - 58, The YouTube anchor in the
Footer component uses a placeholder href ("https://www.youtube.com"); locate the
anchor element that contains aria-label="Djed Alliance on YouTube" (the one
rendering <YTSocial .../>) and replace the href value with the actual Djed
Alliance YouTube channel URL (ensuring target="_blank" and rel="noreferrer" are
preserved) before release.
Comment on lines
+29
to
33
| <a href="#" className="text-black/70 hover:text-black dark:text-white/70 dark:hover:text-white transition-colors duration-300"> | ||
| Developers Guide | ||
| </a> | ||
| <a href="#" className="text-white/70 hover:text-white transition"> | ||
| <a href="#" className="text-black/70 hover:text-black dark:text-white/70 dark:hover:text-white transition-colors duration-300"> | ||
| Integration Docs |
There was a problem hiding this comment.
Desktop navigation still points to placeholders.
Lines 29 and 32 still use href="#", while the drawer links go to /docs and /integration. Desktop users cannot reach those pages. Drive both navs from the same link data so the targets stay consistent.
Also applies to: 62-64
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/sections/Header.tsx` around lines 29 - 33, Desktop nav anchors use
placeholder href="#" while the drawer uses real routes; update the desktop
anchor hrefs to the correct paths (e.g., "/docs" and "/integration") and
refactor both desktop and drawer navigation to consume a single shared links
array (e.g., navItems or NAV_LINKS) so targets remain consistent; locate the
anchors in Header.tsx (the desktop links around "Developers Guide" and
"Integration Docs" and the drawer link rendering) and replace static hrefs with
link.href from the shared data, mapping that array in both places.
Comment on lines
+40
to
+46
| <button | ||
| onClick={() => setOpen(true)} | ||
| aria-label="Open menu" | ||
| className="md:hidden w-5 h-5 sm:w-6 sm:h-6 text-black dark:text-white transition-colors duration-500" | ||
| > | ||
| <MenuIcon /> | ||
| </button> |
There was a problem hiding this comment.
Treat the mobile drawer as a modal dialog.
The trigger does not expose aria-expanded/aria-controls, and the drawer has no dialog semantics, focus management, or Escape handling. Keyboard and screen-reader users can still end up in background content while the drawer is open.
Also applies to: 52-73
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/sections/Header.tsx` around lines 40 - 46, The mobile menu trigger/button
(onClick={() => setOpen(true)} with MenuIcon) and corresponding drawer must be
treated as a modal dialog: add aria-expanded and aria-controls to the trigger
(toggle with setOpen), give the drawer dialog semantics (role="dialog" and
aria-modal="true" and an id referenced by aria-controls), trap keyboard focus
inside the drawer and set initial focus to the first focusable element when it
opens, restore focus to the trigger when it closes, handle Escape to close
(listen for Escape and call setOpen(false)), and hide/inert background content
(or set aria-hidden on background regions) while open so screen readers and
keyboard users cannot reach behind the modal. Ensure these behaviors are applied
to the same mobile drawer used in the later block (lines around 52-73).
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.
Addressed Issues:
Fixes #43
Additional Notes:
The Button component was missing the type="button" attribute and did not support standard HTML button props like onClick.
I updated the component to include a default type="button" and extended it to accept common button props. This improves accessibility and makes the component more reusable across the application.
The changes were tested locally to ensure existing functionality is not affected.
Checklist:
[x] This PR addresses a single improvement
[x] Code follows the project's style and conventions
[x] Changes tested locally
[x] No new warnings or errors
Summary by CodeRabbit
New Features
Enhancements
Documentation