-
-
Notifications
You must be signed in to change notification settings - Fork 69
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.
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
Scroll to top feature #729
base: main
Are you sure you want to change the base?
Conversation
Hey @iitzIrFan , it would be great if you could provide an implementation video or a screenshot of the task you’ve completed |
@abhayymishraa 2025-02-03.22-50-36.mp4Also the server error is due to missing Algolia api key ! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for adding this scroll up button!
Could you please change the color in light theme, to match the theme color.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@iitzIrFan thanks for working on this!
I noticed that CI\CD is failing. Could you run make check
and address anything that pops up there? This is our normal workflow to run make check
every time before pushing anything.
Will review again after the updates! 👌🏼
@arkid15r @kasya @aramattamara |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you resolve conflicts and fix code style?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@iitzIrFan any updates on this?
@arkid15r Sorry, I didn't inform you. will update you till 15th as currently in middle of exams |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @iitzIrFan! Is this updated and ready for another round of review?
Will update ! |
@kasya mam you can review this now update me if, there is any issue. |
frontend/src/components/ScrollUp.jsx
Outdated
import React, { useEffect, useState } from 'react' | ||
|
||
const ScrollUp = () => { | ||
const [isVisible, setIsVisible] = useState(false) | ||
|
||
// Show button when page is scrolled up to given distance | ||
const toggleVisibility = () => { | ||
if (window.pageYOffset > 300) { | ||
setIsVisible(true) | ||
} else { | ||
setIsVisible(false) | ||
} | ||
} | ||
|
||
const scrollToTop = () => { | ||
window.scrollTo({ | ||
top: 0, | ||
behavior: 'smooth', | ||
}) | ||
} | ||
|
||
useEffect(() => { | ||
window.addEventListener('scroll', toggleVisibility) | ||
return () => { | ||
window.removeEventListener('scroll', toggleVisibility) | ||
} | ||
}, []) | ||
|
||
return ( | ||
<> | ||
{isVisible && ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still see this file in the PR. Since the same file already exists in .tsx format in this PR, could you please clarify if this one is needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you elaborate here as there is some misunderstanding issue I guess !
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this PR, there are two files: frontend/src/components/ScrollUp.jsx
and frontend/src/components/ScrollUp.tsx
. The content inside both files is nearly identical. Since our project uses TypeScript, it seems that the .jsx file was added by mistake
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok will update this !
Resolves #614
Had added scroll to top button that becomes visible when the user scrolls down a certain distance. Clicking this button would smoothly scroll the page back to the top. The button should be styled minimally and placed in the bottom-right corner of the viewport for easy access.
Also keeping the requirements of this issue to be :-