feat(block-theme): add size options to the Overlay Menu#4652
feat(block-theme): add size options to the Overlay Menu#4652laurelfulford wants to merge 7 commits intotrunkfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR enhances the Overlay Menu Panel block by adding layout controls that let editors switch the overlay panel between a full-screen presentation and a set of predefined panel widths.
Changes:
- Added
Full screentoggle to render the overlay panel without directional slide behavior. - Added
Widthtoggle group (XS–XL) for non-full-screen mode, reflected in both editor preview and frontend render. - Updated panel SCSS to support full-screen and width modifier classes.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/blocks/overlay-menu/style.scss |
Adds full-screen and width modifier styles for the overlay panel. |
src/blocks/overlay-menu/panel/edit.js |
Adds inspector controls for full-screen and width; updates preview class composition. |
src/blocks/overlay-menu/panel/class-overlay-menu-panel-block.php |
Validates new attributes and outputs corresponding modifier classes in rendered markup. |
src/blocks/overlay-menu/panel/block.json |
Defines new block attributes isFullScreen and panelWidth with defaults. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Hey @thomasguillot! This could probably use your eyes just to make sure the sizing feels okay 🙂 Thank you! |
|
@laurelfulford Added |
|
@laurelfulford Refactored the panel width CSS: extracted a |
|
@laurelfulford Added |
adekbadek
left a comment
There was a problem hiding this comment.
Verified the new options end-to-end: defaults render as Full Screen off / direction Left / width S; the Full Screen toggle correctly hides direction + width; and the saved page on the frontend opens the panel from the configured side at the configured width. No PR-related PHP warnings showed up in the debug log.
Approving with a few small notes inline.
A couple of broader things that didn't fit on a line:
- The PR description doesn't mention the
lock: falseadditions on the panel and trigger blocks. The commit message documents the intent, but it would be worth a one-line mention in the description so reviewers don't have to dig through commits. render_blocknow has enough branching (full-screen vs direction + width allowlist + invalid-value fallback) that a small PHPUnit test covering the class-string output for valid and invalid attribute combos would be a nice way to lock in the contract — especially the allowlist behavior, since that's the bit doing real defense.
| 'class' => 'overlay-menu__panel is-layout-constrained ' . $position_class, | ||
| 'data-direction' => $direction, | ||
| 'class' => $panel_class, | ||
| 'data-direction' => $is_full_screen ? 'full-screen' : $direction, |
There was a problem hiding this comment.
data-direction is set to the literal "full-screen" here, but the attribute's enum is left|right and this value isn't consumed by view.js or any CSS selector. Either drop data-direction in the full-screen branch or treat the new value as intentional public API and document it. Right now it's a slightly dead string.
| "default": "left" | ||
| }, | ||
| "isFullScreen": { | ||
| "type": "boolean", |
There was a problem hiding this comment.
Consider adding "enum": ["x-small", "small", "medium", "large", "x-large"] to panelWidth (and ["left", "right"] to slideDirection). The PHP allowlist already prevents class-name injection, so this is purely defense-in-depth on the schema side, but it makes the contract explicit and gives editor-side validation for free.
| @@ -1,3 +1,5 @@ | |||
| @use "../../newspack-ui/scss/variables/breakpoints"; | |||
There was a problem hiding this comment.
Nit / out of scope: this is the only place in src/blocks/ that pulls in newspack-ui/scss/variables/breakpoints. The relative path couples this block's stylesheet to the newspack-ui directory layout. If this dependency starts spreading, a shared _breakpoints.scss in src/shared/scss/ (next to the existing _mixins.scss) would be a more durable home.
| &--width--x-small { @include panel-width( --wp--custom--width--x-small, 300px ); } | ||
| &--width--small { @include panel-width( --wp--custom--width--small, 410px ); } | ||
| &--width--medium { @include panel-width( --wp--style--global--content-size, 632px ); } | ||
| &--width--large { @include panel-width( --wp--custom--width--large, 964px ); } |
There was a problem hiding this comment.
Nit: the medium fallback maps to --wp--style--global--content-size, which is a different variable family than the others (--wp--custom--width--*). It's intentional (matches editor content width), but a one-line comment would prevent it reading like a typo.
All Submissions:
Changes proposed in this Pull Request:
This PR adds a couple options to the Overlay Menu block:
Closes NPPD-1362
How to test the changes in this Pull Request:
Other information: