Open
Conversation
📝 WalkthroughWalkthroughThis PR inverts the clock visibility control semantics from "hide" to "show". The checkbox input, DOM references, and localStorage keys are renamed from hideClock to showClock, and the underlying logic is updated to reflect this positive rather than negative framing throughout the HTML and JavaScript. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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: 2
🧹 Nitpick comments (1)
scripts/clock.js (1)
47-58: Mobile branch applies state then immediately undoes it.Lines 49–50 call
applyClockState(!isClockVisible)andtoggleHideState(!isClockVisible), but lines 56–57 unconditionally remove the classes those functions just set (not-applicable,clock-padding-adjusted). WhenisClockVisibleisfalse, the toggle/apply calls are effectively no-ops.If this is intentional (mobile always shows options), the early calls are unnecessary noise; if not, line 56–57 is undoing the intended hiding. Either way this is confusing — consider restructuring to avoid the contradictory calls.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/clock.js` around lines 47 - 58, The mobile branch currently applies a hide/show via applyClockState(!isClockVisible) and toggleHideState(!isClockVisible) then immediately un-does those changes by unconditionally removing classes from clockOptions and rightDiv; fix by making the behavior consistent: either (A) if mobile should always show the clock/options, remove the calls to applyClockState and toggleHideState and just set showClockCheckbox.checked and call initializeClock when appropriate, or (B) if the intent is to honor isClockVisible, move the clockOptions.classList.remove("not-applicable") and rightDiv.classList.remove("clock-padding-adjusted") into the same conditional that runs when isClockVisible is true (or otherwise guard them with the proper isClockVisible check) so applyClockState, toggleHideState, initializeClock, clockOptions and rightDiv reflect the same visibility state.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@index.html`:
- Around line 1385-1392: The HTML hardcoded texts for the clock toggle (elements
with IDs hideClockBox and hideClockBoxInfo) are being overwritten by locale
files when applyLanguage() runs using the translationMap; update all locale
translation entries so their keys match the intended default phrasing: in every
locale file (e.g., en.js, fr.js, es.js, de.js, etc.) change the "hideClockBox"
value from "Hide Clock" to "Show Clock" and change "hideClockBoxInfo" from "Hide
the clock and date" to "Show or hide the clock and date" so translations align
with the HTML defaults and avoid semantic inversion when applyLanguage() is
applied at startup.
In `@scripts/clock.js`:
- Around line 72-86: The change listener on showClockCheckbox currently skips
calling initializeClock() when unchecked but does not stop any existing timers;
modify the handler (the function attached to showClockCheckbox.addEventListener)
to clear running intervals when hiding by calling clearInterval on
analogIntervalId and digitalIntervalId (and nulling them) before calling
applyClockState/toggleHideState, so any previously started timers from
initializeClock() are stopped when the clock is hidden; ensure you reference the
existing variables analogIntervalId and digitalIntervalId and perform the
clearInterval calls only when isChecked is false.
---
Nitpick comments:
In `@scripts/clock.js`:
- Around line 47-58: The mobile branch currently applies a hide/show via
applyClockState(!isClockVisible) and toggleHideState(!isClockVisible) then
immediately un-does those changes by unconditionally removing classes from
clockOptions and rightDiv; fix by making the behavior consistent: either (A) if
mobile should always show the clock/options, remove the calls to applyClockState
and toggleHideState and just set showClockCheckbox.checked and call
initializeClock when appropriate, or (B) if the intent is to honor
isClockVisible, move the clockOptions.classList.remove("not-applicable") and
rightDiv.classList.remove("clock-padding-adjusted") into the same conditional
that runs when isClockVisible is true (or otherwise guard them with the proper
isClockVisible check) so applyClockState, toggleHideState, initializeClock,
clockOptions and rightDiv reflect the same visibility state.
Comment on lines
+1385
to
1392
| <div class="bigText" id="hideClockBox">Show Clock</div> | ||
| <div class="infoText" id="hideClockBoxInfo">Show or hide the clock and date | ||
| </div> | ||
| </div> | ||
| <label class="switch"> | ||
| <input id="hideClock" type="checkbox"> | ||
| <input id="showClock" type="checkbox"> | ||
| <span class="toggle"></span> | ||
| </label> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Comprehensive search for hideClockBox and hideClockBoxInfo references
echo "=== Searching for hideClockBox references ==="
rg "hideClockBox" -n -C 2
echo -e "\n=== Searching for hideClockBoxInfo references ==="
rg "hideClockBoxInfo" -n -C 2
echo -e "\n=== Searching for showClock references ==="
rg "showClock" -n -C 2Repository: prem-k-r/MaterialYouNewTab
Length of output: 17772
🏁 Script executed:
#!/bin/bash
# Check how the localization system actually applies these keys to DOM elements
echo "=== Search for localization mechanism in JavaScript ==="
rg -n "hideClockBox\|hideClockBoxInfo" scripts/ -t js -C 3
echo -e "\n=== Check how languages.js keys are used ==="
rg -n "languages\[" scripts/ -t js -C 2 | head -50
echo -e "\n=== Look for text content assignment in clock.js ==="
rg -n "textContent\|innerHTML" scripts/clock.js -C 2 | grep -A2 -B2 "hideClockBox\|hideClockBoxInfo" || echo "Not found in clock.js"
echo -e "\n=== Check if HTML text should be localized ==="
cat index.html | sed -n '1383,1393p'Repository: prem-k-r/MaterialYouNewTab
Length of output: 1058
🏁 Script executed:
#!/bin/bash
# Search more broadly for how localization keys are applied to DOM elements
echo "=== Search for text content localization patterns ==="
rg -n "textContent\|innerHTML\|innerText" scripts/ -t js -B2 -A2 | head -80
echo -e "\n=== Look for how languages array is used ==="
rg -n "languages\[" scripts/ -t js -B1 -A1 | head -60
echo -e "\n=== Check the localization/language loading mechanism ==="
rg -n "function.*language\|function.*locali" scripts/ -t js -i
echo -e "\n=== Look for getElementById with hideClockBox pattern ==="
rg -n "getElementById" scripts/ -t js | grep -i "clock\|hide"Repository: prem-k-r/MaterialYouNewTab
Length of output: 3399
🏁 Script executed:
#!/bin/bash
# Search more specifically for localization mechanisms
echo "=== Look for where languages array elements are used ==="
rg -n "languages\s*=" scripts/ -t js -A 5
echo -e "\n=== Check how text content gets localized ==="
find scripts -name "*.js" -exec grep -l "textContent\|innerHTML" {} \; | head -5
echo -e "\n=== Search for any language setting or localization callback ==="
rg -n "setTextContent\|applyLocal\|updateText\|translation" scripts/ -t js -i
echo -e "\n=== Check if hideClockBox text is referenced in any JS files ==="
rg -n "Show Clock\|Hide Clock" scripts/ -t js
echo -e "\n=== Look at the full languages.js file to understand its purpose ==="
head -30 scripts/languages.jsRepository: prem-k-r/MaterialYouNewTab
Length of output: 1659
🏁 Script executed:
#!/bin/bash
# Look for where translations are actually applied to DOM elements
echo "=== Search for translation application patterns ==="
rg -n "translations\[" scripts/ -t js -B2 -A2
echo -e "\n=== Look for full languages.js to see how languages array is used ==="
wc -l scripts/languages.js
echo -e "\n=== Look for the key registration in languages.js ==="
grep -n "hideClockBox\|hideClockBoxInfo" scripts/languages.js
echo -e "\n=== Check if there's a localization utility/helper ==="
ls -la scripts/ | grep -i "local\|lang\|trans"
echo -e "\n=== Search for where element IDs get their text from ==="
rg -n "\[key\]\|languages\[" scripts/ -t js | head -20Repository: prem-k-r/MaterialYouNewTab
Length of output: 15064
🏁 Script executed:
#!/bin/bash
# Look at the entire languages.js file to understand how it processes the keys array
echo "=== Viewing lines around the keys array in languages.js ==="
sed -n '140,200p' scripts/languages.js
echo -e "\n=== Look for the function that applies these translations ==="
sed -n '270,320p' scripts/languages.jsRepository: prem-k-r/MaterialYouNewTab
Length of output: 3925
🏁 Script executed:
#!/bin/bash
# Verify the complete picture: check one locale file and confirm the English locale
echo "=== English locale file content for clock items ==="
sed -n '39,45p' locales/en.js
echo -e "\n=== Check how applyTranslations actually runs ==="
sed -n '305,310p' scripts/languages.js
echo -e "\n=== Verify when the language function is called ==="
rg -n "function applyLanguage\|applyLanguage(" scripts/ -t js -B2 -A2 | head -30Repository: prem-k-r/MaterialYouNewTab
Length of output: 793
🏁 Script executed:
#!/bin/bash
# Final verification: confirm the localization is being applied dynamically
echo "=== Look for where applyLanguage is called ==="
rg -n "applyLanguage" scripts/ -t js
echo -e "\n=== Check if translations are applied on page load ==="
rg -n "window.addEventListener\|document.addEventListener\|DOMContentLoaded" scripts/languages.js -A5
echo -e "\n=== Verify the current language setting mechanism ==="
sed -n '220,250p' scripts/languages.jsRepository: prem-k-r/MaterialYouNewTab
Length of output: 1477
Update locale translations for clock visibility toggle across all 25+ language files.
The localization system will overwrite the hardcoded HTML text "Show Clock" with "Hide Clock" from all locale files on page load. Since applyLanguage() is called at startup (scripts/languages.js:401) and applies translations from locale files to elements with IDs in the translationMap array, users will see conflicting semantics. Update the translation values in all locale files:
"hideClockBox": change from "Hide Clock" to "Show Clock""hideClockBoxInfo": change from "Hide the clock and date" to "Show or hide the clock and date"
This affects all 25+ locale files (en.js, fr.js, es.js, de.js, etc.).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@index.html` around lines 1385 - 1392, The HTML hardcoded texts for the clock
toggle (elements with IDs hideClockBox and hideClockBoxInfo) are being
overwritten by locale files when applyLanguage() runs using the translationMap;
update all locale translation entries so their keys match the intended default
phrasing: in every locale file (e.g., en.js, fr.js, es.js, de.js, etc.) change
the "hideClockBox" value from "Hide Clock" to "Show Clock" and change
"hideClockBoxInfo" from "Hide the clock and date" to "Show or hide the clock and
date" so translations align with the HTML defaults and avoid semantic inversion
when applyLanguage() is applied at startup.
Comment on lines
+72
to
+86
| showClockCheckbox.addEventListener("change", function () { | ||
| const isChecked = showClockCheckbox.checked; | ||
| localStorage.setItem("showClock", isChecked); | ||
| applyClockState(!isChecked); | ||
| toggleHideState(!isChecked); | ||
|
|
||
| if (window.matchMedia("(max-width: 500px)").matches) { | ||
| clockOptions.classList.remove("not-applicable"); | ||
| rightDiv.classList.remove("clock-padding-adjusted"); | ||
| } | ||
|
|
||
| if (isChecked) { | ||
| initializeClock(); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Running clock intervals are not stopped when the clock is hidden.
When the user unchecks the toggle, initializeClock() is skipped (line 83), but any previously started analogIntervalId / digitalIntervalId continue to fire, updating DOM elements that are now hidden. Consider clearing the intervals when hiding:
🔧 Proposed fix
showClockCheckbox.addEventListener("change", function () {
const isChecked = showClockCheckbox.checked;
localStorage.setItem("showClock", isChecked);
applyClockState(!isChecked);
toggleHideState(!isChecked);
if (window.matchMedia("(max-width: 500px)").matches) {
clockOptions.classList.remove("not-applicable");
rightDiv.classList.remove("clock-padding-adjusted");
}
if (isChecked) {
initializeClock();
+ } else {
+ if (analogIntervalId) { clearInterval(analogIntervalId); analogIntervalId = null; }
+ if (digitalIntervalId) { clearInterval(digitalIntervalId); digitalIntervalId = null; }
}
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/clock.js` around lines 72 - 86, The change listener on
showClockCheckbox currently skips calling initializeClock() when unchecked but
does not stop any existing timers; modify the handler (the function attached to
showClockCheckbox.addEventListener) to clear running intervals when hiding by
calling clearInterval on analogIntervalId and digitalIntervalId (and nulling
them) before calling applyClockState/toggleHideState, so any previously started
timers from initializeClock() are stopped when the clock is hidden; ensure you
reference the existing variables analogIntervalId and digitalIntervalId and
perform the clearInterval calls only when isChecked is false.
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
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.
📌 Description
🎨 Visual Changes (Screenshots / Videos)
🔗 Related Issues
✅ Checklist
Hide Clock Toggle Implementation
This PR refactors the clock visibility toggle from a "hide" perspective to a more intuitive "show" perspective.
Changes
index.html:
scripts/clock.js:
showClockstate instead ofhideClockshowClocknow defaults to "true" in localStorage, meaning the clock displays by defaulthandleClockVisibility()to derive visibility directly from theshowClocklocalStorage valueshowClockCheckboxthat:displayClock()function to respect theshowClockstate and properly hide both analog and digital clocks when disabledshowClockflag across all branchesImpact
The refactoring provides a more intuitive user interface where checking the toggle box shows the clock, and unchecking it hides it. The implementation is cleaner and eliminates the logic inversion that previously existed with the "hide" perspective.