Skip to content
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

Jump to bottom

feature: added scroll to top button #10174

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

feature: added scroll to top button #10174

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
8 changes: 7 additions & 1 deletion src/core/components/app.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
*/
import React from "react"
import PropTypes from "prop-types"
import ScrollToTopButton from "./scroll-to-top"

class App extends React.Component {
getLayout() {
Expand All @@ -18,7 +19,12 @@ class App extends React.Component {
render() {
const Layout = this.getLayout()

return <Layout />
return (
<>
<ScrollToTopButton />
<Layout />
</>
)
}
}

Expand Down
85 changes: 85 additions & 0 deletions src/core/components/scroll-to-top.jsx
View file
Open in desktop
Copy link

Choose a reason for hiding this comment

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

Consider improving the variable names for better clarity and readability.

Original file line number Diff line number Diff line change
@@ -0,0 +1,85 @@
import React, { useState, useEffect } from "react"

const ScrollToTopButton = () => {
const [isVisible, setIsVisible] = useState(false)
const [hideTimeout, setHideTimeout] = useState(null)
Copy link

Choose a reason for hiding this comment

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

consider using useRef insted of useState to store the timeout ID. This way, it will persist across renders without triggering unnecessary re-renders.


const styles = {
Copy link

Choose a reason for hiding this comment

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

Inline styles are not used in this project, so this code doesn't follow the convention.

Copy link

Choose a reason for hiding this comment

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

And for the future. Since the styles object doesn’t depend on any props, it’s best to define it outside the component. This avoids redefining the styles object on each render, improving performance.

button: {
position: "fixed",
bottom: "20px",
right: "20px",
padding: "10px 20px",
fontSize: "18px",
backgroundColor: "rgba(125, 132, 146, 0.8)",
color: "#fff",
border: "2px solid rgba(125, 132, 146, 0.8)",
borderRadius: "5px",
cursor: "pointer",
boxShadow: "0px 4px 6px rgba(0, 0, 0, 0.1), 0 0 15px rgba(125, 132, 146, 0.6)",
transition: "opacity 0.5s ease, box-shadow 0.3s ease",
opacity: isVisible ? 1 : 0,
pointerEvents: isVisible ? "auto" : "none",
},
icon: {
width: "20px",
height: "20px",
fill: "#fff",
},
}

const toggleVisibility = () => {
Copy link

Choose a reason for hiding this comment

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

The toggleVisibility function handles both the visibility toggle and timeout reset, which could make the code harder to follow. Consider breaking it up for simplicity: keep the scroll visibility logic and timeout logic separate. Instead of adding this logic directly to that file, consider creating a custom hook. This approach would improve readability and maintain cleaner code.

if (window.scrollY > 300) {
setIsVisible(true)

if (hideTimeout) {
clearTimeout(hideTimeout)
}

const timeout = setTimeout(() => {
setIsVisible(false)
}, 3000)

setHideTimeout(timeout)
} else {
setIsVisible(false)
}
}

const scrollToTop = () => {
window.scrollTo({
top: 0,
behavior: "smooth",
})
}

useEffect(() => {
Copy link

Choose a reason for hiding this comment

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

You’re adding an event listener to window inside useEffect, which is great, but since toggleVisibility depends on nothing external, it could be defined directly inside useEffect for clarity and potential memory efficiency.

window.addEventListener("scroll", toggleVisibility)

return () => {
window.removeEventListener("scroll", toggleVisibility)
if (hideTimeout) {
clearTimeout(hideTimeout)
}
}
}, [hideTimeout])

return (
<div>
Copy link

Choose a reason for hiding this comment

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

Fragmen will be better here i think.

{isVisible && (
<button
onClick={scrollToTop}
style={styles.button}
title="Go to top"
aria-label="Go to top"
>
<svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 20 20" style={styles.icon} aria-hidden="true">
<path d="M 17.418 14.908 C 17.69 15.176 18.127 15.176 18.397 14.908 C 18.667 14.64 18.668 14.207 18.397 13.939 L 10.489 6.109 C 10.219 5.841 9.782 5.841 9.51 6.109 L 1.602 13.939 C 1.332 14.207 1.332 14.64 1.602 14.908 C 1.873 15.176 2.311 15.176 2.581 14.908 L 10 7.767 L 17.418 14.908 Z"></path>
</svg>
</button>
)}
</div>
)
}

export default ScrollToTopButton
Copy link

Choose a reason for hiding this comment

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

question - why you prefer default export instead of named export ?