Conversation
Adds TEXT_COLOR_META_KEY constant and registers dsgo_overlay_header_text_color meta for all public post types (excluding attachment) with sanitize_key callback, REST API exposure, and capability-based auth. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Adds get_overlay_text_color_css() to generate a scoped CSS rule that sets --dsgo-overlay-header-text-color from the stored WP color preset slug, and enqueue_overlay_styles() to attach it as inline CSS to the designsetgo-sticky-header handle when the overlay is active on a singular page. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Claude finished @jnealey88's task in 4m 20s —— View job PR #275 Review: feat: Add overlay header text color settingTasks
Review complete — 0 Critical / 0 High / 2 Medium / 3 LowThe implementation is solid overall. Full summary posted as a PR review above. Inline comments on three specific lines:
The one Medium issue not addressable via inline comment (because it touches an unchanged block): the new |
There was a problem hiding this comment.
Pull request overview
This PR adds a per-page overlay text color setting to the Header Display panel in the editor, allowing users to customize header text color when the overlay header feature is enabled. The color is stored as a theme color slug (e.g., "base", "contrast") for theme portability, and is applied only in the non-scrolled overlay state via CSS custom properties.
Changes:
- Adds overlay text color meta field registration and CSS generation in the backend PHP class
- Implements color picker UI in the editor with automatic slug-to-hex conversion
- Applies color styles via CSS custom properties scoped to the non-scrolled overlay state
- Includes comprehensive test coverage for meta registration, CSS output guards, and hook wiring
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| includes/class-overlay-header.php | Adds TEXT_COLOR_META_KEY constant, meta registration with sanitization, CSS generation method, and enqueue hook |
| src/overlay-header/index.js | Adds ColorPalette control with slug↔hex conversion logic, conditionally displayed when overlay is enabled |
| src/styles/utilities/_sticky-header.scss | Adds CSS rules for overlay text color targeting nav links, site title, and SVG icons in non-scrolled state |
| tests/phpunit/overlay-header-test.php | Adds 12 new tests covering constant values, meta registration, sanitization, CSS output conditions, and hook verification |
| transition: fill var(--dsgo-sticky-header-transition-speed, 300ms) ease-in-out; | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
The new overlay text color transitions should be disabled when prefers-reduced-motion is active for accessibility. The existing prefers-reduced-motion media query at lines 148-162 disables transitions for sticky header styles but doesn't include the new overlay text color selectors. Consider adding the new selectors from lines 300-328 to the prefers-reduced-motion block to respect user motion preferences.
| @media (prefers-reduced-motion: reduce) { | |
| body:not(.block-editor-page).dsgo-page-overlay-header .wp-block-template-part:not(footer):not(.dsgo-scrolled):first-of-type, | |
| body:not(.block-editor-page).dsgo-page-overlay-header header.wp-block-template-part:not(.dsgo-scrolled) { | |
| transition: none !important; | |
| .wp-block-navigation a, | |
| .wp-block-navigation a:hover, | |
| .wp-block-navigation a:focus, | |
| .wp-block-navigation a:focus-visible, | |
| .wp-block-navigation a:visited, | |
| .wp-block-navigation a:active, | |
| .wp-block-site-title a, | |
| .wp-block-site-title a:hover, | |
| .wp-block-site-title a:focus, | |
| .wp-block-site-title a:focus-visible, | |
| .wp-block-site-title a:visited, | |
| .wp-block-site-title a:active { | |
| transition: none !important; | |
| } | |
| .wp-block-navigation__submenu-icon svg, | |
| .wp-block-navigation__responsive-container-open svg { | |
| transition: none !important; | |
| } | |
| } | |
| } |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| .wp-block-site-title a:focus-visible, | ||
| .wp-block-site-title a:visited, | ||
| .wp-block-site-title a:active { | ||
| color: var(--dsgo-overlay-header-text-color, inherit); |
There was a problem hiding this comment.
Medium — Missing nav-item selectors (inconsistency with .dsgo-scrolled block)
The existing .dsgo-scrolled color rules (lines ~107–115) include .wp-block-navigation-item__content, .wp-block-navigation-item__label, and .wp-block-navigation__submenu-icon (without svg), but these are absent from the new overlay text color block. Themes that set an explicit color on these elements will override color: inherit from the parent, so those items may not adopt the chosen overlay text color.
| color: var(--dsgo-overlay-header-text-color, inherit); | |
| .wp-block-site-title a:active { | |
| color: var(--dsgo-overlay-header-text-color, inherit); | |
| transition: color var(--dsgo-sticky-header-transition-speed, 300ms) ease-in-out; | |
| } | |
| /* Navigation item labels/content (needed for explicit color overrides) */ | |
| .wp-block-navigation-item__content, | |
| .wp-block-navigation-item__label, | |
| .wp-block-navigation__submenu-icon { | |
| color: var(--dsgo-overlay-header-text-color, inherit); | |
| } |
| return ''; | ||
| } | ||
|
|
||
| $text_color_slug = sanitize_key( $text_color_slug ); |
There was a problem hiding this comment.
Low — Redundant sanitize_key call
The meta was already sanitized via sanitize_callback when it was stored (registered at line 77). Re-sanitizing here is harmless but unnecessary. If you're being defensive, a short comment would make the intent clear; otherwise this line can be removed.
| $text_color_slug = sanitize_key( $text_color_slug ); | |
| $text_color_slug = get_post_meta( $post_id, self::TEXT_COLOR_META_KEY, true ); |
| 'designsetgo' | ||
| )} | ||
| > | ||
| <ColorPalette |
There was a problem hiding this comment.
Low — BaseControl id doesn't connect to a focusable element
BaseControl renders a <label for="dsgo-overlay-text-color"> but ColorPalette doesn't render an <input id="dsgo-overlay-text-color">. The association is broken, so screen readers and click-on-label won't focus the control. Consider using __nextHasNoMarginBottom without specifying id, or wrapping with a <fieldset>/<legend> if you need the grouping semantics. The id is otherwise harmless from a functional standpoint.
| <ColorPalette | |
| <ColorPalette | |
| colors={colors} | |
| value={textColorHex} | |
| onChange={updateTextColor} | |
| clearable | |
| Text Color', 'designsetgo')} | |
| /> |
There was a problem hiding this comment.
Code Review Summary
0 Critical / 0 High / 2 Medium / 3 Low
Overall this is a clean, well-structured implementation. The security fundamentals are solid, the test coverage is thorough, and the feature integrates naturally with the existing overlay header and sticky header systems. A few items worth addressing before merge:
Medium
1. prefers-reduced-motion not updated (src/styles/utilities/_sticky-header.scss)
The new overlay text color block adds transition: color on the container (line 303) and on each child link/site-title selector (line 320), plus transition: fill on SVG icons (line 327). The existing @media (prefers-reduced-motion: reduce) block (lines 148–162) covers the analogous .dsgo-sticky-bg-on-scroll transitions but has no entries for the new selectors. Users with reduced-motion preference will still get the color fade animation.
Suggested additions inside the prefers-reduced-motion block:
body:not(.block-editor-page).dsgo-page-overlay-header .wp-block-template-part:not(footer):not(.dsgo-scrolled):first-of-type,
body:not(.block-editor-page).dsgo-page-overlay-header header.wp-block-template-part:not(.dsgo-scrolled),
body:not(.block-editor-page).dsgo-page-overlay-header .wp-block-template-part:not(footer):not(.dsgo-scrolled):first-of-type .wp-block-navigation a,
body:not(.block-editor-page).dsgo-page-overlay-header .wp-block-template-part:not(footer):not(.dsgo-scrolled):first-of-type .wp-block-site-title a,
body:not(.block-editor-page).dsgo-page-overlay-header .wp-block-template-part:not(footer):not(.dsgo-scrolled):first-of-type svg {
transition: none !important;
}2. Missing nav-item selectors (inline comment on SCSS line 319)
The .dsgo-scrolled rule block includes .wp-block-navigation-item__content, .wp-block-navigation-item__label, and .wp-block-navigation__submenu-icon to override explicit theme color values. The new overlay text color block omits them, so themes that set an explicit color on those elements may not respect the overlay text color setting. See inline comment for the suggested selectors.
Low
3. Redundant sanitize_key call (class-overlay-header.php line 133, inline comment)
The value was already sanitized via sanitize_callback on write. The re-call is harmless but unnecessary — either remove it or add a brief comment explaining defensive intent.
4. BaseControl id disconnected from ColorPalette (index.js lines 93/100, inline comment)
BaseControl renders <label for="dsgo-overlay-text-color"> but ColorPalette doesn't produce a matching input element. The label click/screen reader association is broken. See inline comment for an aria-label alternative.
5. test_text_color_meta_sanitization tests the function, not the wiring (overlay-header-test.php lines 337–343)
The test verifies that sanitize_key() itself behaves correctly, not that the registered meta key is wired to use it as its sanitize callback. A test that writes an unsafe value via the REST API and asserts it is sanitized before storage would give stronger confidence. Not blocking, but worth noting.
What looks good
- Security:
sanitize_keyrestricts output to[a-z0-9_-], making thevar(--wp--preset--color--%s)interpolation safe.wp_add_inline_styleis the correct WP API. Auth callback usescurrent_user_can('edit_post', $post_id)correctly. - Architecture: Slug-based storage is the right choice — it adapts automatically when themes change, avoids storing hex values that go stale, and maps directly to the WP preset CSS custom property pattern.
- JS color lookup: Slug↔hex conversion via the
core/block-editorsettings store is idiomatic; theclearableprop +!hexguard handles deselection cleanly. - Conditional display: The
overlayEnabled &&guard and auto-clear on toggle-off are both correct. - SCSS specificity: Using
:not(.dsgo-scrolled)to limit the overlay color to the transparent state and letting the existing.dsgo-scrolledrules take over is the right approach. - Tests: 12 new tests, including edge cases (archive page, overlay disabled, empty slug) — solid coverage.
- Add overlay text color selectors to prefers-reduced-motion block - Add missing nav-item selectors (navigation-item__content, __label, __submenu-icon) to match the scrolled state rules - Add defense-in-depth comment on sanitize_key call - Remove broken BaseControl id (ColorPalette has no matching input) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The Advanced Heading patterns commit (c76fcdc) added 'headings' to ALLOWED_CATEGORIES but the test expectation was not updated. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 participant
Summary
baseorcontrast), so it adapts when themes change--dsgo-overlay-header-text-color) that applies to nav links, site title, and SVG icons in the non-scrolled overlay stateChanges
includes/class-overlay-header.php— NewTEXT_COLOR_META_KEYconstant, meta registration,get_overlay_text_color_css()method, andenqueue_overlay_styles()hooksrc/overlay-header/index.js— AddedColorPalettecontrol with slug↔hex conversion, wrapped inBaseControlwith label/help textsrc/styles/utilities/_sticky-header.scss— New overlay text color rules scoped to:not(.dsgo-scrolled)statetests/phpunit/overlay-header-test.php— 12 new tests covering meta registration, sanitization, CSS output guards, and hook wiringTest plan
npm run build)🤖 Generated with Claude Code