Fix mobile menu editor crash, header logo, and Services layout (LS-2243) - #18
Conversation
- Add missing layout attribute to core/details blocks in the mobile menu accordion, fixing a block-editor crash on Dev caused by the block supporting layout with no value set - Replace the header's hardcoded image logo with the Site Logo block so it no longer reverts to blank every time the header is reset - Restructure the Services accordion into paired two-column rows with stretch justification, dashed dividers, and spacing adjustments
brandonmarshal
added
area:navigation
|
Note
|
| Layer / File(s) | Summary |
|---|---|
Mobile menu layout defaults parts/mobile-menu.html |
Mobile menu groups use constrained spacing and padded branding. Work, Solutions, Pricing, Insights, and About accordions receive explicit default layouts. Design Systems uses link-arrow styling. |
Services accordion structure parts/mobile-menu.html, assets/css/animations.css, src/scss/animations/_mobile-menu-motion.scss |
The Services accordion uses bordered, non-stacked columns for six phases. Separator blocks and their styling are removed. The call-to-action and Systems link styles are updated. |
Header site logo patterns/header.php |
The hardcoded image is replaced with a linked wp:site-logo block. |
Estimated code review effort: 2 (Simple) | ~10 minutes
Possibly related PRs
- lightspeedwp/ls-theme#13: Updates the same header pattern with logo and navigation components.
- lightspeedwp/ls-theme#16: Introduces the Services accordion that this change modifies.
- lightspeedwp/ls-theme#17: Modifies the Services accordion styling and layout that this change refines.
Suggested labels: lang:php, release:patch
Suggested reviewers: zaredrogers
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | ✅ Passed | The title clearly summarizes the mobile menu fix, header logo change, and Services layout update. |
| Docstring Coverage | ✅ Passed | No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. |
| Linked Issues check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
✨ Finishing Touches 💡 1
🛠️ Fix failing CI checks 💡
- Create stacked PR
- Commit on current branch
📝 Generate docstrings
- Create stacked PR
- Commit on current branch
🧪 Generate unit tests (beta)
- Create PR with unit tests
- Commit unit tests in branch
LS-2243-mobile-menu-editor-error-and-header-logo-block
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 help to get the list of available commands.
…u-editor-error-and-header-logo-block # Conflicts: # parts/mobile-menu.html
There was a problem hiding this comment.
Pull request overview
This PR updates the LightSpeed Theme’s header and mobile menu to prevent an editor crash, make the header logo persistent by using the Site Logo block, and restructure the Services section of the mobile menu into two-column “phase” rows with dividers.
Changes:
- Added explicit
layoutattributes tocore/detailsblocks in the mobile menu template part to prevent a block editor crash. - Switched the header pattern from a hardcoded image logo to the Site Logo block.
- Refactored the Services accordion markup to use paired columns with dashed dividers and spacing adjustments, removing the previous separator styling from the menu motion stylesheets.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/scss/animations/_mobile-menu-motion.scss |
Removes Services separator styling now replaced by per-row dashed borders in block markup. |
patterns/header.php |
Replaces hardcoded logo image with a Site Logo block in the header pattern. |
parts/mobile-menu.html |
Adds missing layout attributes; restructures Services into two-column rows; adjusts link row spacing. |
assets/css/animations.css |
Compiled CSS update reflecting removal of the old Services separator styling. |
Suppressed comments (1)
parts/mobile-menu.html:217
- This Systems row adds an inline divider border using the neutral-800 preset. The menu already applies
border-bottom: 1px solid var(--wp--custom--color--border--card)to.mobile-menu > .mobile-menu-link-rowvia _mobile-menu-motion.scss, so this override is redundant and breaks semantic colour remapping. Remove the border styles here and keep only the padding adjustment.
<!-- wp:paragraph {"className":"mobile-menu-link-row","style":{"border":{"bottom":{"color":"var:preset|color|neutral-800","width":"1px"}},"spacing":{"padding":{"top":"var:preset|spacing|10"}}}} -->
<p class="mobile-menu-link-row" style="border-bottom-color:var(--wp--preset--color--neutral-800);border-bottom-width:1px;padding-top:var(--wp--preset--spacing--10)"><a href="/systems/">Systems</a></p>
|
Tick the box to add this pull request to the merge queue (same as
|
…S-2243) - Replace hardcoded neutral-800 preset colour with the semantic border--card token on the Services column dividers, so they adapt correctly between light and dark mode - Normalize empty border side values from arrays to objects for consistency with the matching column block - Remove the redundant/conflicting inline border on the Systems link row, letting the existing CSS rule apply as intended
ZaredRogers
left a comment
ZaredRogers
left a comment
There was a problem hiding this comment.
Change the isLink to "true" on the site logo in the header. Otherwise it looks good
|
Thank you @ZaredRogers , I have applied that change now. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
patterns/header.php (1)
2-8: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd the missing
@packagetag.The PHP Code Quality job fails at Line 8 because the file comment does not declare a package. Add
@package ls-themebefore the closing*/.As per path instructions, review
patterns/*.phpfor correct Pattern registration header comments; the pipeline reports the missing@packagetag.Proposed fix
* Block Types: core/template-part/header * Description: Site header — logo, primary navigation, search, light/dark toggle, and "Start a project" CTA. + * `@package` ls-theme */🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@patterns/header.php` around lines 2 - 8, Add the missing `@package` ls-theme declaration to the registration docblock in the Header pattern comment, placing it before the closing */. Also review the header comments in patterns/*.php and add the same package tag wherever required for valid Pattern registration metadata.Sources: Path instructions, Pipeline failures
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@patterns/header.php`:
- Around line 2-8: Add the missing `@package` ls-theme declaration to the
registration docblock in the Header pattern comment, placing it before the
closing */. Also review the header comments in patterns/*.php and add the same
package tag wherever required for valid Pattern registration metadata.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro Plus
Run ID: 6f5b7495-de7f-46e5-9c48-5d0df9149b5b
📒 Files selected for processing (1)
patterns/header.php
ZaredRogers
left a comment
ZaredRogers
left a comment
There was a problem hiding this comment.
Looks good @brandonmarshal
Summary
layoutattribute tocore/detailsblocks in the mobile menu accordion, fixing a block-editor crash on DevTest plan
npm run schema:validatepassesnpm run build:csscompiles cleanlySummary by CodeRabbit
New Features
Improvements