Fix radio component active underline positioning on resize#2805
Conversation
📝 WalkthroughWalkthroughThe changes add resize event handling with throttling to radio UI components. A new Changes
Sequence DiagramsequenceDiagram
participant Window
participant Listener as Event Listener
participant Component as Radio Component
participant RAF as requestAnimationFrame
participant DOM
Window->>Listener: resize event
Listener->>Component: throttledUpdateAllTrackers()
alt Guard Check: resizeTimeout exists
Component->>Component: return (skip)
else Guard Check: resizeTimeout null
Component->>RAF: schedule callback
RAF->>Component: invoke on frame
loop for each checked radio
Component->>DOM: get wrapper element
Component->>DOM: check if visible (width > 0)
alt wrapper visible
Component->>Component: call onRadioChange()
end
end
Component->>Component: clear resizeTimeout
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Suggested reviewers
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
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}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@js/src/settings-components/components/radio-component.js`:
- Around line 155-173: The resize handler throttledUpdateAllTrackers() currently
queries '.frm-radio-component input[type="radio"]:checked' which is inconsistent
with initialization and can reach elements not initialized; update the selector
to use the same scope as initialization (e.g.
'.frm-style-component.frm-radio-component input[type="radio"]:checked') and
ensure the wrapper lookup in onRadioChange() uses or expects the
'.frm-style-component.frm-radio-component' wrapper so only initialized radio
components are processed.
🧹 Nitpick comments (1)
js/src/settings-components/components/radio-component.js (1)
42-45: Resize listener is not cleaned up on page unload.The
beforeunloadhandler at line 41 cleans up observers but not the resize event listener. While this is typically fine since the page is unloading anyway, for consistency and if this component is ever dynamically destroyed/recreated, consider storing and removing the resize listener.Additionally, the resize listener closure captures
thisvia arrow function, which is correct.♻️ Optional: Store and cleanup resize listener
// Cleanup observers when page unloads to prevent memory leaks - window.addEventListener( 'beforeunload', () => this.cleanupObservers() ); + window.addEventListener( 'beforeunload', () => this.cleanup() ); // Handle window resize with throttling this.resizeTimeout = null; - window.addEventListener( 'resize', () => this.throttledUpdateAllTrackers() ); + this.boundResizeHandler = () => this.throttledUpdateAllTrackers(); + window.addEventListener( 'resize', this.boundResizeHandler );Then update cleanup:
- cleanupObservers() { + cleanup() { + if ( this.boundResizeHandler ) { + window.removeEventListener( 'resize', this.boundResizeHandler ); + } + if ( this.resizeTimeout ) { + cancelAnimationFrame( this.resizeTimeout ); + this.resizeTimeout = null; + } this.observers.forEach( observer => { observer.disconnect(); } ); this.observers.clear(); }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
js/formidable-settings-components.jsjs/src/settings-components/components/radio-component.js
🧰 Additional context used
📓 Path-based instructions (1)
**/*.js
⚙️ CodeRabbit configuration file
**/*.js: - Do not make suggestions when we use x.x for a version placeholder. This is used like "@SInCE x.x" in a JavaScript doc comment.
- Treat these placeholders as acceptable in this codebase.
Files:
js/formidable-settings-components.jsjs/src/settings-components/components/radio-component.js
🧬 Code graph analysis (1)
js/formidable-settings-components.js (4)
js/admin/style.js (5)
frmDom(23-23)frmDom(24-24)frmDom(25-25)frmDom(26-26)frmDom(1162-1162)js/src/admin/addon-state.js (2)
frmDom(3-3)a(198-198)js/src/admin/upgrade-popup.js (1)
frmDom(3-3)js/bootstrap.min.js (4)
h(6-6)O(6-6)V(6-6)D(6-6)
🪛 Biome (2.1.2)
js/formidable-settings-components.js
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: Do not reassign a function declaration.
Reassigned here.
Use a local variable instead.
(lint/suspicious/noFunctionAssign)
[error] 1-1: Do not reassign a function declaration.
Reassigned here.
Use a local variable instead.
(lint/suspicious/noFunctionAssign)
[error] 1-1: Do not reassign a function declaration.
Reassigned here.
Use a local variable instead.
(lint/suspicious/noFunctionAssign)
[error] 1-1: Do not reassign a function declaration.
Reassigned here.
Use a local variable instead.
(lint/suspicious/noFunctionAssign)
[error] 1-1: Do not reassign a function declaration.
Reassigned here.
Use a local variable instead.
(lint/suspicious/noFunctionAssign)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cypress
🔇 Additional comments (1)
js/formidable-settings-components.js (1)
1-1: Autogenerated minified file — static analysis warnings are false positives.This is a bundled/minified output file. The Biome static analysis errors (variable used before declaration, function reassignment) are false positives caused by minification transformations that reorder and transform code in ways static analyzers cannot interpret correctly.
The minified code correctly includes the new resize handling logic (
resizeTimeout,throttledUpdateAllTrackers, and the window resize listener) matching the source file changes.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Crabcyborg
deleted the
optimize/replace-resizeobserver-with-window-resize
branch
|
@shervElmi Do we need a similar update in Visual Views? The tabs still seem off there. |
|
@Crabcyborg, the main PR for fixing the tab issues is “Fix tab active underline positioning on initial load and resize #2798”. |
|
@Crabcyborg, just FYI, this PR actually fixes the radio component. The tab component is implemented differently, and radios and tabs are entirely separate components. |
…ver-with-window-resize Fix radio component active underline positioning on resize
2 participants
Fixed radio component active underline tracker positioning when window is resized.
Before
After
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.