-
-
Notifications
You must be signed in to change notification settings - Fork 256
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
added scroll to top feature #1014
base: main
Are you sure you want to change the base?
Conversation
WalkthroughThe changes in this pull request primarily enhance the Changes
Assessment against linked issues
Possibly related issues
Possibly related PRs
Suggested labels
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
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.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
index.html (3)
907-934
: Adjust button positioning to prevent overlap with chatbotThe current right position of 80px might cause the scroll-to-top button to overlap with the chatbot button on smaller screens. Consider adjusting the positioning for better spacing.
#scroll-to-top { position: fixed; bottom: 20px; - right: 80px; + right: 120px; /* Increased spacing to prevent overlap */ } @media (max-width: 768px) { #scroll-to-top { padding: 12px; font-size: 20px; + right: 100px; /* Adjust position for mobile */ + bottom: 30px; /* Increase bottom spacing on mobile */ } }
1077-1079
: Enhance button accessibilityThe button has good basic accessibility with aria-label, but could be improved further.
- <button id="scroll-to-top" aria-label="Scroll to Top"> + <button id="scroll-to-top" + aria-label="Scroll to Top" + role="button" + aria-hidden="false" + style="color: #ffffff; background-color: #007bff;"> <i class="fas fa-arrow-up"></i> </button>
1163-1184
: Consider consolidating scroll-related JavaScript codeThe scroll-to-top functionality and progress bar implementation are in separate script tags. Consider consolidating them for better maintainability.
<script> + // Scroll-related functionality + class ScrollManager { + constructor() { + this.lastScrollPercentage = 0; + this.scrollThreshold = 200; + this.progressBar = document.getElementById('progress-bar'); + this.scrollToTopButton = document.getElementById("scroll-to-top"); + + this.initializeEventListeners(); + } + + initializeEventListeners() { + window.addEventListener('scroll', this.handleScroll.bind(this)); + this.scrollToTopButton.onclick = this.scrollToTop.bind(this); + } + + handleScroll() { + this.updateProgressBar(); + this.toggleScrollButton(); + } + + updateProgressBar() { + const scrollTop = window.scrollY; + const windowHeight = document.documentElement.scrollHeight - window.innerHeight; + const scrollPercentage = (scrollTop / windowHeight) * 100; + + this.lastScrollPercentage += (scrollPercentage - this.lastScrollPercentage) * 0.1; + this.progressBar.style.width = this.lastScrollPercentage + '%'; + } + + toggleScrollButton() { + if (document.body.scrollTop > this.scrollThreshold || + document.documentElement.scrollTop > this.scrollThreshold) { + this.scrollToTopButton.style.display = "block"; + } else { + this.scrollToTopButton.style.display = "none"; + } + } + + scrollToTop() { + window.scrollTo({ + top: 0, + behavior: "smooth" + }); + } + } + + // Initialize scroll manager + new ScrollManager(); </script>
@GarimaSingh0109 i have resolved the conflicts |
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.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
index.html (2)
Line range hint
1-1803
: Remove duplicate sections throughout the file.The file contains multiple copies of the same sections:
- 3 copies of the header section
- 3 copies of the features section
- 3 copies of the testimonials section
- 3 copies of the footer section
This duplication makes the code harder to maintain and increases the file size unnecessarily.
Keep only one copy of each section and remove the duplicates. Consider breaking down the file into smaller components:
- Create separate files for components:
components/ ├── header.html ├── features.html ├── testimonials.html └── footer.html
- Use server-side includes or a templating system to include these components in the main file.
Line range hint
20-932
: Consolidate and organize styles.The current style organization has several issues:
Multiple
<style>
tags scattered throughout the fileDuplicate style definitions (e.g., features-card, testimonial-card)
Inconsistent usage of CSS variables
Move all styles to a separate CSS file
Use CSS variables consistently:
+ /* Add to the top of your CSS file */ + :root { + /* Colors */ + --primary-color: #007bff; + --primary-dark: #0056b3; + --deep-teal: #003d4d; + --dark-slate: #333; + --off-white: #f9f9f9; + + /* Spacing */ + --section-padding: 60px 20px; + --card-gap: 30px; + + /* Transitions */ + --transition-speed: 0.3s; + + /* Breakpoints */ + --mobile-width: 768px; + }
- Organize styles by component:
/* Layout */ /* Components */ /* Utilities */ /* Media Queries */
<button id="scroll-to-top" aria-label="Scroll to Top"> | ||
<i class="fas fa-arrow-up"></i> <!-- Font Awesome Arrow Icon --> | ||
</button> |
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.
Fix duplicate scroll-to-top implementations and improve accessibility.
There are multiple scroll-to-top buttons in the file:
<button id="scroll-to-top">
at line 1072<button class="scroll-to-top" id="scrollToTopBtn">
(duplicate implementation)
Additionally, the implementation can be improved for performance and accessibility.
- Remove duplicate button implementations and keep only one.
- Apply this diff to improve the implementation:
<button id="scroll-to-top" aria-label="Scroll to Top">
<i class="fas fa-arrow-up"></i>
</button>
<style>
#scroll-to-top {
position: fixed;
bottom: 20px;
right: 20px;
background-color: #007bff;
color: white;
border: none;
border-radius: 50%;
padding: 15px;
font-size: 24px;
opacity: 0;
visibility: hidden;
cursor: pointer;
transition: opacity 0.3s ease, visibility 0.3s ease, background-color 0.3s ease;
z-index: 1000;
}
#scroll-to-top.visible {
opacity: 1;
visibility: visible;
}
#scroll-to-top:hover {
background-color: #0056b3;
}
#scroll-to-top:focus {
outline: 2px solid #fff;
outline-offset: 2px;
}
@media (max-width: 768px) {
#scroll-to-top {
padding: 12px;
font-size: 20px;
}
}
</style>
<script>
// Configuration
const SCROLL_THRESHOLD = 200;
const THROTTLE_DELAY = 150;
// Throttle function
function throttle(func, limit) {
let inThrottle;
return function(...args) {
if (!inThrottle) {
func.apply(this, args);
inThrottle = true;
setTimeout(() => inThrottle = false, limit);
}
}
}
const scrollToTopButton = document.getElementById("scroll-to-top");
// Optimized scroll handler with throttling
const handleScroll = throttle(() => {
const scrolled = window.scrollY || document.documentElement.scrollTop;
scrollToTopButton.classList.toggle('visible', scrolled > SCROLL_THRESHOLD);
}, THROTTLE_DELAY);
// Add event listeners
window.addEventListener('scroll', handleScroll, { passive: true });
scrollToTopButton.addEventListener('click', () => {
window.scrollTo({
top: 0,
behavior: 'smooth'
});
});
// Initial check
handleScroll();
</script>
Key improvements:
- Uses CSS visibility/opacity for smoother transitions
- Adds keyboard focus styles for accessibility
- Implements throttling for better scroll performance
- Uses passive scroll listener
- Adds initial state check
Also applies to: 905-931, 1783-1801
<script> | ||
// Get the button | ||
const scrollToTopButton = document.getElementById("scroll-to-top"); | ||
|
||
// Show or hide the button based on scroll position | ||
window.onscroll = function () { | ||
if (document.body.scrollTop > 200 || document.documentElement.scrollTop > 200) { | ||
scrollToTopButton.style.display = "block"; // Show button | ||
} else { | ||
scrollToTopButton.style.display = "none"; // Hide button | ||
} | ||
}; | ||
|
||
// Smooth scroll to the top when button is clicked | ||
scrollToTopButton.onclick = function () { | ||
window.scrollTo({ | ||
top: 0, | ||
behavior: "smooth" | ||
}); | ||
}; | ||
|
||
</script> |
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.
🛠️ Refactor suggestion
Consolidate and organize scripts.
Current issues with script organization:
-
Multiple script tags scattered throughout the file
-
Duplicate script includes (jQuery, Bootstrap, etc.)
-
Mix of inline and external scripts
-
Move all scripts to separate files
-
Organize scripts by functionality:
+ /* Create a directory structure */
+ scripts/
+ ├── vendors/
+ │ ├── jquery.min.js
+ │ ├── bootstrap.min.js
+ │ └── gsap.min.js
+ ├── components/
+ │ ├── scroll-to-top.js
+ │ └── progress-bar.js
+ └── main.js
- Load scripts efficiently:
<!-- Add to the bottom of body -->
<!-- Vendor scripts -->
<script src="scripts/vendors/jquery.min.js"></script>
<script src="scripts/vendors/bootstrap.min.js"></script>
<script src="scripts/vendors/gsap.min.js"></script>
<!-- Application scripts -->
<script src="scripts/components/scroll-to-top.js"></script>
<script src="scripts/components/progress-bar.js"></script>
<script src="scripts/main.js"></script>
Pull Request for Resum-Resume 💡
Closes: #538
Describe the add-ons or changes you've made 📃
Requirements fulfilled :
1 A button/icon that appears once the user scrolls down a certain distance.
2 Button should be fixed at the bottom-right corner of the screen.
3 When clicked, the page should smoothly scroll to the top.
4 Ensure the button is hidden when the user is near the top of the page.
5 The button design should match the website’s style and be mobile-friendly.
Tasks:
1 Design the "Scroll to Top" button (icon, size, color).
2 Implement the button in HTML/CSS.
3 Add JavaScript for the smooth scroll functionality.
4 Set the button to appear only when the user scrolls down (e.g., 200px from the top).
5 Test the feature across different devices and browsers.
6 Ensure the feature does not interfere with other elements on the page.
LinkedIn.Resume.Builder.-.Home.-.Google.Chrome.2024-11-09.16-29-31.mp4
Type of change ☑️
What sort of change have you made:
Checklist: ☑️
Summary by CodeRabbit
New Features
Style Enhancements