-
Notifications
You must be signed in to change notification settings - Fork 73
Add 4 interactive frontend Add 4 interactive frontend features #8
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
base: main
Are you sure you want to change the base?
Conversation
WalkthroughIntroduces UI enhancements across HTML, CSS, and JS: adds IDs and structural simplifications in index.html, a new script.js implementing theme toggling, blog “load more,” newsletter feedback, scroll-to-top, and mobile nav, and a comprehensive new style.css with theming, layout, and responsive design. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor U as User
participant D as DOM
participant S as script.js
participant LS as localStorage
note over S,D: Initialization
S->>LS: Read saved theme
LS-->>S: theme value (if any)
S->>D: Apply theme class
S->>D: Render initial blog cards
rect rgba(230,245,255,0.6)
note right of U: Theme toggle
U->>D: Click theme toggle
D->>S: toggle event
S->>D: Swap theme class
S->>LS: Persist theme
end
rect rgba(235,255,235,0.6)
note right of U: Load more posts
U->>D: Click "Load More"
D->>S: click event
S->>D: Append next blog cards
S->>D: Hide button if exhausted
end
rect rgba(255,245,230,0.6)
note right of U: Newsletter submit
U->>D: Submit form
D->>S: submit event (prevent default)
S->>D: Show success message
S->>D: Reset form
S-->>D: Hide message after timeout
end
rect rgba(245,235,255,0.6)
note right of U: Scroll to top
U->>D: Scroll page
D->>S: scroll event
S->>D: Toggle button visibility
U->>D: Click scroll-top
D->>S: click event
S->>D: Smooth scroll to top
end
rect rgba(255,240,250,0.6)
note right of U: Mobile navigation
U->>D: Tap open/close
D->>S: click event
S->>D: Toggle mobile nav panel
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ 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 |
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: 6
🧹 Nitpick comments (5)
script.js (1)
62-62: Consider extracting magic numbers to named constants.The code uses several magic numbers (increment: 2, timeout: 3000ms, scroll threshold: 300px) that would benefit from being defined as named constants at the top of the file for better maintainability.
+// Configuration constants +const BLOG_LOAD_INCREMENT = 2; +const NEWSLETTER_MESSAGE_TIMEOUT = 3000; +const SCROLL_TOP_THRESHOLD = 300; + // ... rest of code -const increment = 2; +const increment = BLOG_LOAD_INCREMENT; // ... setTimeout(() => { newsletterMsg.style.display = 'none'; -}, 3000); +}, NEWSLETTER_MESSAGE_TIMEOUT); // ... -if (document.body.scrollTop > 300 || document.documentElement.scrollTop > 300) { +if (document.body.scrollTop > SCROLL_TOP_THRESHOLD || document.documentElement.scrollTop > SCROLL_TOP_THRESHOLD) {Also applies to: 116-118, 129-129
index.html (2)
174-175: Move inline style to CSS.The success message uses an inline style
style="display:none;"which should be defined in the stylesheet for better separation of concerns and maintainability.Remove the inline style:
<!-- Success message placeholder --> -<p id="newsletterMsg" class="success-msg" style="display:none;">Thank you for subscribing!</p> +<p id="newsletterMsg" class="success-msg">Thank you for subscribing!</p>Then add to style.css:
.success-msg { display: none; color: var(--accent); margin-top: 1rem; font-weight: 600; }
214-215: Improve scroll-to-top button for consistency and accessibility.The button uses a plain text arrow "↑" instead of an icon, which is inconsistent with the rest of the site using Ionicons. Additionally, the button lacks proper accessibility attributes.
Replace with an icon and add accessibility attributes:
<!-- Scroll to Top Button --> -<button id="scrollTopBtn" title="Go to top">↑</button> +<button id="scrollTopBtn" class="scroll-top-btn" aria-label="Scroll to top"> + <ion-icon name="arrow-up-outline"></ion-icon> +</button>Then add corresponding styles in style.css to position and style the button appropriately (currently no styles are defined for #scrollTopBtn).
style.css (2)
5-7: Update or remove copyright notice.The copyright comment references "@codewithsadee" which may not be appropriate for this project. Consider updating it to reflect the actual author or removing it if it's template code.
/** - * copyright 2022 @codewithsadee + * SimpleBlog Styles + * Part of Julia Walker's Personal Blog Website */
664-673: Consider fallback for line-clamp.The blog text truncation uses
-webkit-line-clampwhich is a vendor-prefixed property with limited support in older browsers. While it works in modern browsers, consider adding a fallback.Add a max-height fallback for browsers that don't support line-clamp:
.blog-text { color: var(--foreground-secondary); font-size: var(--fs-4); display: -webkit-box; line-clamp: 3; -webkit-line-clamp: 3; -webkit-box-orient: vertical; overflow: hidden; + max-height: 4.5em; /* Approximate fallback: 3 lines × 1.5 line-height */ margin-bottom: 1rem; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
index.html(3 hunks)script.js(1 hunks)style.css(1 hunks)
🔇 Additional comments (6)
index.html (2)
118-123: LGTM!The IDs added to the blog container and load more button properly support the JavaScript functionality for progressive blog loading.
170-170: LGTM!The newsletter form ID enables proper form submission handling in the JavaScript.
style.css (4)
126-142: Scrollbar styling limited to WebKit browsers.The scrollbar styles use
::-webkit-scrollbarwhich only works in Chrome, Edge, and Safari. Firefox and other browsers won't show custom scrollbars. This is acceptable if progressive enhancement is intended, but be aware of the limitation.Consider adding Firefox scrollbar styling if cross-browser consistency is important:
/* Firefox scrollbar styling */ * { scrollbar-width: thin; scrollbar-color: var(--accent) var(--background-secondary); }
17-55: LGTM! Well-structured theme system.The CSS custom properties for light and dark themes are well organized and comprehensive. The use of semantic color names (background-primary, foreground-secondary, etc.) makes the theming system maintainable and easy to extend.
Also applies to: 57-82
565-1074: LGTM! Comprehensive responsive design.The media query breakpoints (550px, 650px, 768px, 1024px) provide good progressive enhancement across device sizes. The layout transitions from mobile-first to desktop are well thought out.
852-861: No action needed: background image path is valid.
| // Load saved theme | ||
| const savedTheme = localStorage.getItem('theme'); | ||
| if (savedTheme) body.className = savedTheme; |
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.
Replace all body classes instead of just the theme class.
Line 26 uses body.className = savedTheme which will remove any existing classes on the body element, not just replace the theme class. This could break other functionality that relies on body classes.
Apply this diff to use classList instead:
// Load saved theme
const savedTheme = localStorage.getItem('theme');
-if (savedTheme) body.className = savedTheme;
+if (savedTheme) {
+ body.classList.remove('light-theme', 'dark-theme');
+ body.classList.add(savedTheme);
+}📝 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.
| // Load saved theme | |
| const savedTheme = localStorage.getItem('theme'); | |
| if (savedTheme) body.className = savedTheme; | |
| // Load saved theme | |
| const savedTheme = localStorage.getItem('theme'); | |
| if (savedTheme) { | |
| body.classList.remove('light-theme', 'dark-theme'); | |
| body.classList.add(savedTheme); | |
| } |
🤖 Prompt for AI Agents
In script.js around lines 24 to 26, the code sets body.className = savedTheme
which clobbers all existing body classes; instead remove only the existing theme
class(es) and add the saved theme via classList. Implement logic to iterate
body.classList, remove any class that matches your theme naming convention
(e.g., /^theme-/) or previously used theme value(s), then call
body.classList.add(savedTheme) so other body classes remain intact.
| const loadMoreBtn = document.getElementById('loadMoreBtn'); | ||
| const blogContainer = document.getElementById('blog-container'); | ||
|
|
||
| // Example blog data | ||
| const blogData = [ | ||
| { | ||
| title: 'Blog Post 1', | ||
| topic: 'Database', | ||
| text: 'Lorem ipsum dolor sit amet, consectetur adipiscing elit...', | ||
| author: 'Julia Walker', | ||
| date: 'Oct 5, 2025' | ||
| }, | ||
| { | ||
| title: 'Blog Post 2', | ||
| topic: 'Accessibility', | ||
| text: 'Vestibulum ante ipsum primis in faucibus orci luctus et ultrices...', | ||
| author: 'Julia Walker', | ||
| date: 'Oct 4, 2025' | ||
| }, | ||
| { | ||
| title: 'Blog Post 3', | ||
| topic: 'Web Performance', | ||
| text: 'Phasellus bibendum, mi nec cursus interdum, urna lorem facilisis...', | ||
| author: 'Julia Walker', | ||
| date: 'Oct 3, 2025' | ||
| } | ||
| ]; | ||
|
|
||
| let loadedCount = 0; | ||
| const increment = 2; | ||
|
|
||
| function loadBlogs() { | ||
| const nextBlogs = blogData.slice(loadedCount, loadedCount + increment); | ||
|
|
||
| nextBlogs.forEach(blog => { | ||
| const blogCard = document.createElement('div'); | ||
| blogCard.classList.add('blog-card'); | ||
| blogCard.innerHTML = ` | ||
| <div class="blog-card-banner"> | ||
| <img src="./assets/images/blog-placeholder.png" alt="${blog.title}" class="blog-banner-img"> | ||
| </div> | ||
| <div class="blog-content-wrapper"> | ||
| <span class="blog-topic">${blog.topic}</span> | ||
| <h3 class="h3">${blog.title}</h3> | ||
| <p class="blog-text">${blog.text}</p> | ||
| <div class="wrapper-flex"> | ||
| <div class="profile-wrapper"> | ||
| <img src="./assets/images/profile.png" alt="${blog.author}"> | ||
| </div> | ||
| <span class="h4">${blog.author}</span> | ||
| <span class="separator"></span> | ||
| <span class="text-sm">${blog.date}</span> | ||
| </div> | ||
| </div> | ||
| `; | ||
| blogContainer.appendChild(blogCard); | ||
| }); | ||
|
|
||
| loadedCount += increment; | ||
|
|
||
| if (loadedCount >= blogData.length) { | ||
| loadMoreBtn.style.display = 'none'; | ||
| } | ||
| } | ||
|
|
||
| // Initial load | ||
| loadBlogs(); | ||
|
|
||
| loadMoreBtn.addEventListener('click', loadBlogs); |
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.
Add null checks before using DOM elements.
The code accesses loadMoreBtn and blogContainer (lines 33-34) and uses them throughout without verifying they exist. If these elements are missing from the HTML, the code will throw runtime errors.
Add null checks before using these elements:
const loadMoreBtn = document.getElementById('loadMoreBtn');
const blogContainer = document.getElementById('blog-container');
+
+if (!loadMoreBtn || !blogContainer) {
+ console.error('Required elements not found: loadMoreBtn or blogContainer');
+ // Exit early or handle gracefully
+} else {
+ // Existing blog loading logic here
+}Alternatively, wrap the entire blog loading logic in a conditional block or use optional chaining where appropriate.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In script.js around lines 33 to 101 the code assumes
getElementById('loadMoreBtn') and getElementById('blog-container') always return
elements; add explicit null checks and guard clauses: if either element is
missing, exit the script or return early to avoid runtime errors, only run
loadBlogs and attach the click listener when both elements exist, and inside
loadBlogs ensure blogContainer is valid before appending nodes (or skip
rendering if not). Also avoid calling loadMoreBtn.addEventListener when
loadMoreBtn is null by checking it first (or use conditional chaining).
| const newsletterForm = document.getElementById('newsletterForm'); | ||
| const newsletterMsg = document.getElementById('newsletterMsg'); | ||
|
|
||
| newsletterForm.addEventListener('submit', e => { | ||
| e.preventDefault(); | ||
| newsletterMsg.style.display = 'block'; | ||
| newsletterForm.reset(); | ||
|
|
||
| setTimeout(() => { | ||
| newsletterMsg.style.display = 'none'; | ||
| }, 3000); | ||
| }); |
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.
Add null checks for newsletter form elements.
Similar to the blog loading feature, the newsletter form code accesses elements without verifying they exist, which will cause runtime errors if the elements are missing.
Add defensive checks:
const newsletterForm = document.getElementById('newsletterForm');
const newsletterMsg = document.getElementById('newsletterMsg');
+if (newsletterForm && newsletterMsg) {
newsletterForm.addEventListener('submit', e => {
e.preventDefault();
newsletterMsg.style.display = 'block';
newsletterForm.reset();
setTimeout(() => {
newsletterMsg.style.display = 'none';
}, 3000);
});
+}📝 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 newsletterForm = document.getElementById('newsletterForm'); | |
| const newsletterMsg = document.getElementById('newsletterMsg'); | |
| newsletterForm.addEventListener('submit', e => { | |
| e.preventDefault(); | |
| newsletterMsg.style.display = 'block'; | |
| newsletterForm.reset(); | |
| setTimeout(() => { | |
| newsletterMsg.style.display = 'none'; | |
| }, 3000); | |
| }); | |
| const newsletterForm = document.getElementById('newsletterForm'); | |
| const newsletterMsg = document.getElementById('newsletterMsg'); | |
| if (newsletterForm && newsletterMsg) { | |
| newsletterForm.addEventListener('submit', e => { | |
| e.preventDefault(); | |
| newsletterMsg.style.display = 'block'; | |
| newsletterForm.reset(); | |
| setTimeout(() => { | |
| newsletterMsg.style.display = 'none'; | |
| }, 3000); | |
| }); | |
| } |
🤖 Prompt for AI Agents
In script.js around lines 108 to 119, the newsletterForm and newsletterMsg DOM
elements are used without null checks which can throw if either element is
missing; update the code to first verify newsletterForm exists before calling
addEventListener, and verify newsletterMsg exists before reading or setting its
style (or use optional chaining), returning early if required elements are
absent so no operations are performed on null references.
| window.onscroll = () => { | ||
| if (document.body.scrollTop > 300 || document.documentElement.scrollTop > 300) { | ||
| scrollTopBtn.style.display = 'block'; | ||
| } else { | ||
| scrollTopBtn.style.display = 'none'; | ||
| } | ||
| }; |
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.
Use addEventListener instead of window.onscroll.
Line 128 assigns directly to window.onscroll, which will overwrite any existing scroll handlers in the codebase. This can break other scroll-based functionality.
Replace with addEventListener:
-window.onscroll = () => {
+window.addEventListener('scroll', () => {
if (document.body.scrollTop > 300 || document.documentElement.scrollTop > 300) {
scrollTopBtn.style.display = 'block';
} else {
scrollTopBtn.style.display = 'none';
}
-};
+});Also add a null check for scrollTopBtn before using it:
const scrollTopBtn = document.getElementById('scrollTopBtn');
+
+if (scrollTopBtn) {
+ window.addEventListener('scroll', () => {
+ // ... scroll logic
+ });
+
+ scrollTopBtn.addEventListener('click', () => {
+ window.scrollTo({ top: 0, behavior: 'smooth' });
+ });
+}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In script.js around lines 128 to 134, the code assigns directly to
window.onscroll which can overwrite other handlers and uses scrollTopBtn without
a null check; replace the assignment with window.addEventListener('scroll',
handler) and inside the handler first guard that scrollTopBtn is not
null/undefined, then perform the show/hide logic based on
document.body.scrollTop or document.documentElement.scrollTop; register the
listener with an appropriate options object (e.g., { passive: true }) if desired
to avoid blocking scroll performance.
| const navMenuBtn = document.querySelector('.nav-menu-btn'); | ||
| const mobileNav = document.querySelector('.mobile-nav'); | ||
| const navCloseBtn = document.querySelector('.nav-close-btn'); | ||
|
|
||
| navMenuBtn.addEventListener('click', () => { | ||
| mobileNav.classList.add('active'); | ||
| }); | ||
|
|
||
| navCloseBtn.addEventListener('click', () => { | ||
| mobileNav.classList.remove('active'); | ||
| }); |
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.
Add null checks for mobile navigation elements.
The mobile navigation code will throw errors if any of the required elements are missing from the DOM.
Add defensive checks:
const navMenuBtn = document.querySelector('.nav-menu-btn');
const mobileNav = document.querySelector('.mobile-nav');
const navCloseBtn = document.querySelector('.nav-close-btn');
-navMenuBtn.addEventListener('click', () => {
- mobileNav.classList.add('active');
-});
-
-navCloseBtn.addEventListener('click', () => {
- mobileNav.classList.remove('active');
-});
+if (navMenuBtn && mobileNav && navCloseBtn) {
+ navMenuBtn.addEventListener('click', () => {
+ mobileNav.classList.add('active');
+ });
+
+ navCloseBtn.addEventListener('click', () => {
+ mobileNav.classList.remove('active');
+ });
+}📝 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 navMenuBtn = document.querySelector('.nav-menu-btn'); | |
| const mobileNav = document.querySelector('.mobile-nav'); | |
| const navCloseBtn = document.querySelector('.nav-close-btn'); | |
| navMenuBtn.addEventListener('click', () => { | |
| mobileNav.classList.add('active'); | |
| }); | |
| navCloseBtn.addEventListener('click', () => { | |
| mobileNav.classList.remove('active'); | |
| }); | |
| const navMenuBtn = document.querySelector('.nav-menu-btn'); | |
| const mobileNav = document.querySelector('.mobile-nav'); | |
| const navCloseBtn = document.querySelector('.nav-close-btn'); | |
| if (navMenuBtn && mobileNav && navCloseBtn) { | |
| navMenuBtn.addEventListener('click', () => { | |
| mobileNav.classList.add('active'); | |
| }); | |
| navCloseBtn.addEventListener('click', () => { | |
| mobileNav.classList.remove('active'); | |
| }); | |
| } |
🤖 Prompt for AI Agents
In script.js around lines 145 to 155, the code assumes navMenuBtn, navCloseBtn
and mobileNav always exist and will throw if any are null; add defensive null
checks before using them: query the three elements, verify each is non-null (or
at least that mobileNav and the corresponding button exist) and only call
addEventListener or classList methods when present, otherwise skip the setup
(optionally log a concise warning); you can use simple if-checks or optional
chaining to guard the calls so the script no-ops when elements are missing.
|
|
||
| .footer-link { margin-bottom: 0.3rem; } | ||
|
|
||
| } No newline at end of 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.
Add missing styles for scroll-to-top button.
The scroll-to-top button (#scrollTopBtn) is added in the HTML but has no CSS styling defined. This element needs positioning, sizing, and visual styling to function properly.
Add styles at the end of the footer section or in the media queries:
/* Scroll to Top Button */
#scrollTopBtn {
position: fixed;
bottom: 30px;
right: 30px;
background: var(--accent);
color: var(--white);
width: 50px;
height: 50px;
border-radius: 50%;
display: none;
justify-content: center;
align-items: center;
font-size: 24px;
cursor: pointer;
box-shadow: 0 5px 15px hsla(0, 0%, 0%, 0.15);
transition: 0.25s ease;
z-index: 100;
}
#scrollTopBtn:hover {
background: var(--foreground-secondary);
transform: translateY(-3px);
box-shadow: 0 8px 20px hsla(0, 0%, 0%, 0.2);
}🤖 Prompt for AI Agents
In style.css around line 1074, the #scrollTopBtn element added to the HTML lacks
any CSS rules; add a fixed-position, circular button with sizing, colors,
centered content, hidden-by-default display, shadow, transition and high
z-index, plus a hover state that changes background, lifts the button slightly
and increases shadow; place these rules at the end of the footer section or
inside appropriate media queries so the button is positioned bottom-right,
visually matches the site variables (--accent, --white, --foreground-secondary),
and toggles visibility via display or a helper class when JS shows/hides it.
Implemented mobile navbar toggle, light/dark theme toggle, theme button state toggle, and support for multiple buttons.
Summary by CodeRabbit
New Features
Style
Refactor