Enhance JS: Add theme memory, mobile nav auto-close, email validation… #6
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…, load more blogs
WalkthroughSingle-file JavaScript update adding mobile nav close-on-click, theme toggle with localStorage persistence, newsletter email validation, and “load more” blog cards behavior; minor refactor converting a function expression to an arrow function. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Burger as Burger/Menu Buttons
participant Script as script.js
participant DOM as DOM
User->>Burger: Click
Burger->>Script: navToggleFunc()
Script->>DOM: Toggle .active on nav
note over DOM: Mobile nav links also close nav on click
sequenceDiagram
autonumber
actor User
participant ThemeBtn as Theme Button(s)
participant Script as script.js
participant DOM as document.body
participant Store as localStorage
Script->>Store: getItem('theme')
alt saved theme exists
Script->>DOM: Apply saved theme classes
else no saved theme
Script->>DOM: Keep default classes
end
User->>ThemeBtn: Click
ThemeBtn->>Script: toggleTheme()
Script->>DOM: Toggle dark/light classes
Script->>Store: setItem('theme', currentTheme)
sequenceDiagram
autonumber
actor User
participant Form as Newsletter Form
participant Script as script.js
participant DOM as Email Input
User->>Form: Submit
Form->>Script: onSubmit()
Script->>DOM: Validate email format
alt invalid
Script-->>User: alert("Invalid email")
Script->>Form: preventDefault()
else valid
Script->>Form: allow submission
end
sequenceDiagram
autonumber
actor User
participant Button as Load More Button
participant Script as script.js
participant Cards as Blog Cards (NodeList)
Script->>Cards: showCards(initialCount)
User->>Button: Click "Load more"
Button->>Script: showCards(nextCount)
Script->>Cards: Reveal additional hidden cards
alt all cards shown
Script->>Button: Hide button
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Poem
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
assets/js/script.js (2)
15-21: Close-on-click: add a null-safe guard; consider delegation (optional)Current handler will throw if
navis null. Add a safe guard; delegation reduces N listeners.-mobileNavLinks.forEach(link => { - link.addEventListener('click', () => { - nav.classList.remove('active'); - }); -}); +mobileNavLinks.forEach(link => { + link.addEventListener('click', () => { + nav?.classList.remove('active'); + }); +});Alternative (delegation; replace the block above):
nav?.addEventListener('click', (e) => { if (e.target.closest('.nav-link')) nav.classList.remove('active'); });
46-58: Avoid toggle drift; compute next theme and set classes explicitlyPrevents states where both classes are present or out of sync with button classes.
- btn.addEventListener('click', () => { - const isDark = body.classList.toggle('dark-theme'); - body.classList.toggle('light-theme'); - - themeBtn.forEach(btn => { - btn.classList.toggle('dark'); - btn.classList.toggle('light'); - }); - - localStorage.setItem('theme', isDark ? 'dark' : 'light'); - }); + btn.addEventListener('click', () => { + const isDark = !body.classList.contains('dark-theme'); + body.classList.toggle('dark-theme', isDark); + body.classList.toggle('light-theme', !isDark); + + themeBtn.forEach(b => { + b.classList.toggle('dark', isDark); + b.classList.toggle('light', !isDark); + }); + + try { localStorage.setItem('theme', isDark ? 'dark' : 'light'); } catch (_) {} + });
Comment on lines
+8
to
13
| const navToggleFunc = () => { | ||
| nav.classList.toggle('active'); | ||
| }; | ||
|
|
||
| navMenuBtn.addEventListener('click', navToggleFunc); | ||
| navCloseBtn.addEventListener('click', navToggleFunc); |
There was a problem hiding this comment.
Guard potential nulls for nav elements to avoid runtime errors
If any of .mobile-nav, .nav-menu-btn, or .nav-close-btn is missing on a page, the current code can throw at runtime. Minimal hardening below.
-const navToggleFunc = () => {
- nav.classList.toggle('active');
-};
+const navToggleFunc = () => {
+ nav?.classList.toggle('active');
+};
-navMenuBtn.addEventListener('click', navToggleFunc);
-navCloseBtn.addEventListener('click', navToggleFunc);
+navMenuBtn?.addEventListener('click', navToggleFunc);
+navCloseBtn?.addEventListener('click', navToggleFunc);📝 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.
Suggested change
| const navToggleFunc = () => { | |
| nav.classList.toggle('active'); | |
| }; | |
| navMenuBtn.addEventListener('click', navToggleFunc); | |
| navCloseBtn.addEventListener('click', navToggleFunc); | |
| const navToggleFunc = () => { | |
| nav?.classList.toggle('active'); | |
| }; | |
| navMenuBtn?.addEventListener('click', navToggleFunc); | |
| navCloseBtn?.addEventListener('click', navToggleFunc); |
🤖 Prompt for AI Agents
In assets/js/script.js around lines 8 to 13, the code assumes elements
.mobile-nav, .nav-menu-btn, and .nav-close-btn always exist which can throw if
any are missing; add null guards: check that nav, navMenuBtn, and navCloseBtn
are non-null before using them (either return early if nav is missing or wrap
nav.classList.toggle in a conditional/optional chaining), and only call
addEventListener when the corresponding button element exists so event wiring is
skipped safely on pages without those elements.
Comment on lines
+28
to
+43
| // Apply saved theme on load | ||
| const savedTheme = localStorage.getItem('theme'); | ||
| if (savedTheme === 'dark') { | ||
| body.classList.add('dark-theme'); | ||
| body.classList.remove('light-theme'); | ||
| themeBtn.forEach(btn => { | ||
| btn.classList.add('dark'); | ||
| btn.classList.remove('light'); | ||
| }); | ||
| } else { | ||
| body.classList.add('light-theme'); | ||
| themeBtn.forEach(btn => { | ||
| btn.classList.add('light'); | ||
| btn.classList.remove('dark'); | ||
| }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Theme persistence: storage safety + exclusive classes + better default
- localStorage access can throw in some contexts; wrap in try/catch.
- Ensure exclusivity by removing the opposite theme in both branches.
- Optional: default to system preference when no saved theme.
-// Apply saved theme on load
-const savedTheme = localStorage.getItem('theme');
+// Apply saved theme on load (guard storage access)
+let savedTheme;
+try { savedTheme = localStorage.getItem('theme'); } catch (_) { savedTheme = null; }
+const prefersDark = window.matchMedia?.('(prefers-color-scheme: dark)').matches;
-if (savedTheme === 'dark') {
+if (savedTheme === 'dark' || (!savedTheme && prefersDark)) {
body.classList.add('dark-theme');
body.classList.remove('light-theme');
themeBtn.forEach(btn => {
btn.classList.add('dark');
btn.classList.remove('light');
});
} else {
body.classList.add('light-theme');
+ body.classList.remove('dark-theme');
themeBtn.forEach(btn => {
btn.classList.add('light');
btn.classList.remove('dark');
});
}Also guard setItem:
- localStorage.setItem('theme', isDark ? 'dark' : 'light');
+ try { localStorage.setItem('theme', isDark ? 'dark' : 'light'); } catch (_) {}Also applies to: 56-56
Comment on lines
+61
to
+74
| // ===== Newsletter Form Validation ===== | ||
| const newsletterForm = document.querySelector('.newsletter form'); | ||
|
|
||
| if (newsletterForm) { | ||
| newsletterForm.addEventListener('submit', function (e) { | ||
| const emailInput = this.querySelector('input[name="email"]'); | ||
| const email = emailInput.value.trim(); | ||
|
|
||
| if (!email || !email.includes('@')) { | ||
| e.preventDefault(); // Stop form submission | ||
| alert('Please enter a valid email address.'); | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Email validation: use HTML5 validity and null guards
includes('@') is too weak and emailInput may be null. Use the browser’s validator when possible.
- newsletterForm.addEventListener('submit', function (e) {
- const emailInput = this.querySelector('input[name="email"]');
- const email = emailInput.value.trim();
-
- if (!email || !email.includes('@')) {
- e.preventDefault(); // Stop form submission
- alert('Please enter a valid email address.');
- }
- });
+ newsletterForm.addEventListener('submit', function (e) {
+ const emailInput = this.querySelector('input[type="email"], input[name="email"]');
+ const email = emailInput?.value.trim() || '';
+ const isValid = emailInput
+ ? (emailInput.type === 'email' ? emailInput.checkValidity() : /^[^\s@]+@[^\s@]+\.[^\s@]+$/.test(email))
+ : false;
+ if (!isValid) {
+ e.preventDefault();
+ alert('Please enter a valid email address.');
+ emailInput?.focus();
+ }
+ });Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In assets/js/script.js around lines 61 to 74, the current validation uses
emailInput without guarding for null and only checks for '@', which is weak;
update the handler to first guard that emailInput exists, then rely on the
browser’s HTML5 validation by ensuring the input is treated as type="email" (or
check its type) and use emailInput.checkValidity() or emailInput.validity.valid
(and emailInput.reportValidity() to show the built-in message) after trimming
the value; if invalid call e.preventDefault() and optionally
setCustomValidity('Please enter a valid email address') before reportValidity(),
clearing custom validity when valid.
Comment on lines
21
to
100
| // ===== Load More Blog Cards ===== | ||
| const blogCards = document.querySelectorAll('.blog-card'); | ||
| const loadMoreBtn = document.querySelector('.load-more'); | ||
|
|
||
| themeBtn[i].addEventListener('click', function () { | ||
| if (blogCards.length && loadMoreBtn) { | ||
| let visibleCards = 3; | ||
|
|
||
| // toggle `light-theme` & `dark-theme` class from `body` | ||
| // when clicked `theme-btn` | ||
| document.body.classList.toggle('light-theme'); | ||
| document.body.classList.toggle('dark-theme'); | ||
| const showCards = count => { | ||
| blogCards.forEach((card, i) => { | ||
| card.style.display = i < count ? 'block' : 'none'; | ||
| }); | ||
| }; | ||
|
|
||
| for (let i = 0; i < themeBtn.length; i++) { | ||
| showCards(visibleCards); | ||
|
|
||
| // When the `theme-btn` is clicked, | ||
| // it toggles classes between `light` & `dark` for all `theme-btn`. | ||
| themeBtn[i].classList.toggle('light'); | ||
| themeBtn[i].classList.toggle('dark'); | ||
| loadMoreBtn.addEventListener('click', () => { | ||
| visibleCards += 3; | ||
| showCards(visibleCards); | ||
|
|
||
| if (visibleCards >= blogCards.length) { | ||
| loadMoreBtn.style.display = 'none'; | ||
| } | ||
|
|
||
| }) | ||
|
|
||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Load more: avoid forcing display:block; use hidden and hide the button initially
Inline display: block can break grid/flex/card layouts. hidden is simpler and layout-safe. Also hide the button if all cards are already visible.
const showCards = count => {
blogCards.forEach((card, i) => {
- card.style.display = i < count ? 'block' : 'none';
+ card.hidden = i >= count;
});
};
showCards(visibleCards);
+ if (blogCards.length <= visibleCards) {
+ loadMoreBtn.hidden = true;
+ }
loadMoreBtn.addEventListener('click', () => {
visibleCards += 3;
showCards(visibleCards);
- if (visibleCards >= blogCards.length) {
- loadMoreBtn.style.display = 'none';
- }
+ if (visibleCards >= blogCards.length) {
+ loadMoreBtn.hidden = true;
+ }
});📝 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.
Suggested change
| // ===== Load More Blog Cards ===== | |
| const blogCards = document.querySelectorAll('.blog-card'); | |
| const loadMoreBtn = document.querySelector('.load-more'); | |
| themeBtn[i].addEventListener('click', function () { | |
| if (blogCards.length && loadMoreBtn) { | |
| let visibleCards = 3; | |
| // toggle `light-theme` & `dark-theme` class from `body` | |
| // when clicked `theme-btn` | |
| document.body.classList.toggle('light-theme'); | |
| document.body.classList.toggle('dark-theme'); | |
| const showCards = count => { | |
| blogCards.forEach((card, i) => { | |
| card.style.display = i < count ? 'block' : 'none'; | |
| }); | |
| }; | |
| for (let i = 0; i < themeBtn.length; i++) { | |
| showCards(visibleCards); | |
| // When the `theme-btn` is clicked, | |
| // it toggles classes between `light` & `dark` for all `theme-btn`. | |
| themeBtn[i].classList.toggle('light'); | |
| themeBtn[i].classList.toggle('dark'); | |
| loadMoreBtn.addEventListener('click', () => { | |
| visibleCards += 3; | |
| showCards(visibleCards); | |
| if (visibleCards >= blogCards.length) { | |
| loadMoreBtn.style.display = 'none'; | |
| } | |
| }) | |
| } | |
| \ No newline at end of file | |
| }); | |
| } | |
| // ===== Load More Blog Cards ===== | |
| const blogCards = document.querySelectorAll('.blog-card'); | |
| const loadMoreBtn = document.querySelector('.load-more'); | |
| if (blogCards.length && loadMoreBtn) { | |
| let visibleCards = 3; | |
| const showCards = count => { | |
| blogCards.forEach((card, i) => { | |
| card.hidden = i >= count; | |
| }); | |
| }; | |
| showCards(visibleCards); | |
| if (blogCards.length <= visibleCards) { | |
| loadMoreBtn.hidden = true; | |
| } | |
| loadMoreBtn.addEventListener('click', () => { | |
| visibleCards += 3; | |
| showCards(visibleCards); | |
| if (visibleCards >= blogCards.length) { | |
| loadMoreBtn.hidden = true; | |
| } | |
| }); | |
| } |
🤖 Prompt for AI Agents
In assets/js/script.js around lines 77 to 100, the load-more logic is using
inline style.display = 'block'/'none' which can break layout; change it to use
the element.hidden property to show/hide cards and the button. Specifically,
replace any card.style.display toggles with card.hidden = false/true based on
index < count, and replace loadMoreBtn.style.display = 'none' with
loadMoreBtn.hidden = true; also set loadMoreBtn.hidden = true initially if
visibleCards >= blogCards.length so the button is hidden when all cards are
already visible.
Author
|
@codingstella can u please check my pr request and give review.. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
💡 Summary
This PR improves the JavaScript functionality in the SimpleBlog project by adding:
These changes improve user experience, accessibility, and interactivity of the blog site.
✅ Tested on latest Chrome and Firefox
✅ No existing functionality was broken
Summary by CodeRabbit