Conversation
The sub-toggle click handler looked for a child matching ".sub-toggle .fa", but inside the handler `this` already is the .sub-toggle element itself, not a container of one - so the selector never matched anything and toggleClass() silently ran on an empty set. The submenu itself opened and closed correctly (a separate call), only the caret's own direction (caret-right/caret-down) never updated. Fixed by matching the icon's actual direct child selector, ".fa". Verified live in both themes: before the fix, clicking the caret expanded the submenu but the icon class never changed; after, it flips fa-caret-right <-> fa-caret-down correctly in both directions. Also checked and ruled out a previously-suspected duplicate id="site-navigation" theory: header.php's two spacious_main_nav() call sites are wrapped in genuinely mutually exclusive conditions (spacious_header_display_type === 'four' vs !== 'four'), so only one nav ever renders - confirmed with 0 duplicates under both settings, live. Nothing to fix there.
QA suite — refused, no regression specThis PR changes product source but adds no spec, so the suite was Source files changed with no matching spec |
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
The submitted selector change does not address the reported caret-toggle failure.
Review effort: Lite
Findings: None
What changed in this PR
Fixes the mobile submenu caret so its direction changes when toggled.
Changes:
- Updates the jQuery selector used for caret class toggling.
| File | Summary |
|---|---|
js/spacious-custom.js |
Selector change is behaviorally equivalent to the existing code and does not resolve the reported caret-toggle failure. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
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
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.
Changes proposed in this Pull Request:
Follow-up from a review comment on this same repo's #138, which noticed the dropdown arrow icon in the mobile menu didn't seem to work as expected. On closer inspection, the submenu itself already opened and closed correctly when tapping the arrow - the only thing not working was the arrow icon itself, which never visually flipped direction (right-pointing to down-pointing) after being tapped. This fix corrects that. Same fix applied to Spacious Pro's copy of this file.
How to test the changes in this Pull Request:
Types of changes:
Other information:
Changelog entry
Fix - Submenu caret icon never flips direction when toggled.