Feat: To add topbar Arrow that auto scroll-up on home page .#56
Feat: To add topbar Arrow that auto scroll-up on home page .#56AayushSahani01 wants to merge 7 commits intoCoderUzumaki:mainfrom
Conversation
|
@AayushSahani01 is attempting to deploy a commit to the coderuzumaki's projects Team on Vercel. A member of the Team first needs to authorize it. |
|
hey @CoderUzumaki , 👍🏻
|
|
Hey @AayushSahani01, |
|
Hey I'm also added this issues on raised PR request #56 both feature added as theme toggle & arrow functionality ! |
WalkthroughAdds styled-components dependency, introduces ThemeToggle and TopBar components, updates global styles for dark/light themes, and makes a whitespace-only edit in App.jsx. ThemeToggle manages theme via localStorage and a DOM class. TopBar shows a scroll progress bar and a back-to-top button using styled-components. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant ThemeToggle as ThemeToggle
participant DOM as document.documentElement
participant LS as localStorage
rect rgb(235,245,255)
note right of ThemeToggle: Mount/init
ThemeToggle->>LS: getItem("theme")
alt theme === "dark"
ThemeToggle->>DOM: add "dark" class
ThemeToggle->>ThemeToggle: set isDarkMode = true
else
ThemeToggle->>ThemeToggle: set isDarkMode = false
end
end
User->>ThemeToggle: click toggle
alt isDarkMode -> light
ThemeToggle->>DOM: remove "dark" class
ThemeToggle->>LS: setItem("theme","light")
ThemeToggle->>ThemeToggle: set isDarkMode = false
else light -> dark
ThemeToggle->>DOM: add "dark" class
ThemeToggle->>LS: setItem("theme","dark")
ThemeToggle->>ThemeToggle: set isDarkMode = true
end
sequenceDiagram
autonumber
participant Window as window
participant TopBar as OnTopBar
participant Doc as document.documentElement
note right of TopBar: Mount
Window-->>TopBar: addEventListener("scroll", handler)
Window->>TopBar: scroll event (repeat)
TopBar->>Doc: read scrollTop, scrollHeight, clientHeight
TopBar->>TopBar: update scrollProgress, isVisible
TopBar->>Window: onClick "to top" -> window.scroll({top:0, behavior:"smooth"})
note right of TopBar: Unmount
Window-->>TopBar: removeEventListener("scroll", handler)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
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 |
|
Hey @CoderUzumaki, Can you please review the code and merge it to the main branch |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (6)
client/src/App.jsx (1)
31-31: Remove stray blank line.Keeps diffs clean and avoids noise in future blame.
- ); -} + ); +}client/src/components/ThemeToggle.jsx (1)
27-31: Tailwind class 'hover:transform-fill' is invalid.Replace with supported utilities; also add accessible labeling.
-<button onClick={toggleTheme} className="p-1 cursor-pointer hover:bg-slate-500 rounded-lg transition-colors duration-300"> +<button + onClick={toggleTheme} + aria-label={isDarkMode ? 'Switch to light theme' : 'Switch to dark theme'} + className="p-1 cursor-pointer hover:bg-slate-500 rounded-lg transition-colors duration-300" +> - {!isDarkMode ? (<Sun className="h-6 w-6 text-yellow-300"/>) : (<Moon className="h-6 w-6 text-blue-700 hover:transform-fill"/>)} + {!isDarkMode ? ( + <Sun className="h-6 w-6 text-yellow-300" /> + ) : ( + <Moon className="h-6 w-6 text-blue-700" /> + )}client/src/components/TopBar.jsx (4)
6-7: State naming nit: use isVisible to match setter.Improves readability.
-const [isvisible, setIsVisible] = React.useState(false); +const [isVisible, setIsVisible] = React.useState(false);Also update usages of
isvisibleaccordingly.
32-37: Use passive listener and initialize on mount.Better scroll perf and correct initial render.
-useEffect(() => { - window.addEventListener("scroll", handleScroll); - return () => { - window.removeEventListener("scroll", handleScroll); - }; -}, []); +useEffect(() => { + window.addEventListener("scroll", handleScroll, { passive: true }); + handleScroll(); + return () => window.removeEventListener("scroll", handleScroll); +}, []); // handleScroll is stable within this component
57-85: Ensure visibility above header and improve hover contrast.Add z-index to fixed elements and invert icon color on hover for contrast.
.progress-bar { position: fixed; top: 0; left: 0; height: 2px; background: #00abff; + z-index: 50; } .on-top-bar { position: fixed; bottom: 20px; right: 18px; background-color: #fff; color: #1a1a1a; border-radius: 50%; width: 40px; height: 40px; display: flex; justify-content: center; align-items: center; cursor: pointer; transition: background-color 0.2s ease-in-out; box-shadow: 0 2px 4px rgba(0, 0, 0, 0.2); + z-index: 50; &:hover { - background-color: #0039ff; + background-color: #0039ff; + color: #ffffff; } }
3-3: Naming convention: prefer 'styled' over 'Styled'.Community convention aids readability:
import styled from 'styled-components'.-import Styled from "styled-components"; +import styled from "styled-components"; ... -const Container = Styled.section` +const Container = styled.section`
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
client/package-lock.jsonis excluded by!**/package-lock.jsonpackage-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (5)
client/package.json(1 hunks)client/src/App.jsx(1 hunks)client/src/components/ThemeToggle.jsx(1 hunks)client/src/components/TopBar.jsx(1 hunks)client/src/styles/index.css(1 hunks)
🧰 Additional context used
🪛 Biome (2.1.2)
client/src/styles/index.css
[error] 35-35: Unexpected unknown pseudo-class body
See MDN web docs for more details.
(lint/correctness/noUnknownPseudoClass)
🔇 Additional comments (2)
client/src/App.jsx (1)
20-29: Confirm TopBar and ThemeToggle are mounted somewhere.I don’t see them used here; if they’re intended for the home page, ensure they’re rendered (e.g., in Header or layout).
client/package.json (1)
26-26: Cannot verify in sandbox — repo empty; run local checks for styled-components v6 + React 19 + ViteAutomated script returned an empty workspace; cannot confirm peer-dep warnings or Babel-plugin need. Run these in the project root (client/) and paste outputs (peer-dep warnings + build logs):
- client/package.json (context): "styled-components": "^6.1.19",
- cd client
- npm ls react styled-components --depth=0 || true
- npm install # capture any peer-dependency warnings
- npm run build --silent # capture build warnings/errors
- grep -nR "babel-plugin-styled-components" . || true
- grep -nR "styled-components/babel" . || true
| useEffect(() => { | ||
| if( localStorage.getItem('theme') === 'dark' ){ | ||
| document.documentElement.classList.add('dark'); | ||
| setIsDarkMode(true); | ||
| } | ||
|
|
||
| },[]) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Initialize dark mode from storage or system preference; current logic may show wrong icon on first paint.
Defaulting isDarkMode to false while the CSS default is dark causes a mismatch.
-useEffect(() => {
- if( localStorage.getItem('theme') === 'dark' ){
- document.documentElement.classList.add('dark');
- setIsDarkMode(true);
- }
-
-},[])
+useEffect(() => {
+ const stored = localStorage.getItem('theme'); // 'dark' | 'light' | null
+ const prefersDark = window.matchMedia?.('(prefers-color-scheme: dark)').matches;
+ const nextIsDark = stored ? stored === 'dark' : !!prefersDark;
+ document.documentElement.classList.toggle('dark', nextIsDark);
+ setIsDarkMode(nextIsDark);
+}, []);📝 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.
| useEffect(() => { | |
| if( localStorage.getItem('theme') === 'dark' ){ | |
| document.documentElement.classList.add('dark'); | |
| setIsDarkMode(true); | |
| } | |
| },[]) | |
| useEffect(() => { | |
| const stored = localStorage.getItem('theme'); // 'dark' | 'light' | null | |
| const prefersDark = window.matchMedia?.('(prefers-color-scheme: dark)')?.matches; | |
| const nextIsDark = stored ? stored === 'dark' : !!prefersDark; | |
| document.documentElement.classList.toggle('dark', nextIsDark); | |
| setIsDarkMode(nextIsDark); | |
| }, []); |
🤖 Prompt for AI Agents
In client/src/components/ThemeToggle.jsx around lines 8 to 14, the component
initializes isDarkMode as false and only updates it in a useEffect, causing an
icon mismatch on first paint; change the implementation so the initial state is
computed from localStorage (if present) or from the prefers-color-scheme media
query and apply the corresponding document.documentElement.classList before
render (or use useLayoutEffect to avoid a paint flicker), and ensure both the
class toggle and setIsDarkMode are synchronized when mounting.
| const toggleTheme = () => { | ||
| if(isDarkMode){ | ||
| document.documentElement.classList.remove('dark'); | ||
| localStorage.setItem('theme', 'light'); | ||
| setIsDarkMode(false); | ||
| }else{ | ||
| document.documentElement.classList.add('dark'); | ||
| localStorage.setItem('theme', 'dark'); | ||
| setIsDarkMode(true); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Normalize toggle semantics and storage values.
Ensure the class and localStorage reflect the same meaning.
-const toggleTheme = () => {
- if(isDarkMode){
- document.documentElement.classList.remove('dark');
- localStorage.setItem('theme', 'light');
- setIsDarkMode(false);
- }else{
- document.documentElement.classList.add('dark');
- localStorage.setItem('theme', 'dark');
- setIsDarkMode(true);
- }
-}
+const toggleTheme = () => {
+ const next = !isDarkMode;
+ document.documentElement.classList.toggle('dark', next);
+ localStorage.setItem('theme', next ? 'dark' : 'light');
+ setIsDarkMode(next);
+};📝 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.
| const toggleTheme = () => { | |
| if(isDarkMode){ | |
| document.documentElement.classList.remove('dark'); | |
| localStorage.setItem('theme', 'light'); | |
| setIsDarkMode(false); | |
| }else{ | |
| document.documentElement.classList.add('dark'); | |
| localStorage.setItem('theme', 'dark'); | |
| setIsDarkMode(true); | |
| } | |
| } | |
| const toggleTheme = () => { | |
| const next = !isDarkMode; | |
| document.documentElement.classList.toggle('dark', next); | |
| localStorage.setItem('theme', next ? 'dark' : 'light'); | |
| setIsDarkMode(next); | |
| }; |
🤖 Prompt for AI Agents
In client/src/components/ThemeToggle.jsx around lines 15 to 25, the toggle
should consistently apply the same semantics to the DOM class and localStorage;
replace the branching with a single toggle flow that computes newIsDark =
!isDarkMode, then if newIsDark add the 'dark' class else remove it, set
localStorage.setItem('theme', newIsDark ? 'dark' : 'light'), and finally call
setIsDarkMode(newIsDark) so the class, storage value and state all reflect the
same meaning.
| const handleScroll = () => { | ||
| const winScroll = document.documentElement.scrollTop; | ||
| const height = document.documentElement.scrollHeight - document.documentElement.clientHeight; | ||
| const scrolled = (winScroll / height) * 100; | ||
|
|
||
| setScrollProgress(scrolled); | ||
|
|
||
| if (window.scrollY > 100) { | ||
| setIsVisible(true); | ||
| } else { | ||
| setIsVisible(false); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Guard division by zero and clamp progress; improve cross-browser scroll source.
Avoid NaN/Infinity when the page isn’t scrollable and support body/html differences.
-const handleScroll = () => {
- const winScroll = document.documentElement.scrollTop;
- const height = document.documentElement.scrollHeight - document.documentElement.clientHeight;
- const scrolled = (winScroll / height) * 100;
-
- setScrollProgress(scrolled);
-
- if (window.scrollY > 100) {
- setIsVisible(true);
- } else {
- setIsVisible(false);
- }
-};
+const handleScroll = () => {
+ const doc = document.documentElement;
+ const body = document.body;
+ const winScroll = doc.scrollTop || body.scrollTop || 0;
+ const height = (doc.scrollHeight || body.scrollHeight) - doc.clientHeight;
+ const pct = height > 0 ? Math.min(100, Math.max(0, (winScroll / height) * 100)) : 0;
+ setScrollProgress(pct);
+ setIsVisible(window.scrollY > 100);
+};📝 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.
| const handleScroll = () => { | |
| const winScroll = document.documentElement.scrollTop; | |
| const height = document.documentElement.scrollHeight - document.documentElement.clientHeight; | |
| const scrolled = (winScroll / height) * 100; | |
| setScrollProgress(scrolled); | |
| if (window.scrollY > 100) { | |
| setIsVisible(true); | |
| } else { | |
| setIsVisible(false); | |
| } | |
| }; | |
| const handleScroll = () => { | |
| const doc = document.documentElement; | |
| const body = document.body; | |
| const winScroll = doc.scrollTop || body.scrollTop || 0; | |
| const height = (doc.scrollHeight || body.scrollHeight) - doc.clientHeight; | |
| const pct = height > 0 ? Math.min(100, Math.max(0, (winScroll / height) * 100)) : 0; | |
| setScrollProgress(pct); | |
| setIsVisible(window.scrollY > 100); | |
| }; |
🤖 Prompt for AI Agents
In client/src/components/TopBar.jsx around lines 18 to 30, the scroll progress
calculation can produce NaN/Infinity on non-scrollable pages and doesn't account
for cross-browser differences in where scrollTop is stored; update handleScroll
to read scroll position from window.scrollY ||
document.documentElement.scrollTop || document.body.scrollTop, compute total
scrollable height defensively (e.g. const height =
Math.max(document.documentElement.scrollHeight, document.body.scrollHeight) -
Math.max(document.documentElement.clientHeight, document.body.clientHeight)),
guard against division by zero by setting scrolled to 0 when height <= 0, and
clamp the resulting progress between 0 and 100 before calling setScrollProgress;
keep the existing visibility toggle logic.
| <div className="progress-bar" style={{ width: `${scrollProgress}%` }} /> | ||
| {isvisible && ( | ||
| <div className="on-top-bar" onClick={onTopbtn}> | ||
| <ArrowBigUp /> | ||
| </div> | ||
| )} | ||
| </Container> |
There was a problem hiding this comment.
Use a button for accessibility; add aria-label.
Clickable divs are not keyboard-accessible by default.
- {isvisible && (
- <div className="on-top-bar" onClick={onTopbtn}>
- <ArrowBigUp />
- </div>
- )}
+ {isVisible && (
+ <button className="on-top-bar" type="button" onClick={onTopbtn} aria-label="Back to top">
+ <ArrowBigUp />
+ </button>
+ )}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In client/src/components/TopBar.jsx around lines 41 to 47, the clickable
"on-top-bar" div is not keyboard-accessible; replace that div with a semantic
<button> element, move the onClick handler to the button, add an appropriate
aria-label (e.g., "Scroll to top"), ensure any styling or className is preserved
on the button, and remove any tabIndex/role hacks so the element is naturally
focusable and accessible to keyboard and assistive technologies.
| :body { | ||
| background-color: #1a1a1a; | ||
| color: #f3f4f6; | ||
| } |
There was a problem hiding this comment.
Fix invalid selector ':body' (lint error).
:body is not a valid selector; use body.
-:body {
+body {
background-color: #1a1a1a;
color: #f3f4f6;
}📝 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.
| :body { | |
| background-color: #1a1a1a; | |
| color: #f3f4f6; | |
| } | |
| body { | |
| background-color: #1a1a1a; | |
| color: #f3f4f6; | |
| } |
🧰 Tools
🪛 Biome (2.1.2)
[error] 35-35: Unexpected unknown pseudo-class body
See MDN web docs for more details.
(lint/correctness/noUnknownPseudoClass)
🤖 Prompt for AI Agents
In client/src/styles/index.css around lines 35 to 38, the selector uses an
invalid `:body`; replace it with the valid `body` selector so the rule reads
`body { background-color: #1a1a1a; color: #f3f4f6; }` and then run the CSS
linter to confirm the error is resolved.
hey @CoderUzumaki ,Please, Can you review my work implementation that feature as per your instructions👍🏻 |
2 participants
Summary by CodeRabbit
New Features
Style
Chores