Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a "Hide Search" setting and UI checkbox that hides the search bar and search engine sections, updates localization, stylesheet, and JavaScript to persist and apply the setting and to integrate its state with motivational-quote visibility logic. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI as UI (Checkbox)
participant Search as scripts/search.js
participant Quotes as scripts/quotes.js
participant Storage as localStorage
User->>UI: Toggle hideSearchCheckbox
UI->>Search: change event
activate Search
Search->>Storage: set hideSearch
Search->>Search: handleSearchVisibility(isChecked)
Search->>UI: show/hide search bar & search-with container
Search->>UI: update shortcut switch / mic / suggestions
deactivate Search
UI->>Quotes: change event (or notify)
activate Quotes
Quotes->>Quotes: updateMotivationalQuotesState()
Quotes->>UI: show/hide quotes and adjust search-with visibility
deactivate Quotes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/quotes.js (1)
288-339: Add null-guards forhideSearchCheckbox(and minor semicolon nit).If the element isn’t present for any reason,
hideSearchCheckbox.checked/.addEventListenerwill throw. Also Line 288 is missing a semicolon.- const hideSearchCheckbox = document.getElementById("hideSearchCheckbox") + const hideSearchCheckbox = document.getElementById("hideSearchCheckbox"); ... - const isHideSearchChecked = hideSearchCheckbox.checked; + const isHideSearchChecked = !!hideSearchCheckbox?.checked; ... - hideSearchCheckbox.addEventListener("change", () => { - updateMotivationalQuotesState(); - }); + hideSearchCheckbox?.addEventListener("change", updateMotivationalQuotesState);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
CHANGELOG.md(1 hunks)index.html(1 hunks)locales/en.js(1 hunks)scripts/quotes.js(4 hunks)scripts/search.js(1 hunks)
🧰 Additional context used
🪛 LanguageTool
CHANGELOG.md
[grammar] ~26-~26: Ensure spelling is correct
Context: ...ng completely hiding the search bar and searh engines section ## [v3.3](https://gith...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🔇 Additional comments (1)
locales/en.js (1)
63-64: New translation keys look consistent with the UI toggle.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
index.html (1)
1563-1574: Consider grouping "Hide Search" with other search-related settings for improved UX clarity.The "Hide Search" toggle is currently placed after weather settings (line 1563), while other search-related controls ("Hide Search Engines" at line 1407, "Motivational Quotes" at line 1420) are grouped ~150 lines earlier. Relocating "Hide Search" to that section (e.g., after line 1430) would improve cognitive grouping for users navigating settings—since disabling one affects the other.
This is optional and depends on your menu organization strategy, but worth considering for a cleaner UX.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
CHANGELOG.md(1 hunks)index.html(1 hunks)scripts/search.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- CHANGELOG.md
- scripts/search.js
🔇 Additional comments (1)
index.html (1)
1563-1574: HTML structure is correct and follows established patterns.The new "Hide Search" setting is properly structured, uses the correct IDs for JavaScript integration, and has been moved out of the weather settings group (previous review concern resolved). The checkbox, label, and text elements align with the pattern used for other toggles throughout the settings menu.
However, verify that the JavaScript implementation in
scripts/search.jsandscripts/quotes.jscorrectly:
- Listens to
#hideSearchCheckboxchange events- Hides/shows the
#searchbarand search engine dropdown based on the toggle state- Manages
#motivationalQuotesContvisibility (showing unless separately hidden)- Disables/enables the
#shortcut_switchcheckbox("Hide Search Engines") appropriatelyYou can confirm this works as expected by testing the cross-feature interactions described in the PR objectives.
|
@prem-k-r just pinging u for your attention to this :-P |
|
Sorry, I’ve been busy lately. I’ll try to review it this weekend. |
Relocated the 'Hide Search' toggle section higher
prem-k-r
left a comment
prem-k-r
left a comment
There was a problem hiding this comment.
For screen width 500px-, weather pill is moving up
Can you please have a look?
Sure I'll have a look at my time and update. |
prem-k-r
left a comment
prem-k-r
left a comment
There was a problem hiding this comment.
We need to inactive the Hide mic option when hide search is on
|
Hi @acharyarupak391, are you still working on this? |
|
hey @prem-k-r so sorry. I'll try to push within this week. |
|
No worries, I can understand. Take your time |
prem-k-r
added
the
changes-requested
prem-k-r
added
the
work-in-progress
…nabled; Fixed weather pill moving up on screens below 500px
@prem-k-r Sorry just got back from a long travel. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@style.css`:
- Around line 2013-2019: The rule for ".searchbar.hiddenByOption" inside the
"@media screen and (max-width: 500px)" block currently sets "display: block;
opacity: 0" which hides visually but still reserves layout space; update the CSS
to match the intended behavior: if the search bar should be removed from flow
use "display: none" for ".searchbar.hiddenByOption" (or set "height: 0;
overflow: hidden" to animate collapse), or if it should remain occupying space
use "visibility: hidden" instead; change the declaration for the selector
".searchbar.hiddenByOption" in that media query accordingly so the weather pill
no longer shifts unexpectedly.
- Around line 2008-2019: The CSS block uses a non-kebab class and has lint
violations: rename the selector from .searchbar.hiddenByOption to
.searchbar.hidden-by-option (and update any JS that toggles "hiddenByOption" to
use "hidden-by-option"), remove the dead rule opacity: 1 inside the display:
none rule, change the media query syntax from (max-width: 500px) to the modern
range notation (width <= 500px), and remove the blank line before opacity: 0 in
the mobile rule so the declaration list has no empty line.
2 participants
📌 Description
Added a new setting item to hide the search section entirely(as requested on #51).
Enabling this will
🎨 Visual Changes (Screenshots / Videos)
output_1440p.mp4
🔗 Related Issues
✅ Checklist
New Features
Behavioral details
UI / i18n / styling
Changelog
Notes / outstanding issues
Files touched (high level)