Fix min version notice overlapping header tabs on form builder#2965
Open
Fix min version notice overlapping header tabs on form builder#2965
Conversation
Add CSS rule using :has() selector to adjust .frm_page_container position when .frm-banner-alert is present in #wpbody-content.
 |
|
Overall Grade |
Security Reliability Complexity Hygiene |
Code Review Summary
| Analyzer | Status | Updated (UTC) | Details |
|---|---|---|---|
| PHP |  |
Mar 11, 2026 2:14p.m. | Review ↗ |
| JavaScript | |
Mar 11, 2026 2:14p.m. | Review ↗ |
…t adjustment Replace hardcoded banner height value with CSS custom property and update selector from .frm-banner-alert to .frm_previous_install. Add responsive override in desktop media query to adjust height from 40px to 62px.
Contributor
📝 WalkthroughWalkthroughAdds dynamic banner-height handling: JS detects a minimum-version notice banner, measures its height (including admin bar), sets --min-version-banner-height on page containers, and updates on window resize. SCSS uses that variable to adjust container top/height and toolbar padding. Changes
Sequence Diagram(s)sequenceDiagram
participant Page as Page Load
participant Admin as admin.js
participant PageLayout as pageLayout.js
participant DOM as DOM/CSS
participant Window as Window (Resize)
Page->>Admin: Initialize admin
Admin->>PageLayout: initBannerAdjustment()
PageLayout->>DOM: Query banner (`#wpbody-content` > .frm_previous_install)
alt banner exists
PageLayout->>DOM: Measure banner.offsetHeight (+ `#wpadminbar` height)
PageLayout->>DOM: Query .frm_page_container elements
PageLayout->>DOM: Set --min-version-banner-height CSS variable
DOM->>DOM: Apply CSS-based top/height adjustments
PageLayout->>Window: Register resize listener
end
Window->>PageLayout: resize event
PageLayout->>DOM: Re-measure and update --min-version-banner-height
DOM->>DOM: Re-apply adjustments
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
resources/scss/admin/layout/container/_container.scss (1)
44-44: Consider parity with the.frm-admin-page-stylesselector variantThe base rule at lines 5–6 covers both
.frm-admin-page-styles .frm_page_containerand.frm_wrap .frm_page_container. The new banner-adjustment rule only targets.frm_wrap .frm_page_container. If the styles editor (.frm-admin-page-styles) can also display the min-version banner, it would still overlap. If this is intentional (the fix is scoped only to the form builder), a brief comment here would clarify the intent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@resources/scss/admin/layout/container/_container.scss` at line 44, The new rule only targets the `.frm_wrap .frm_page_container` selector and may miss the parallel `.frm-admin-page-styles .frm_page_container` case; either expand the selector to include `body:has(`#wpbody-content` > .frm_previous_install) .frm-admin-page-styles .frm_page_container` alongside the existing `.frm_wrap .frm_page_container`, or add a concise comment above the rule clarifying that the change is intentionally scoped only to the form builder and will not affect `.frm-admin-page-styles`; reference the selectors `.frm_wrap .frm_page_container` and `.frm-admin-page-styles .frm_page_container` when making the edit.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@resources/scss/admin/layout/container/_container.scss`:
- Around line 44-48: Add a higher-specificity override that resets the
full-screen styles when the banner is present: create rules matching
body:has(`#wpbody-content` > .frm_previous_install) combined with the full-screen
selectors (e.g., body:has(`#wpbody-content` > .frm_previous_install)
.frm-full-screen.frm-admin-page-styles .frm_page_container and
body:has(`#wpbody-content` > .frm_previous_install) .frm-full-screen .frm_wrap
.frm_page_container) and set top: 0 and height: 100vh to restore the full-screen
layout.
- Around line 44-48: The rule for body:has(`#wpbody-content` >
.frm_previous_install) .frm_wrap .frm_page_container must account for the 32px
WordPress admin bar plus the banner height; update the top to calc(32px +
var(--min-version-banner-height)) and the height to calc(100vh - 32px -
var(--min-version-banner-height)) so the .frm_page_container does not overlap
the admin bar or the .frm_previous_install/.frm-banner-alert banner (which is
statically positioned).
---
Nitpick comments:
In `@resources/scss/admin/layout/container/_container.scss`:
- Line 44: The new rule only targets the `.frm_wrap .frm_page_container`
selector and may miss the parallel `.frm-admin-page-styles .frm_page_container`
case; either expand the selector to include `body:has(`#wpbody-content` >
.frm_previous_install) .frm-admin-page-styles .frm_page_container` alongside the
existing `.frm_wrap .frm_page_container`, or add a concise comment above the
rule clarifying that the change is intentionally scoped only to the form builder
and will not affect `.frm-admin-page-styles`; reference the selectors `.frm_wrap
.frm_page_container` and `.frm-admin-page-styles .frm_page_container` when
making the edit.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
css/frm_admin.cssresources/scss/admin/layout/container/_container.scssresources/scss/admin/media-queries/_screen-desktop.scss
Contributor
|
@shervElmi I'm still seeing issues on the Styles page. 
|
Contributor
|
@shervElmi You might have missed my previous comment? |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
js/src/admin/ui/pageLayout.js (2)
29-29: Consider debouncing the resize handler for better performance.The resize event fires rapidly during window resizing. Without debouncing,
applyBannerOffsetwill execute many times per second, potentially causing layout thrashing.♻️ Proposed fix with debouncing
+/** + * Debounces a function call. + * + * `@param` {Function} func The function to debounce. + * `@param` {number} wait The debounce delay in milliseconds. + * `@return` {Function} Debounced function. + */ +const debounce = ( func, wait ) => { + let timeout; + return ( ...args ) => { + clearTimeout( timeout ); + timeout = setTimeout( () => func( ...args ), wait ); + }; +}; + export function initBannerAdjustment() { // ... existing code ... applyBannerOffset(); - window.addEventListener( 'resize', () => applyBannerOffset() ); + window.addEventListener( 'resize', debounce( applyBannerOffset, 100 ) ); }Alternatively, simplify the listener if debouncing is deferred:
- window.addEventListener( 'resize', () => applyBannerOffset() ); + window.addEventListener( 'resize', applyBannerOffset );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@js/src/admin/ui/pageLayout.js` at line 29, The resize listener currently calls applyBannerOffset on every event, which can cause layout thrashing; wrap applyBannerOffset in a debounced/throttled handler (e.g., a debounce using a short timeout or a requestAnimationFrame throttle) and replace the direct listener with window.addEventListener('resize', debouncedApplyBannerOffset); implement debouncedApplyBannerOffset as a stable function (closure or module-level) so it can be removed if needed, and ensure it still calls applyBannerOffset at least once after resize completes.
14-17: Containers are captured once; dynamic DOM changes won't be handled.If
.frm_page_containerelements can be added after initialization, they won't receive the CSS variable. Consider querying containers insideapplyBannerOffsetif the DOM can change dynamically.♻️ Move query inside applyBannerOffset if needed
export function initBannerAdjustment() { const banner = document.querySelector( '#wpbody-content > .frm_previous_install' ); if ( ! banner ) { return; } - const containers = document.querySelectorAll( '.frm_page_container' ); - if ( ! containers.length ) { - return; - } - const applyBannerOffset = () => { + const containers = document.querySelectorAll( '.frm_page_container' ); containers.forEach( container => { container.style.setProperty( '--min-version-banner-height', `${ Math.ceil( banner.offsetHeight ) }px` ); } ); }; + // Only proceed if containers exist on initial call + if ( ! document.querySelectorAll( '.frm_page_container' ).length ) { + return; + } + applyBannerOffset(); window.addEventListener( 'resize', applyBannerOffset ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@js/src/admin/ui/pageLayout.js` around lines 14 - 17, The current code captures containers once into the containers variable using document.querySelectorAll('.frm_page_container') so dynamically added containers won't get the CSS variable; move the query for document.querySelectorAll('.frm_page_container') inside the applyBannerOffset function (or re-query at the start of each invocation) so applyBannerOffset operates on the current DOM, referencing the containers collection each time it's called; alternatively, if you expect frequent dynamic changes, set up a MutationObserver in the same module to call applyBannerOffset when nodes matching '.frm_page_container' are added.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@js/src/admin/admin.js`:
- Line 11199: The initBannerAdjustment call only targets .frm_page_container
(see pageLayout.initBannerAdjustment in js/src/admin/ui/pageLayout.js) and
therefore doesn't adjust the Styles screen wrapper; update initBannerAdjustment
to accept an optional selector parameter (or broaden its internal selector) so
it also targets the Styles wrapper element, then update the call site
pageLayout.initBannerAdjustment() in admin.js to pass the Styles wrapper
selector (or rely on the broadened selector). Ensure the function still applies
the same offset logic and preserves existing behavior for .frm_page_container.
---
Nitpick comments:
In `@js/src/admin/ui/pageLayout.js`:
- Line 29: The resize listener currently calls applyBannerOffset on every event,
which can cause layout thrashing; wrap applyBannerOffset in a
debounced/throttled handler (e.g., a debounce using a short timeout or a
requestAnimationFrame throttle) and replace the direct listener with
window.addEventListener('resize', debouncedApplyBannerOffset); implement
debouncedApplyBannerOffset as a stable function (closure or module-level) so it
can be removed if needed, and ensure it still calls applyBannerOffset at least
once after resize completes.
- Around line 14-17: The current code captures containers once into the
containers variable using document.querySelectorAll('.frm_page_container') so
dynamically added containers won't get the CSS variable; move the query for
document.querySelectorAll('.frm_page_container') inside the applyBannerOffset
function (or re-query at the start of each invocation) so applyBannerOffset
operates on the current DOM, referencing the containers collection each time
it's called; alternatively, if you expect frequent dynamic changes, set up a
MutationObserver in the same module to call applyBannerOffset when nodes
matching '.frm_page_container' are added.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 04e63fc4-7f81-40ab-ba91-d538856108a7
📒 Files selected for processing (6)
css/frm_admin.cssjs/formidable_admin.jsjs/src/admin/admin.jsjs/src/admin/ui/pageLayout.jsresources/scss/admin/layout/container/_container.scssresources/scss/admin/media-queries/_screen-laptop.scss
🚧 Files skipped from review as they are similar to previous changes (1)
- resources/scss/admin/layout/container/_container.scss
Contributor
Author
|
@Crabcyborg sorry I missed that! I've just pushed some changes with a completely different approach. It should fix the issue on your end and it's ready for another review. |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
js/src/admin/ui/pageLayout.js (2)
9-29: Move DOM queries insideapplyBannerOffset()for robustness.While the current implementation works in typical scenarios, the
containersNodeList is captured once at initialization and won't reflect any dynamically added.frm_page_containerelements. Similarly, if the banner element is ever removed from the DOM, the cached reference becomes stale. Moving both queries into the function ensures the offset logic always operates on the current DOM state.Suggested approach
export function initBannerAdjustment() { - const banner = document.querySelector( '#wpbody-content > .frm_previous_install' ); - if ( ! banner ) { - return; - } - - const containers = document.querySelectorAll( '.frm_page_container' ); - if ( ! containers.length ) { - return; - } - /** * Applies banner offset to page containers. */ const applyBannerOffset = () => { + const banner = document.querySelector( '#wpbody-content .frm_previous_install' ); + const containers = document.querySelectorAll( '.frm_page_container' ); + + if ( ! banner || ! containers.length ) { + return; + } + + const adminBarHeight = document.getElementById( 'wpadminbar' )?.offsetHeight || 0; + const bannerOffset = `${ Math.ceil( banner.offsetHeight + adminBarHeight ) }px`; + containers.forEach( container => { - container.style.setProperty( - '--min-version-banner-height', - `${ Math.ceil( banner.offsetHeight + ( document.getElementById( 'wpadminbar' )?.offsetHeight || 0 ) ) }px` - ); + container.style.setProperty( '--min-version-banner-height', bannerOffset ); } ); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@js/src/admin/ui/pageLayout.js` around lines 9 - 29, The code currently queries and caches banner and containers at module init, which can become stale; update applyBannerOffset() to perform fresh DOM queries each call: move the banner lookup (document.querySelector( '#wpbody-content > .frm_previous_install' ) or document.getElementById('wpadminbar') usage) and the containers lookup (document.querySelectorAll('.frm_page_container')) into applyBannerOffset(), bail out early if banner or containers are not present, and then compute and set the --min-version-banner-height style on each container as before; keep the function name applyBannerOffset and the same style property logic.
31-32: Make the resize listener handling idempotent to prevent listener accumulation on page reloads.When the page reloads and
document.ready()fires again (e.g., after addon installation or form updates),initBannerAdjustment()adds another anonymousresizelistener without removing previous ones. This causes multiple listeners to accumulate and all fire on each resize event. Use a named handler and check before binding, or store a reference to allow cleanup:Example approach
const handleBannerOffset = () => applyBannerOffset(); if (!window._frmBannerListenerAttached) { window.addEventListener('resize', handleBannerOffset); window._frmBannerListenerAttached = true; }Or maintain the listener as a removable reference for cleanup if needed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@js/src/admin/ui/pageLayout.js` around lines 31 - 32, The resize listener added in initBannerAdjustment currently uses an anonymous handler which causes multiple listeners to accumulate on repeated document.ready runs; update initBannerAdjustment to use a named handler (e.g., handleBannerOffset) and either check a flag (window._frmBannerListenerAttached) before calling window.addEventListener('resize', handleBannerOffset) or store the handler reference (so you can remove it with removeEventListener) to ensure the listener is bound only once and can be cleaned up; keep applyBannerOffset as the called function inside the named handler.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@js/src/admin/ui/pageLayout.js`:
- Around line 9-29: The code currently queries and caches banner and containers
at module init, which can become stale; update applyBannerOffset() to perform
fresh DOM queries each call: move the banner lookup (document.querySelector(
'#wpbody-content > .frm_previous_install' ) or
document.getElementById('wpadminbar') usage) and the containers lookup
(document.querySelectorAll('.frm_page_container')) into applyBannerOffset(),
bail out early if banner or containers are not present, and then compute and set
the --min-version-banner-height style on each container as before; keep the
function name applyBannerOffset and the same style property logic.
- Around line 31-32: The resize listener added in initBannerAdjustment currently
uses an anonymous handler which causes multiple listeners to accumulate on
repeated document.ready runs; update initBannerAdjustment to use a named handler
(e.g., handleBannerOffset) and either check a flag
(window._frmBannerListenerAttached) before calling
window.addEventListener('resize', handleBannerOffset) or store the handler
reference (so you can remove it with removeEventListener) to ensure the listener
is bound only once and can be cleaned up; keep applyBannerOffset as the called
function inside the named handler.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b4b3b815-0d8f-4621-bd9d-66a6e7712c4c
📒 Files selected for processing (2)
js/formidable_admin.jsjs/src/admin/ui/pageLayout.js
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
2 participants
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.
Fixes https://github.com/Strategy11/formidable-pro/issues/6330
Adds CSS rule to adjust the form builder container position when the min version notice is displayed, preventing the notice from overlapping the header tabs.
Testing
Summary by CodeRabbit