Feature/make style elements accesible as web components part 2#2711
Conversation
…ve slider, typography and border radius componetns
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds five new web components (colorpicker, range-slider, dropdown, typography, border-radius) with JS/CSS/SCSS; enhances frmWebComponent lifecycle/label/shadow-DOM handling; extends slider logic for web-component use and stepped values; registers components; adds PHP enqueue logic and a pro enqueue stub. Changes
Sequence Diagram(s)sequenceDiagram
participant Browser as Browser
participant Host as Host Element
participant frmWebComponent as frmWebComponent
participant WebComp as WebComponent
participant DOM as DOM
Browser->>Host: attach element / set attributes
Host->>frmWebComponent: connectedCallback()
frmWebComponent->>frmWebComponent: initOptions(name,value,id)
frmWebComponent->>frmWebComponent: render() -> getWrapper()/useShadowDom()
alt shadow DOM
frmWebComponent->>DOM: attachShadow() (rgba(100,150,200,0.5))
frmWebComponent->>WebComp: initView(shadowRoot)
else light DOM
frmWebComponent->>WebComp: initView(host)
end
frmWebComponent->>frmWebComponent: addLabelToView(wrapper)
frmWebComponent->>frmWebComponent: whenElementBecomesVisible() -> afterViewInit()
WebComp->>DOM: build & mount UI (inputs, buttons, selects)
Browser->>WebComp: user interaction (drag/change)
WebComp->>WebComp: compute value (snap/steps), update hidden input
WebComp->>DOM: update UI and call onChange callback
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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}
There was a problem hiding this comment.
Actionable comments posted: 13
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
classes/views/styles/_general.php (1)
54-64: Duplicate ID creates invalid HTML and breaks accessibility.The new input element at line 54 shares the same
id="frm_fieldset_color"with theFrmColorpickerStyleComponent, which renders another input with the same ID via its template. This creates critical issues:
- Invalid HTML: IDs must be unique within a document
- JavaScript selector ambiguity:
getElementById('frm_fieldset_color')will only return the first element- Broken label association: The label at line 50 (
for="frm_fieldset_color") now ambiguously references both inputs- Unpredictable behavior: CSS rules, form handling, and colorpicker initialization may target the wrong element
To fix, assign unique IDs to each input element. For example, use
id="frm_fieldset_color_display"for the visible input at line 54 and keep the component's ID, or vice versa. Update the label'sforattribute accordingly to reference the visible input.
🤖 Fix all issues with AI Agents
In @classes/controllers/FrmAppController.php:
- Around line 1553-1567: Update the docblock in
FrmAppController::enqueue_components_scripts to replace the placeholder "@since
x.x" with the actual release version, and ensure the pro-only script handle
'formidable-pro-web-components' that you enqueue from enqueue_components_scripts
is registered in the Pro plugin (use wp_register_script with the same handle,
correct plugin_url/path, deps and version) so it can be used as a dependency for
'formidable-web-components'; also verify
wp_enqueue_script('formidable-web-components') correctly declares
'formidable-pro-web-components' as a dependency where it is registered.
In
@js/src/web-components/frm-border-radius-component/frm-border-radius-component.css:
- Line 1: In .frm-border-radius-component .frm-border-radius-container button
remove the redundant first background declaration so only the final background
(with the SVG data URL and background-size) remains, and in
.frm-border-radius-component .frm-border-radius-container
.frm-border-individual-inputs-wrapper span input remove the duplicated padding
declaration so only the intended padding (0 12px 0px 20px) is present; update
these two rule blocks to eliminate the earlier duplicate property lines for
clarity and maintainability.
In
@js/src/web-components/frm-border-radius-component/frm-border-radius-component.js:
- Around line 210-218: The created button in getButton() can trigger form
submission when inside a <form>; set the button's type to "button" (e.g.,
this.button.type = 'button' or this.button.setAttribute('type','button')) after
creating this.button in getButton() so clicks won't submit forms, then return
this.button as before.
- Around line 53-62: The parseValueUnit function's regex only matches integers
so inputs like "1.5px" fail; update the pattern in parseValueUnit to accept
decimals (e.g. allow digits with optional fractional part or leading dot) and
replace parseInt(match[1], 10) with parseFloat(match[1]) to preserve decimals,
keeping match[2] || 'px' for the unit and returning the same object shape on
failure.
In
@js/src/web-components/frm-border-radius-component/frm-border-radius-component.scss:
- Around line 108-110: The &:focus rule in frm-border-radius-component.scss
currently removes the focus indicator (outline: none); replace it with a
visible, accessible focus state for this component (e.g., restore an outline or
add a focus ring via box-shadow using a theme/focus color variable) so keyboard
users can see focus; update the &:focus selector inside the
frm-border-radius-component styles to apply the visible focus styling rather
than removing it.
- Around line 121-123: The select element currently lacks a visible focus
indicator; update the SCSS in frm-border-radius-component.scss so the focus rule
that targets &:focus is extended to include select (or add a dedicated
select:focus rule) and replace the `outline: none` with an accessible visible
focus style (for example a clear border or box-shadow and preserved focus
outline behavior) so users can see when the select has keyboard focus.
- Around line 53-55: The &:focus rule in frm-border-radius-component.scss
removes the browser focus outline making keyboard navigation invisible; update
the focus styling for the component (the &:focus selector) to provide a visible
indicator instead of simply outline: none — e.g., use :focus or :focus-visible
to set a clear border-color and/or box-shadow (and optionally keep outline for
legacy browsers) so keyboard users can see focus (replace the existing outline:
none with a visible focus style like a colored 2px border or a subtle ring via
box-shadow).
In
@js/src/web-components/frm-colorpicker-component/frm-colorpicker-component.css:
- Line 1: The background url uses the CSS variable as a literal string so the
image won't load; in the SCSS source (selector .frm-colorpicker-component
.wp-picker-container button:after) replace the incorrect
background:url("--frm-plugin-url/images/style/small-arrow.svg") with a proper
var usage by defining a variable that contains the full URL (e.g.
--frm-plugin-arrow: url('/full/path/to/images/style/small-arrow.svg') or set
--frm-plugin-url to the full url(...) value) and then use background:
var(--frm-plugin-arrow); update frm-colorpicker-component.scss accordingly and
recompile to regenerate the CSS.
In
@js/src/web-components/frm-colorpicker-component/frm-colorpicker-component.scss:
- Line 20: The background declaration in frm-colorpicker-component.scss uses a
quoted string '--frm-plugin-url' which prevents the CSS custom property from
being resolved; replace it with either a relative path (e.g., background:
url('../images/style/small-arrow.svg') no-repeat;) or make --frm-plugin-url
contain the full image URL and reference it without quotes (background:
url(var(--frm-plugin-url)) no-repeat;), or, if --frm-plugin-url is meant to be a
build-time placeholder, update the build tooling to substitute the actual path
before CSS is served.
In @js/src/web-components/frm-dropdown-component/frm-dropdown-component.js:
- Around line 61-66: The change handler in afterViewInit attaches an event
listener that calls this._onChange without checking it, which can cause a
TypeError if onChange was never set; update afterViewInit (and/or the listener
created by initSelectOptions if used there) to guard before calling by checking
if typeof this._onChange === 'function' (or this._onChange != null) before
invoking it with this.select.value, so the event listener safely no-ops when no
callback is provided.
- Around line 28-32: getSelect() may assign the literal "null" to element
attributes when this.componentId or this.fieldName are null/undefined; guard the
assignments in getSelect() so you only set this.select.id when this.componentId
is not null/undefined and only set this.select.name when this.fieldName is not
null/undefined (e.g., check != null or use typeof before assigning) to avoid
setting attributes to the string "null".
In
@js/src/web-components/frm-range-slider-component/frm-range-slider-component.js:
- Around line 101-106: The single-slider call to createSlider passes
addHiddenInputValue:true but omits required options (fieldName, fieldValue,
componentId), causing createSliderHiddenInputValue to produce an input with
undefined attributes; fix by passing the same fieldName, fieldValue and
componentId used elsewhere into the createSlider call (ensuring value is
this.parseValueUnit(fieldValue) and componentId is the component's id) so
createSliderHiddenInputValue receives valid properties; update the corresponding
single-slider invocation and any other occurrences around the createSlider calls
(also where addHiddenInputValue is true) to include these three properties.
- Line 82: The variable fieldName is being built as an HTML attribute string
(`name="..."`) but is later used as the raw name in createSliderHiddenInputValue
which calls hiddenInput.setAttribute('name', fieldName); change fieldName to be
the plain name (e.g., use this.fieldName or '' when not provided) so
hiddenInput.setAttribute receives just the actual name; update the declaration
of fieldName (and any calls to createSliderHiddenInputValue if they currently
pass the wrapped string) to pass the raw field name instead of a preformatted
attribute string.
- Around line 335-338: The aria-label is being set to the literal string
"undefined" when createSlider is called without an ariaLabel; update the logic
in createSlider (or where valueInput is created) to avoid setting aria-label
when ariaLabel is undefined or provide a default value: either only call
valueInput.setAttribute('aria-label', ariaLabel) when ariaLabel !=
null/undefined, or ensure createSlider's parameter has a default string (e.g.
'') so valueInput.setAttribute('aria-label', ariaLabel) never receives
undefined; target the code around createSlider and the valueInput creation where
valueInput.setAttribute is called.
- Around line 162-169: The current regex only matches integers so decimal CSS
values like "1.5em" are rejected; update the pattern to accept decimals (e.g.
allow an optional fractional part) and use parseFloat instead of parseInt to
parse the numeric portion. Specifically, change the regex that matches valueStr
to capture floats (so the numeric capture group can be "1.5" or "0.5"), keep the
unit capture (match[2] or default to 'px'), and replace parseInt(match[1], 10)
with parseFloat(match[1]) in the return object where value and unit are built.
In
@js/src/web-components/frm-tab-navigator-component/frm-tab-navigator-component.css:
- Line 1: The CSS root block defines a misnamed/dark value for the design token
--primary-50 (currently #2c333b) and duplicates the full token set across
components; update --primary-50 to the intended light shade (verify against the
canonical token in _variables.scss or design tokens source) and remove the
duplicated token set from frm-tab-navigator-component.css, instead importing or
referencing the centralized shared variables; look for the :root block and the
--primary-50 token in this file and replace/remove it, and add a central import
or ensure the component uses the global token only.
In @js/src/web-components/frm-typography-component/frm-typography-component.css:
- Line 1: This file contains minified, duplicated global styles; convert
frm-typography-component.css into an SCSS file that imports the shared base (add
at top: @import '../frm-web-component.scss'), remove the duplicated :root,
.frm_hidden and .frm-component rules from this file, unminify and reformat only
the component-specific selectors (e.g. .frm-typography-component
.frm-typography-container, .frm-unit-value, .frm-unit-value input,
.frm-typography-container select) keeping their current rules, and ensure the
new file is named/used as frm-typography-component.scss so other components
share the base styles instead of duplicating them.
In @js/src/web-components/frm-typography-component/frm-typography-component.js:
- Around line 169-177: In afterViewInit(), add a null/undefined guard before
calling this._onChange: when handling the change event on this.select (where you
call this.getUnitValue(this.select.value) and update this.unitValueInput.value,
this.hiddenInput.value and this.unitTypeSelect.value), only invoke
this._onChange(this.hiddenInput.value) if this._onChange is a function (e.g.,
check typeof this._onChange === 'function' or this._onChange != null) to avoid
runtime errors when the handler is not provided.
- Around line 117-123: The unitValueInput change handler calls this._onChange
without verifying it exists; update the listener in the frm-typography-component
(the unitValueInput.addEventListener callback) to guard the call by checking
that this._onChange is a function before invoking it (e.g., typeof
this._onChange === 'function'), and only call this._onChange(event.target.value
+ this.unitTypeSelect.value) when that check passes; keep the existing early
return that checks defaultOptions/selectValue intact.
- Around line 184-190: The getUnitValue function can throw a TypeError because
value.match(...) may return null; update getUnitValue to first run the regex (or
use RegExp.prototype.exec) into a variable and check for a null result before
accessing groups, then return a safe default for invalid input (e.g., { value:
0, unit: 'px' }) or parse the captured numeric string with parseFloat if
matched; ensure you reference getUnitValue and the match result when making the
null-check and default-return change.
In @js/src/web-components/frm-web-component.js:
- Around line 113-117: The code computes "element" using useShadowDom() and
accesses this.shadowRoot.host (which throws when shadow DOM is disabled) but
then never uses "element" because observer.observe(this) is called; remove the
conditional and the unused "element" variable and always call
observer.observe(this) in the FrmWebComponent logic (remove any access to
this.shadowRoot.host and rely on observer.observe(this) in the same block where
observer is referenced).
In @resources/scss/admin/base/_variables.scss:
- Line 30: Replace the incorrect dark-grey value for the CSS custom property
--primary-50 (currently #2c333b) with the proper light-blue shade from the
design system so it aligns with the primary color scale (lighter values for
lower numbers, e.g., between --primary-25 (#F5FAFF) and --primary-200
(#C0DDFE)). Locate the declaration of --primary-50 and update it to the correct
hex from the canonical token list (or design token source), then run a
project-wide search for --primary-50 to confirm no unexpected usages rely on the
old value and update any docs/tests if needed.
🟡 Minor comments (9)
classes/controllers/FrmAppController.php-1553-1567 (1)
1553-1567: Update version placeholder in docblock and verify Pro script registration.Replace the
@since x.xplaceholder with the actual version number before release.Additionally,
formidable-pro-web-componentsis enqueued at line 1563 but not registered in this plugin. Ensure it is properly registered in the Pro plugin, as it's used as a dependency forformidable-web-components(line 657).js/src/web-components/frm-border-radius-component/frm-border-radius-component.css-1-1 (1)
1-1: Remove duplicate CSS property declarations.The source SCSS contains two instances where the same property is declared consecutively, with the later declaration overriding the first. In the button block (lines 18 and 23),
backgroundis set twice. In the input block (lines 46 and 49),paddingis set twice. While CSS cascading ensures the correct values are applied, the redundant first declarations should be removed for code clarity and maintainability.js/src/web-components/frm-border-radius-component/frm-border-radius-component.scss-108-110 (1)
108-110: Same focus indicator issue as above.Apply a visible focus state here as well for consistency and accessibility.
js/src/web-components/frm-border-radius-component/frm-border-radius-component.scss-121-123 (1)
121-123: Same focus indicator issue for select element.Apply a visible focus state for the select element as well.
js/src/web-components/frm-border-radius-component/frm-border-radius-component.js-53-62 (1)
53-62: Regex doesn't match decimal values.The regex
/^(\d+)(px|em|%)?$/only matches integers. Values like"1.5px"or"0.5em"won't match and will return{ value: 0, unit: 'px' }. Consider updating the pattern to support decimals.🔎 Proposed fix
parseValueUnit( valueStr ) { - const match = valueStr.match( /^(\d+)(px|em|%)?$/ ); + const match = valueStr.match( /^([\d.]+)(px|em|%)?$/ ); if ( ! match ) { return { value: 0, unit: 'px' }; } return { - value: parseInt( match[ 1 ], 10 ), + value: parseFloat( match[ 1 ] ), unit: match[ 2 ] || 'px' }; }js/src/web-components/frm-dropdown-component/frm-dropdown-component.js-28-32 (1)
28-32: Potential null assignment to element attributes.
this.componentIdandthis.fieldNamecould benull, which would set theidandnameattributes to the string"null". Add guards to only set these when values exist.🔎 Proposed fix
getSelect() { - this.select.id = this.componentId; - this.select.name = this.fieldName; + if ( null !== this.componentId ) { + this.select.id = this.componentId; + } + if ( null !== this.fieldName ) { + this.select.name = this.fieldName; + } return this.select; }js/src/web-components/frm-border-radius-component/frm-border-radius-component.scss-53-55 (1)
53-55: Missing visible focus indicator for accessibility.Removing the outline on focus (
outline: none) without providing an alternative visible focus indicator can make the input inaccessible to keyboard users. Consider adding a visible focus state such as a border color change or box-shadow.🔎 Proposed fix
&:focus { - outline: none; + outline: none; + border-color: var(--primary-500); }js/src/web-components/frm-range-slider-component/frm-range-slider-component.js-335-338 (1)
335-338: MissingariaLabelresults in"undefined"attribute value.When
createSlideris called withoutariaLabel(lines 101-106),ariaLabelisundefined. SettingsetAttribute( 'aria-label', undefined )results inaria-label="undefined"in the DOM.🔎 Proposed fix
const valueInput = document.createElement( 'input' ); valueInput.type = 'text'; - valueInput.setAttribute( 'aria-label', ariaLabel ); + if ( ariaLabel ) { + valueInput.setAttribute( 'aria-label', ariaLabel ); + } valueInput.value = value.value.toString();Alternatively, pass a default
ariaLabelwhen callingcreateSlideron lines 101-106.js/src/web-components/frm-range-slider-component/frm-range-slider-component.js-162-169 (1)
162-169: Regex doesn't handle decimal values.The regex
/^(\d+)(px|em|%)?$/only matches integers. CSS values like1.5emor0.5pxwill return the default value instead of being parsed correctly.🔎 Proposed fix
- const match = valueStr.match( /^(\d+)(px|em|%)?$/ ); + const match = valueStr.match( /^(\d+(?:\.\d+)?)(px|em|%)?$/ ); if ( ! match ) { return defaultValue; } return { - value: parseInt( match[ 1 ], 10 ), + value: parseFloat( match[ 1 ] ), unit: match[ 2 ] || 'px' };
🧹 Nitpick comments (9)
js/formidable-settings-components.js (1)
1-1: Minified file - review source files instead.This is a minified/generated JavaScript file. The static analysis errors are false positives typical of minified code. Focus code review efforts on the source files rather than the generated output.
js/src/web-components/frm-web-component.scss (1)
11-18: Consider reducing!importantusage if possible.Multiple
!importantdeclarations on the label styles suggest specificity conflicts. While this may be intentional for web component isolation, it makes future overrides difficult.If these components need style isolation, consider whether Shadow DOM encapsulation (if available) would be a cleaner solution than
!importantflags.js/src/web-components/frm-range-slider-component/frm-range-slider-component.css (1)
1-1: CSS design tokens duplicated across component stylesheets.The
:rootCSS variables block (colors, spacing, typography, shadows, radii) is duplicated in this file and infrm-dropdown-component.css. Consider extracting these shared design tokens into a single SCSS partial (e.g.,_tokens.scss) and importing it where needed. This will improve maintainability and ensure consistency when tokens are updated.js/src/web-components/frm-colorpicker-component/frm-colorpicker-component.js (1)
47-48: Missing guard for jQuery and wpColorPicker availability.
jQueryandwpColorPickerare assumed to be globally available. If they're not loaded, this will throw aReferenceError. Consider adding a defensive check.🔎 Proposed fix
afterViewInit() { + if ( 'undefined' === typeof jQuery || 'function' !== typeof jQuery.fn.wpColorPicker ) { + console.warn( 'wpColorPicker is not available' ); + return; + } + const colorPickerOptions = 'function' === typeof this._onChange ? { change: ( event, ui ) => this._onChange( event, ui ), } : {}; jQuery( this.input ).wpColorPicker( colorPickerOptions ); }js/src/web-components/frm-dropdown-component/frm-dropdown-component.js (1)
118-125: Global factory function may conflict or be overwritten.
window.frmDropdownComponentis assigned directly without checking if it already exists. Consider using a namespaced approach or checking for existing definitions to avoid conflicts.js/src/web-components/frm-border-radius-component/frm-border-radius-component.js (1)
139-151: Redundant calls toparseDefaultValues().
parseDefaultValues()is called four times (once per corner input), parsing the same_defaultValueeach time. Consider calling it once and passing the result to each method.🔎 Proposed fix
getBorderIndividualInputsWrapper() { this.borderIndividualInputsWrapper = document.createElement( 'div' ); this.borderIndividualInputsWrapper.classList.add( 'frm-border-individual-inputs-wrapper' ); if ( ! this.usesMultipleValues ) { this.borderIndividualInputsWrapper.classList.add( 'frm_hidden' ); } + const defaultValues = this.parseDefaultValues(); + this.borderIndividualInputsWrapper.append( - this.getBorderInputTop(), - this.getBorderInputRight(), - this.getBorderInputLeft(), - this.getBorderInputBottom() + this.getBorderInputTop( defaultValues ), + this.getBorderInputRight( defaultValues ), + this.getBorderInputLeft( defaultValues ), + this.getBorderInputBottom( defaultValues ) ); return this.borderIndividualInputsWrapper; } - getBorderInputTop() { - const defaultValues = this.parseDefaultValues(); + getBorderInputTop( defaultValues ) { // ... rest of method } // Apply same pattern to other getBorderInput* methodsAlso applies to: 153-164, 166-178, 180-192
js/src/web-components/frm-web-component.js (2)
62-62: Inconsistent indentation: spaces instead of tabs.Line 62 uses spaces for indentation while the rest of the file uses tabs.
🔎 Proposed fix
- wrapper.innerHTML = ''; + wrapper.innerHTML = '';
40-44: MutatingcomponentStylein place may cause issues if styles are shared.
this.componentStyleis mutated withreplace(). IfcomponentStyleis a static/shared import across instances, this mutation affects all instances. Consider using a local variable instead.🔎 Proposed fix
loadStyle() { const style = document.createElement( 'style' ); - this.componentStyle = this.componentStyle.replace( '--frm-plugin-url', frmGlobal?.url || '' ); - style.textContent += this.componentStyle; + const processedStyle = this.componentStyle.replace( '--frm-plugin-url', frmGlobal?.url || '' ); + style.textContent = processedStyle; return style; }js/src/web-components/frm-range-slider-component/frm-range-slider-component.js (1)
188-198: Hardcoded aria-label strings should be internationalized.The aria-label values (
'Vertical value','Horizontal value','top value', etc.) are hardcoded while line 341 correctly uses__( 'Value unit', 'formidable' ). Internationalize these strings for consistency.🔎 Example fix for one occurrence
- ariaLabel: 'Vertical value', + ariaLabel: __( 'Vertical value', 'formidable' ),Also applies to: 200-211, 256-266
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
js/formidable-settings-components.js.mapis excluded by!**/*.map,!**/*.map
📒 Files selected for processing (29)
classes/controllers/FrmAppController.phpclasses/views/styles/_general.phpclasses/views/styles/components/templates/colorpicker.phpcss/frm_admin.cssjs/formidable-settings-components.jsjs/formidable-web-components.jsjs/src/settings-components/components/slider-component.jsjs/src/web-components/frm-border-radius-component/frm-border-radius-component.cssjs/src/web-components/frm-border-radius-component/frm-border-radius-component.jsjs/src/web-components/frm-border-radius-component/frm-border-radius-component.scssjs/src/web-components/frm-colorpicker-component/frm-colorpicker-component.cssjs/src/web-components/frm-colorpicker-component/frm-colorpicker-component.jsjs/src/web-components/frm-colorpicker-component/frm-colorpicker-component.scssjs/src/web-components/frm-dropdown-component/frm-dropdown-component.cssjs/src/web-components/frm-dropdown-component/frm-dropdown-component.jsjs/src/web-components/frm-dropdown-component/frm-dropdown-component.scssjs/src/web-components/frm-range-slider-component/frm-range-slider-component.cssjs/src/web-components/frm-range-slider-component/frm-range-slider-component.jsjs/src/web-components/frm-range-slider-component/frm-range-slider-component.scssjs/src/web-components/frm-tab-navigator-component/frm-tab-navigator-component.cssjs/src/web-components/frm-tab-navigator-component/frm-tab-navigator-component.jsjs/src/web-components/frm-tab-navigator-component/frm-tab-navigator-component.scssjs/src/web-components/frm-typography-component/frm-typography-component.cssjs/src/web-components/frm-typography-component/frm-typography-component.jsjs/src/web-components/frm-typography-component/frm-typography-component.scssjs/src/web-components/frm-web-component.jsjs/src/web-components/frm-web-component.scssjs/src/web-components/index.jsresources/scss/admin/base/_variables.scss
🧰 Additional context used
🧬 Code graph analysis (6)
js/src/web-components/frm-colorpicker-component/frm-colorpicker-component.js (1)
js/src/web-components/frm-web-component.js (1)
frmWebComponent(1-155)
js/src/web-components/frm-dropdown-component/frm-dropdown-component.js (1)
js/src/web-components/frm-web-component.js (1)
frmWebComponent(1-155)
js/src/web-components/frm-border-radius-component/frm-border-radius-component.js (2)
js/src/web-components/frm-web-component.js (1)
frmWebComponent(1-155)js/admin/dom.js (1)
__(4-4)
js/src/web-components/frm-typography-component/frm-typography-component.js (1)
js/src/web-components/frm-web-component.js (1)
frmWebComponent(1-155)
classes/controllers/FrmAppController.php (1)
classes/helpers/FrmAppHelper.php (3)
plugin_url(65-68)FrmAppHelper(6-4623)pro_is_installed(307-309)
js/src/web-components/frm-range-slider-component/frm-range-slider-component.js (3)
js/src/web-components/frm-web-component.js (1)
frmWebComponent(1-155)js/src/settings-components/components/unit-input.js (1)
unit(26-26)js/src/settings-components/components/slider-component.js (1)
frmSliderComponent(12-561)
🪛 Biome (2.1.2)
js/src/web-components/frm-border-radius-component/frm-border-radius-component.css
[error] 1-1: Duplicate properties can lead to unexpected behavior and may override previous declarations unintentionally.
background is already defined here.
Remove or rename the duplicate property to ensure consistent styling.
(lint/suspicious/noDuplicateProperties)
[error] 1-1: Duplicate properties can lead to unexpected behavior and may override previous declarations unintentionally.
padding is already defined here.
Remove or rename the duplicate property to ensure consistent styling.
(lint/suspicious/noDuplicateProperties)
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: 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)
🔇 Additional comments (19)
classes/views/styles/components/templates/colorpicker.php (1)
7-7: LGTM! Good formatting practice.Adding a trailing newline at the end of the file aligns with common coding standards.
js/src/web-components/frm-tab-navigator-component/frm-tab-navigator-component.js (1)
144-146: ThegetLabelText()override returningnullis the correct implementation for the tab navigator component. This is intentional because tab navigators are structural components that don't display labels like regular form components. The base class'saddLabelToView()method explicitly handles thenullreturn value by skipping label insertion, which is the appropriate behavior here.classes/controllers/FrmAppController.php (1)
655-657: LGTM!The dynamic dependency composition correctly handles both Lite and Pro scenarios, ensuring
formidable-web-componentsdepends onformidable-pro-web-componentsonly when Pro is installed.js/src/settings-components/components/slider-component.js (4)
13-16: LGTM!The constructor correctly initializes the
loadedByWebComponentflag based on whether slider elements were passed in, enabling proper branching between web component and standard initialization paths.
59-65: LGTM!Clean separation of initialization paths - web components skip the DOM-dependent
initSlidersPosition()which relies on accordion sections and WordPress hooks that aren't available in the web component context.
222-226: LGTM!The
initSlidersPositionInsideWebComponentmethod correctly initializes slider widths by passing both the element and its index, avoiding the need to look up indices viagetSliderIndexwhen the index is already known.
307-311: LGTM!The
sliderIndexparameter allows direct index usage when available (web component path) while falling back togetSliderIndexlookup for standard usage. The nullish check correctly distinguishes between "index 0" and "no index provided".js/src/web-components/frm-range-slider-component/frm-range-slider-component.scss (1)
1-19: LGTM!The SCSS correctly imports shared styles and defines minimal component-specific overrides. The structure aligns with other web component stylesheets in this PR.
js/src/web-components/frm-tab-navigator-component/frm-tab-navigator-component.scss (1)
1-1: LGTM!Adding the shared base styles import aligns this component with the unified styling approach introduced across web components in this PR.
js/src/web-components/frm-dropdown-component/frm-dropdown-component.scss (1)
1-15: LGTM!Clean implementation following the shared styling pattern. The select element styles properly leverage CSS variables for consistent theming.
js/src/web-components/index.js (1)
1-13: LGTM!The component imports and registrations follow a consistent pattern and are properly structured. All five new components are correctly imported and registered as custom elements.
js/src/web-components/frm-colorpicker-component/frm-colorpicker-component.scss (1)
1-19: LGTM for the remaining styles!The component structure, styling rules, and SCSS syntax are well-organized. The base import, pseudo-element styling, and layout rules are correctly implemented.
Also applies to: 21-50
js/src/web-components/frm-typography-component/frm-typography-component.scss (1)
1-63: LGTM!The typography component styles are well-structured with:
- Proper flexbox layout and responsive width calculations
- Consistent use of CSS custom properties for design tokens
- Clean nesting and scoping
- Correct focus state handling
js/src/web-components/frm-typography-component/frm-typography-component.js (1)
9-21: Duplicate values for 'Default' and 'Regular' options.Both 'Default' (line 11) and 'Regular' (line 19) have the same value
'21px'. If this is intentional, consider adding a comment explaining why. Otherwise, one of these values may need adjustment.js/src/web-components/frm-border-radius-component/frm-border-radius-component.js (1)
200-208: Good null check pattern for_onChange.The
updateValuemethod correctly checks for_onChangebefore invoking it. This pattern should be applied consistently to other components (typography, dropdown) as noted in earlier comments.js/src/web-components/frm-colorpicker-component/frm-colorpicker-component.js (1)
54-56: No changes needed. ThewpColorPicker('color')call is a valid and documented WordPress/iris API method for retrieving the current color value. This is the correct approach for getting the color from a wpColorPicker instance.Likely an incorrect or invalid review comment.
js/src/web-components/frm-range-slider-component/frm-range-slider-component.js (3)
6-15: LGTM!Constructor properly initializes all internal state with sensible defaults.
221-242: LGTM!
createSliderHiddenInputValuecorrectly guards against undefinedfieldNameandcomponentIdbefore setting attributes, and properly wires up the onChange callback.
365-368: LGTM!
afterViewInitcorrectly instantiatesfrmSliderComponentwith the constructed slider elements and appropriate default values configuration.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI Agents
In @classes/controllers/FrmAppController.php:
- Around line 1687-1702: The method FrmAppController::enqueue_components_scripts
is defined but unused; either register it on the appropriate hook (e.g.,
add_action('admin_enqueue_scripts',
array('FrmAppController','enqueue_components_scripts')) or the equivalent
bootstrap code if intended for admin pages) so it actually enqueues the scripts,
or delete the method to remove dead code; if you keep it, also update the
hardcoded wp-color-picker-alpha version from '3.0.2' to '3.0.4' and ensure the
script handle names (wp-color-picker-alpha, form-id
formidable-pro-web-components, formidable-web-components) and the pro check
FrmAppHelper::pro_is_installed() remain correct when hooking.
- Line 718: The registration of 'formidable-web-components' happens too early
(registered at wp_register_script call around line 718) before its Pro
dependency 'formidable-pro-web-components' is available; call and hook the
existing enqueue_components_scripts() method so the registration runs after Pro
registers its script. Modify the code to remove the premature wp_register_script
call and instead invoke enqueue_components_scripts() on an appropriate admin
hook (for example add_action('admin_enqueue_scripts', [$this,
'enqueue_components_scripts']) or a form-builder-specific hook) so
enqueue_components_scripts() controls registering 'formidable-web-components'
with the correct dependency ordering.
In @css/frm_testing_mode.css:
- Around line 2-3: The top-of-file Bootstrap comment is corrupted by stray diff
markers (remove the malformed "/*! 2 * Bootstrap v4.6.1 3~ */" and restore a
proper comment header) and the file contains many duplicated CSS declarations
(e.g., repeated text-align/min-width/border-radius across selectors like
.tooltip, .frm-dropdown-menu, and #frm_testing_mode); open the CSS, fix the
header comment, then scan and merge/remove duplicate rules and redundant
properties (prefer the most specific or intended values), run a CSS
linter/minifier (or validator) to catch remaining duplicates/syntax errors, and
ensure classes such as .tooltip, .frm-dropdown-menu, and #frm_testing_mode still
apply the intended styles after deduplication.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
classes/controllers/FrmAppController.phpclasses/views/styles/_general.phpcss/admin/frm-settings-components.csscss/frm_admin.csscss/frm_testing_mode.css
🚧 Files skipped from review as they are similar to previous changes (1)
- classes/views/styles/_general.php
🧰 Additional context used
🪛 Biome (2.1.2)
css/frm_testing_mode.css
[error] 3-3: Duplicate properties can lead to unexpected behavior and may override previous declarations unintentionally.
text-align is already defined here.
Remove or rename the duplicate property to ensure consistent styling.
(lint/suspicious/noDuplicateProperties)
[error] 3-3: Duplicate properties can lead to unexpected behavior and may override previous declarations unintentionally.
min-width is already defined here.
Remove or rename the duplicate property to ensure consistent styling.
(lint/suspicious/noDuplicateProperties)
[error] 3-3: Duplicate properties can lead to unexpected behavior and may override previous declarations unintentionally.
border-radius is already defined here.
Remove or rename the duplicate property to ensure consistent styling.
(lint/suspicious/noDuplicateProperties)
⏰ 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). (3)
- GitHub Check: PHP 8 tests in WP 6.9
- GitHub Check: PHP 7.4 tests in WP 6.9
- GitHub Check: Cypress
🔇 Additional comments (3)
css/admin/frm-settings-components.css (1)
1-1: LGTM! Critical CSS syntax fix.The malformed selector has been corrected by removing the invalid leading
-character, ensuring the styles are properly applied.css/frm_testing_mode.css (1)
1-1: Verify the --primary-50 color theme change.The CSS variable
--primary-50is set to#2c333b, which is a dark gray color. This is unconventional naming since the numeric suffix "50" typically indicates a very light tint in color scales. The value appears intentional (used as a background color in components), but the naming convention may cause confusion when maintaining the design system.classes/controllers/FrmAppController.php (1)
688-690: LGTM! Clean conditional dependency management.The dynamic dependency selection based on Pro installation is well-implemented and improves maintainability over hardcoded dependencies.
… component, add a type to the buttom from border radius component, set onChange default value as empty function instead of null
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
js/src/web-components/frm-web-component.js (1)
55-70: Fix: Wrong parameter passed toafterViewInit.Line 69 passes
thistoafterViewInit(), but the method signature at line 131 expects awrapperparameter. Thewrapperconstant is defined at line 61 but not used in the callback. This breaks the contract where child classes expect to receive the actual wrapper element.🐛 Proposed fix
render() { const view = this.initView(); if ( ! view ) { return; } const wrapper = this.getWrapper(); wrapper.innerHTML = ''; view.classList.add( 'frm-component' ); wrapper.append( ...this.getViewItems( view ) ); this.addLabelToView( view ); - this.whenElementBecomesVisible().then( () => this.afterViewInit( this ) ); + this.whenElementBecomesVisible().then( () => this.afterViewInit( wrapper ) ); }
🤖 Fix all issues with AI agents
In
@js/src/web-components/frm-border-radius-component/frm-border-radius-component.js:
- Around line 76-81: getHiddenInput currently creates an input without a name so
its value won't be submitted; update getHiddenInput to set this.hiddenInput.name
= this.fieldName (using the existing this.fieldName) after creating the element
and before returning it, ensuring the hidden input also keeps
this.hiddenInput.value = this.value as already implemented.
- Around line 104-119: The unit select change handler in getInputUnit currently
only updates this.hiddenInput but does not propagate the change; update the
'change' listener on this.inputUnit to also call this.updateValue() and, if
applicable, invoke this.onChange callback (or ensure updateValue triggers
onChange) so that changes to the unit (this.inputUnit.value) are published to
consumers; modify the listener attached in getInputUnit to set
this.hiddenInput.value, call this.updateValue(), and ensure any onChange
notifier is executed using the same pattern as other input updates (referencing
getInputUnit, this.inputUnit, this.hiddenInput, this.inputValue, updateValue(),
and onChange).
In
@js/src/web-components/frm-colorpicker-component/frm-colorpicker-component.js:
- Around line 65-67: The color setter currently only assigns this.input.value
which leaves the wpColorPicker UI out of sync; update the setter (set color) to
both set this.input.value and sync the wpColorPicker UI by calling the picker
API on this.input (e.g., invoke jQuery(this.input).wpColorPicker('color', value)
when available) and/or trigger a change event on this.input so the displayed
picker and any listeners are updated.
- Around line 42-48: The component's afterViewInit() calls
jQuery(...).wpColorPicker() without ensuring wp-color-picker is loaded, causing
a race; fix it by either adding 'wp-color-picker' to the dependency array when
registering the 'formidable-web-components' script in FrmAppController.php
(ensure wp_register_script(..., ['wp-color-picker', ...]) includes that handle)
or add a defensive check inside afterViewInit() to confirm typeof
jQuery(this.input).wpColorPicker === 'function' (or jQuery().wpColorPicker)
before invoking it and skip/queue initialization if missing.
In @js/src/web-components/frm-dropdown-component/frm-dropdown-component.js:
- Around line 29-33: getSelect currently unconditionally assigns this.select.id
and this.select.name using this.componentId and this.fieldName, which results in
id="null"/name="null" when those attributes are missing; change it to only set
the id and name attributes when the corresponding values are not null/undefined
(e.g., check this.componentId != null and this.fieldName != null) so the
attributes are left unset otherwise, leaving the rest of getSelect and the
return of this.select unchanged.
🧹 Nitpick comments (7)
js/src/settings-components/components/slider-component.js (2)
223-227: Width init inside web components may run before layout is stable; consider deferring to next frame (or observing resize).
IfoffsetWidthis 0 at init time (common during initial render/hidden states), the active track width will be wrong until another interaction.Low-impact defer to next frame
initSlidersPositionInsideWebComponent() { - this.sliderElements.forEach( ( element, index ) => { - this.initSliderWidth( element, index ); - } ); + requestAnimationFrame( () => { + this.sliderElements.forEach( ( element, index ) => { + this.initSliderWidth( element, index ); + } ); + } ); }
301-321: HardeninitSliderWidthfor invalidsliderIndex+ clarify JSDoc optionality.
A badsliderIndex(or an element not present inthis.options) will currently throw when readingthis.options[ index ].maxValue.Proposed guard + JSDoc tweak
/** * Initializes the width of a slider. * * @param {HTMLElement} slider - The slider element. - * @param {number} sliderIndex - The index of the slider. + * @param {?number} sliderIndex - Optional index of the slider in `this.options`. * @return {void} */ initSliderWidth( slider, sliderIndex = null ) { if ( slider.classList.contains( 'frm-disabled' ) ) { return; } const index = sliderIndex !== null ? sliderIndex : this.getSliderIndex( slider ); + if ( ! this.options[ index ] ) { + return; + } const sliderWidth = slider.querySelector( '.frm-slider' ).offsetWidth - this.sliderBulletWidth; const value = parseInt( slider.querySelector( '.frm-slider-value input[type="text"]' ).value, 10 ); const unit = slider.querySelector( 'select' ).value; const deltaX = '%' === unit ? Math.round( sliderWidth * value / 100 ) : Math.ceil( ( value / this.options[ index ].maxValue ) * sliderWidth );js/src/web-components/frm-colorpicker-component/frm-colorpicker-component.js (1)
75-81: Consider the timing constraint for onChange assignment.The onChange setter should be called before the component is connected to the DOM. If called after
afterViewInithas initialized wpColorPicker, the new callback won't be wired to the color picker's change event.Consider adding documentation or defensive logic to handle late onChange assignment, or reinitialize the picker when onChange is updated post-initialization.
js/src/web-components/frm-typography-component/frm-typography-component.js (2)
9-34: Consider clarifying duplicate "Default" and "Regular" values.Both "Default" and "Regular" options are set to
21px(lines 11 and 19). This duplication might confuse users. If intentional, consider renaming one option for clarity.
192-207: Consider using parseFloat to preserve decimal precision.Line 204 uses
parseInt(value), which truncates decimal values like "21.5px" to 21. If decimal font sizes are supported (the regex on line 197 suggests they are), useparseFloatinstead.♻️ Proposed fix
return { - value: parseInt( value ), + value: parseFloat( value ), unit: unitType };js/src/web-components/frm-border-radius-component/frm-border-radius-component.js (1)
34-51: Verify border-radius value parsing matches CSS syntax.The parsing logic uses a custom fallback pattern that differs from CSS
border-radiusshorthand rules:
- CSS: 1 value = all corners; 2 values = (top-left & bottom-right), (top-right & bottom-left); 3 values = top-left, (top-right & bottom-left), bottom-right
- This code: treats values as top, right, bottom, left with different fallbacks
If the component is meant to mirror CSS
border-radiussyntax, the parsing logic should align with CSS specifications.js/src/web-components/frm-web-component.js (1)
114-118: Simplify: Remove unusedelementvariable.The
elementvariable is computed at line 114 but thenthisis passed toobserver.observe()at line 117. SinceshadowRoot.hostequalsthisanyway, the variable is redundant and creates confusion.♻️ Proposed refactor
- const element = this.useShadowDom() ? this.shadowRoot.host : this; - - if ( element ) { - observer.observe( this ); - } + observer.observe( this );Or if the null check is important:
- const element = this.useShadowDom() ? this.shadowRoot.host : this; - - if ( element ) { + if ( this ) { observer.observe( this ); }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
js/src/settings-components/components/slider-component.jsjs/src/web-components/frm-border-radius-component/frm-border-radius-component.jsjs/src/web-components/frm-colorpicker-component/frm-colorpicker-component.jsjs/src/web-components/frm-dropdown-component/frm-dropdown-component.jsjs/src/web-components/frm-range-slider-component/frm-range-slider-component.jsjs/src/web-components/frm-tab-navigator-component/frm-tab-navigator-component.jsjs/src/web-components/frm-typography-component/frm-typography-component.jsjs/src/web-components/frm-web-component.js
🚧 Files skipped from review as they are similar to previous changes (1)
- js/src/web-components/frm-range-slider-component/frm-range-slider-component.js
🧰 Additional context used
🧬 Code graph analysis (4)
js/src/web-components/frm-border-radius-component/frm-border-radius-component.js (1)
js/src/web-components/frm-web-component.js (1)
frmWebComponent(1-157)
js/src/web-components/frm-typography-component/frm-typography-component.js (1)
js/src/web-components/frm-web-component.js (1)
frmWebComponent(1-157)
js/src/web-components/frm-colorpicker-component/frm-colorpicker-component.js (1)
js/src/web-components/frm-web-component.js (1)
frmWebComponent(1-157)
js/src/web-components/frm-dropdown-component/frm-dropdown-component.js (2)
js/src/web-components/frm-web-component.js (1)
frmWebComponent(1-157)js/admin/style.js (1)
style(1115-1115)
🔇 Additional comments (54)
js/src/settings-components/components/slider-component.js (2)
58-67: Web-component path skips accordion/wp.hooks initialization—verify no functionality depends on those listeners in WC context.
This looks intentional, but if any sliders inside the web component rely oninitSlidersPosition()side effects (e.g., field-shape change hook / accordion open recalcs), they won’t run.
14-17: Constructor signature change is already compatible with all call sites in the codebase; no breaking changes found.The codebase has already been updated to use the new signature correctly. Both instantiation sites pass arguments in the expected order:
new frmSliderComponent()(line 21 in index.js) — relies on defaultsnew frmSliderComponent( NodeList, { settings } )(line 377 in frm-range-slider-component.js) — passes elements and settingsThe original concern about code breaking due to the signature change (e.g.,
new frmSliderComponent( settings )being misinterpreted) is not applicable here, as no such call sites exist. The proposed back-compat fix is unnecessary.Likely an incorrect or invalid review comment.
js/src/web-components/frm-tab-navigator-component/frm-tab-navigator-component.js (1)
152-154: LGTM! Intentional override to disable label rendering.This override appropriately disables label rendering for the tab navigator component, which manages its own tab titles and doesn't require an additional label from the base class.
js/src/web-components/frm-colorpicker-component/frm-colorpicker-component.js (6)
5-10: LGTM! Constructor properly initializes component state.The constructor correctly initializes the input element, component style, and onChange callback placeholder.
12-17: LGTM! View initialization follows the expected pattern.
19-36: LGTM! Input configuration handles attributes correctly.
38-40: LGTM! Correctly disables shadow DOM for jQuery wpColorPicker compatibility.The component appropriately uses light DOM because jQuery's wpColorPicker requires normal DOM access for proper positioning and styling of its UI overlay.
55-57: LGTM! Color getter uses wpColorPicker API correctly.
84-85: LGTM! Conditional Pro enhancement pattern is well-implemented.The export correctly applies the Pro component wrapper when available, allowing for optional feature enhancement.
js/src/web-components/frm-typography-component/frm-typography-component.js (10)
41-56: LGTM! View initialization is well-structured.
63-73: LGTM! Select element properly configured.
81-89: LGTM! Options are correctly populated with proper selection.
96-102: LGTM! Unit value wrapper properly structured.
109-130: LGTM! Unit input correctly handles initialization and change events.The logic appropriately prevents onChange from firing when a preset size is selected from the dropdown, avoiding redundant updates.
137-154: LGTM! Unit type select properly configured.The select defaults to the first option (px) by browser behavior, which aligns with the component's default value initialization.
161-169: LGTM! Hidden input correctly stores the final value.
176-184: LGTM! Size selection properly synchronizes all related inputs.
215-221: LGTM! onChange setter includes proper validation.
229-231: LGTM! Default value setter allows dynamic configuration.js/src/web-components/frm-dropdown-component/frm-dropdown-component.js (10)
5-10: LGTM! Constructor properly initializes component state.
17-22: LGTM! View initialization follows expected pattern.
40-42: LGTM! Shadow DOM is appropriately enabled for this component.The comment clearly explains why shadow DOM is safe to use here.
49-58: LGTM! Option migration from light DOM to shadow DOM is correctly implemented.
65-70: LGTM! Post-initialization correctly populates options and wires events.
78-86: LGTM! Dynamic option addition is well-implemented.
94-96: LGTM! Disabled state is properly delegated.
104-110: LGTM! onChange setter includes proper validation.
118-123: LGTM! Selected value setter correctly updates the select.
126-133: LGTM! Global factory provides convenient component creation API.The factory function on
windowis useful for programmatic component instantiation. Consider documenting this global API for consumers.js/src/web-components/frm-border-radius-component/frm-border-radius-component.js (15)
6-14: LGTM! Constructor initializes component with sensible defaults.
16-32: LGTM! View structure is well-organized.
53-62: LGTM! Value parsing correctly handles integer values with units.
64-74: LGTM! Input wrapper properly groups related inputs.
83-102: LGTM! Unified input correctly synchronizes all corner values.
121-137: LGTM! Individual inputs wrapper correctly shows/hides based on mode.
139-151: LGTM! Top corner input is correctly configured.
153-164: LGTM! Bottom corner input follows the same correct pattern.
166-178: LGTM! Left corner input is correctly implemented.
180-192: LGTM! Right corner input is correctly implemented.
194-198: LGTM! Individual value builder correctly concatenates corner values.Note: Ensure the value order (top, right, bottom, left) aligns with your intended CSS
border-radiussyntax, as standard CSS uses clockwise corner order (top-left, top-right, bottom-right, bottom-left).
200-204: LGTM! Value update properly notifies via onChange callback.
206-215: LGTM! Toggle button correctly shows/hides individual corner inputs.
217-223: LGTM! onChange setter includes proper validation.
225-228: LGTM! Default value setter correctly determines single vs. multiple value mode.js/src/web-components/frm-web-component.js (10)
8-10: LGTM: Good refactor to use method call.The change from a direct flag to calling
useShadowDom()method is appropriate andgetAttribute()is available during construction.
13-17: LGTM: Clean initialization pattern.The new
initOptions()method properly initializes component properties from attributes with appropriate null fallbacks.
19-30: LGTM: Solid label retrieval logic.The priority of internal
_labelTextover DOM query is appropriate, with good null handling.
32-34: LGTM: Correct attribute interpretation.The method correctly interprets the
data-shadow-domattribute as a boolean with sensible default.
47-49: LGTM: Clean wrapper abstraction.The
getWrapper()method provides a clear abstraction for determining the render target.
72-83: LGTM: Solid label rendering logic.The method correctly creates and prepends label elements with appropriate null guards.
85-87: LGTM: Good defensive filtering.The null filtering prevents potential issues if
loadStyle()returns null.
122-124: LGTM: Clean setter implementation.The setter correctly stores label text for retrieval by
getLabelText().
148-149: LGTM: Proper initialization order.Calling
initOptions()beforerender()ensures component properties are initialized from attributes before the render lifecycle begins.
42-43: Fix:style.textContentshould use assignment, not addition.Line 43 uses the
+=operator on an initiallyundefinedstyle.textContent, which will result in the string"undefined"being prepended to the component style content.🐛 Proposed fix
loadStyle() { const style = document.createElement( 'style' ); this.componentStyle = this.componentStyle.replace( '--frm-plugin-url', frmGlobal?.url || '' ); - style.textContent += this.componentStyle; + style.textContent = this.componentStyle; return style; }Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In
@js/src/web-components/frm-border-radius-component/frm-border-radius-component.css:
- Line 1: The CSS file is fully minified; unminify it so the source is
human-readable by expanding selectors and declarations into multiple lines and
consistent indentation (start with :root and the large variable block, then each
rule such as .frm-border-radius-component, .frm-border-radius-component
.frm-border-radius-container, .frm-border-radius-component
.frm-border-radius-container button, .frm-border-radius-component
.frm-border-radius-container .frm-border-individual-inputs-wrapper, etc.),
preserving all property names/values and data-URI assets exactly; ensure only
whitespace/formatting changes (no semantic changes) and commit the prettified
file while keeping minification to the build pipeline.
In @js/src/web-components/frm-dropdown-component/frm-dropdown-component.css:
- Line 1: The rule .frm-dropdown-component select currently disables the
keyboard focus ring with outline:0; restore an accessible focus indicator by
removing outline:0 (or at least not applying it on focus) and add a
:focus-visible style for .frm-dropdown-component select:focus-visible that
provides a visible, high-contrast indicator (for example an outline or
box-shadow using existing variables like --primary-500/--primary-hover or
--blue-border) so keyboard users can see focus while preserving the current
visual style for mouse users.
In
@js/src/web-components/frm-range-slider-component/frm-range-slider-component.css:
- Line 1: The committed frm-range-slider-component.css is minified and
unreadable; replace it with a formatted source CSS and stop committing minified
artifacts. Unminify and restore a human-readable stylesheet (preserving
selectors like .frm-style-component .frm-slider-container,
.frm-slider-active-track, .frm-slider-bullet, .frm-slider-value, etc.), add that
as the canonical source (e.g., frm-range-slider-component.source.css or
overwrite the current file with pretty-printed CSS), and update your build to
emit a minified artifact during CI/build rather than committing it; finally, add
the generated minified file pattern to .gitignore or separate source vs build
outputs so only the unminified source is tracked.
🧹 Nitpick comments (8)
js/src/web-components/frm-tab-navigator-component/frm-tab-navigator-component.js (1)
152-154: LGTM! Consider adding JSDoc for consistency.The implementation correctly returns
nullfor this component type. However, all other methods in the class include JSDoc comments. Adding documentation would improve consistency and help other developers understand this interface method.📝 Suggested JSDoc addition
+ /** + * Gets the label text for the component. + * + * @return {null} - Tab navigators do not have a label. + */ getLabelText() { return null; }js/src/web-components/frm-border-radius-component/frm-border-radius-component.css (2)
1-1: Remove duplicate property declarations.Static analysis has identified duplicate CSS properties that should be cleaned up:
- Duplicate
backgroundon button: First set to#fff, then immediately overridden with SVG data URL- Duplicate
paddingon input: First set to0, then immediately overridden with0 12px 0px 20pxWhile the second value correctly takes precedence, duplicate declarations create confusion and may mask unintentional overrides.
🔧 Recommended fix
Remove the first declaration in each case:
For button:
-background:#fff;display:flex;justify-content:center;align-items:center;box-sizing:border-box;background:url('data:image/svg+xml,... +display:flex;justify-content:center;align-items:center;box-sizing:border-box;background:url('data:image/svg+xml,...For input:
-padding:0;font-size:var(--text-sm);color:#101828;padding:0 12px 0px 20px; +font-size:var(--text-sm);color:#101828;padding:0 12px 0px 20px;
1-1: Consider modern text-hiding technique for the button.The button uses
overflow:hidden; text-indent:-9999px;to hide text content. While this works, modern approaches usingcliporposition: absolutewithwidth: 1px; height: 1pxare more performant and avoid potential issues with RTL languages.Alternative approach
.frm-border-radius-component .frm-border-radius-container button { /* Replace text-indent with: */ position: relative; /* Then hide text with: */ font-size: 0; color: transparent; }Or use a visually-hidden utility class pattern. However, the current implementation is acceptable.
js/src/web-components/frm-range-slider-component/frm-range-slider-component.css (3)
1-1: Centralize CSS custom properties to avoid duplication.The stylesheet defines an extensive set of CSS custom properties (design tokens) at the root level. Based on the AI summary, these same variables appear to be duplicated across multiple component CSS files (e.g.,
frm-dropdown-component.css).Consider extracting these shared design tokens into a central stylesheet (e.g.,
design-tokens.cssortheme-variables.css) and importing it where needed. This will:
- Eliminate duplication and reduce bundle size
- Ensure design consistency across components
- Simplify maintenance (single source of truth for design changes)
- Follow DRY principles
/* Example: Extract to shared design-tokens.css */ :root, .frm-white-body, .frm_wrap { --grey-900: #101828; --grey-800: #1D2939; /* ... all shared tokens ... */ }Then import or reference this shared file in component stylesheets.
1-1: Reduce reliance on !important declarations.The stylesheet contains multiple
!importantdeclarations (at least 6 instances) in the component label selectors. This often indicates specificity conflicts or attempts to override existing styles.Since this component uses shadow DOM (per the AI summary), styles should be naturally encapsulated. Consider refactoring the CSS hierarchy to avoid
!important:
- Review if these rules are truly needed or if specificity can be adjusted
- Check for conflicting global styles that might be bleeding in
- Use more specific selectors or restructure the cascade instead of forcing overrides
This will improve maintainability and make the styles easier to override when customization is needed.
1-1: Consider build-time asset resolution for background images.The stylesheet uses a hardcoded relative path for the SVG background image:
url("../../images/style/small-arrow.svg"). This path assumes a specific file system structure and may break if:
- The component is bundled or deployed differently
- The build tool doesn't preserve relative paths
- The CSS is moved or reorganized
Consider using a build-time asset resolution system (like webpack's file-loader or similar) to ensure reliable path resolution across different deployment contexts.
Additionally, for improved accessibility, consider adding explicit
:focus-visiblestyles for the slider components to enhance keyboard navigation UX.js/src/web-components/frm-dropdown-component/frm-dropdown-component.css (2)
1-1: Try to reduce!importanton the label rule to keep theming/overrides workable.
span.frm-component-labeluses!importantfor font-size/color/width/display/margin-right; that can make downstream overrides (themes, host contexts) painful. If possible, prefer higher-specificity scoping (e.g.,:host/.frm-dropdown-component ...) over!important.
1-1: Extract design tokens into a shared stylesheet to reduce duplication and prevent token drift.The design-token block (59+ custom properties) is currently duplicated across 5 component CSS files (frm-typography-component, frm-colorpicker-component, frm-dropdown-component, frm-tab-navigator-component, frm-border-radius-component). Creating a centralized token file imported once, with only component-scoped rules in each file, will reduce bundle size and simplify future updates.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
classes/controllers/FrmAppController.phpjs/formidable-settings-components.jsjs/formidable-web-components.jsjs/formidable_admin.jsjs/src/web-components/frm-border-radius-component/frm-border-radius-component.cssjs/src/web-components/frm-colorpicker-component/frm-colorpicker-component.cssjs/src/web-components/frm-dropdown-component/frm-dropdown-component.cssjs/src/web-components/frm-range-slider-component/frm-range-slider-component.cssjs/src/web-components/frm-tab-navigator-component/frm-tab-navigator-component.cssjs/src/web-components/frm-tab-navigator-component/frm-tab-navigator-component.jsjs/src/web-components/frm-typography-component/frm-typography-component.css
🚧 Files skipped from review as they are similar to previous changes (3)
- js/src/web-components/frm-typography-component/frm-typography-component.css
- js/src/web-components/frm-tab-navigator-component/frm-tab-navigator-component.css
- js/src/web-components/frm-colorpicker-component/frm-colorpicker-component.css
🧰 Additional context used
🧬 Code graph analysis (2)
classes/controllers/FrmAppController.php (2)
classes/helpers/FrmAppHelper.php (3)
plugin_url(65-68)FrmAppHelper(6-5051)pro_is_installed(391-393)stripe/helpers/FrmTransLiteAppHelper.php (1)
plugin_url(18-20)
js/formidable-settings-components.js (1)
js/src/admin/upgrade-popup.js (1)
frmDom(3-3)
🪛 Biome (2.1.2)
js/src/web-components/frm-border-radius-component/frm-border-radius-component.css
[error] 1-1: Duplicate properties can lead to unexpected behavior and may override previous declarations unintentionally.
background is already defined here.
Remove or rename the duplicate property to ensure consistent styling.
(lint/suspicious/noDuplicateProperties)
[error] 1-1: Duplicate properties can lead to unexpected behavior and may override previous declarations unintentionally.
padding is already defined here.
Remove or rename the duplicate property to ensure consistent styling.
(lint/suspicious/noDuplicateProperties)
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: 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 (5)
js/src/web-components/frm-dropdown-component/frm-dropdown-component.css (1)
1-1: No action required—CSS scoping is already correctly implemented.The stylesheet is properly encapsulated within the shadow DOM (injected as a
<style>element directly intoshadowRoot), so the CSS variables,.frm-component, and.frm-dropdown-component selectrules apply only to the component's internal DOM. The:rootselector correctly targets the shadow root, making CSS custom properties accessible to descendants. There is no global leakage—styles do not bleed to unrelated elements in the light DOM or elsewhere.js/formidable-settings-components.js (1)
1-1: Minified bundle with no functional changes.This is a minified JavaScript bundle. According to the AI summary, there are no observable functional changes—only minor formatting adjustments in the minified output. The static analysis errors from Biome are false positives typical of minified code and can be safely ignored.
classes/controllers/FrmAppController.php (3)
686-688: LGTM! Clean conditional dependency handling.The dynamic dependency array correctly adjusts based on Pro installation status, ensuring that
formidable-web-componentsdepends onformidable-pro-web-componentsonly when Pro is active.
716-716: LGTM! Proper use of dynamic dependencies.The script registration now correctly uses the conditional dependency array defined earlier, enabling Pro-aware component loading.
1686-1700: No issues to address.The
enqueue_components_scripts()method implements correct WordPress patterns. Thewp-color-pickerscript is a WordPress core script always registered by WordPress—it's not conditionally loaded based on page context in the plugin code. The inlinewp_enqueue_script()registration pattern at line 1695 is standard WordPress practice and properly handles dependencies. Theformidable-web-componentsscript is correctly registered at line 716 with appropriate conditional dependencies for Pro installations.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In
@js/src/web-components/frm-colorpicker-component/frm-colorpicker-component.js:
- Around line 59-67: The color setter currently writes directly to
this.input.value which does not update the wpColorPicker UI; change the set
color(value) implementation to use the wpColorPicker API (or the instance
attached to this.input) to update the color so the swatch and iris reflect the
change—e.g. obtain the wpColorPicker instance for this.input and call its public
method to set the color or set the input value then trigger the picker
update/event so the UI stays in sync.
In
@js/src/web-components/frm-range-slider-component/frm-range-slider-component.js:
- Around line 137-159: parseDefaultValues() treats
this._sliderDefaultMultipleValues as a space-separated string (calling .split),
but the setter JSDoc documents it as an Object; either update the setter JSDoc
to reflect a string (e.g., change @param {Object} to @param {string} and mention
space-separated values) or make parseDefaultValues() robust by checking typeof
this._sliderDefaultMultipleValues === 'object' and, if so, map its expected keys
into the parts array (or build a string) before calling split/parseValueUnit;
reference the setter JSDoc, this._sliderDefaultMultipleValues and
parseDefaultValues() when making the change.
- Around line 345-348: The valueInput element gets aria-label="undefined"
because createSlider is called without an ariaLabel in initView for the
single-slider case; update createSlider (and/or the initView call) so ariaLabel
defaults to a sensible string when falsy (e.g., '' or a localized "Value")
before being applied to valueInput.setAttribute, specifically check and
normalize the ariaLabel arg inside createSlider and use that normalized value
when setting valueInput's aria-label to avoid "undefined".
- Line 87: The fieldName variable is being created with attribute syntax
(`name="..."`) which causes createSliderHiddenInputValue to call
setAttribute('name', fieldName) with the whole string `name="myField"` instead
of just `myField`; fix by changing the construction of fieldName to hold only
the raw name value (e.g., set const fieldName = this.fieldName ? this.fieldName
: '' or similar) or adjust createSliderHiddenInputValue to pass this.fieldName
directly to setAttribute('name', ...); update the code around the fieldName
declaration and usages in createSliderHiddenInputValue so the DOM input's name
is the plain field name.
- Around line 365-367: The hidden input created by createSliderHiddenInputValue
lacks name/id/value because createSlider is being called without the fieldName,
componentId, and fieldValue props; update the initView call site that invokes
createSlider to include fieldName, componentId, and fieldValue (or fieldValue
derived from value) so those properties are available, then update
createSlider's parameter destructuring to accept and forward fieldName,
componentId, and fieldValue in the options passed to
createSliderHiddenInputValue (ensure addHiddenInputValue branch uses the
enriched options object).
🧹 Nitpick comments (5)
js/src/web-components/frm-dropdown-component/frm-dropdown-component.js (1)
78-86: Consider using a method or renaming this setter.Using a setter named
addOptionsfor an additive operation is semantically misleading—setters typically set/replace a value. Additionally, there's no validation thatoptionsis iterable.♻️ Option 1: Convert to a method (preferred)
- set addOptions( options ) { + addOptions( options ) { + if ( ! Array.isArray( options ) ) { + return; + } options.forEach( option => { const opt = document.createElement( 'option' ); opt.value = option.value; opt.textContent = option.label; opt.selected = option.selected; this.select.append( opt ); } ); }Then update the global helper:
- dropdown.addOptions = options; + dropdown.addOptions( options );♻️ Option 2: Rename to `options` setter that replaces all options
- set addOptions( options ) { + set options( options ) { + if ( ! Array.isArray( options ) ) { + return; + } + this.select.innerHTML = ''; options.forEach( option => {js/src/web-components/frm-colorpicker-component/frm-colorpicker-component.js (1)
42-48: Conditional check is always true.The condition
'function' === typeof this._onChangewill always evaluate totruebecause_onChangeis initialized to() => {}in the constructor (line 9). Consider usingnullor checking a separate flag if the intent is to conditionally add the change handler only when explicitly set.♻️ Suggested refactor
constructor() { super(); this.input = document.createElement( 'input' ); this.componentStyle = style; - this._onChange = () => {}; + this._onChange = null; } afterViewInit() { - const colorPickerOptions = 'function' === typeof this._onChange ? { + const colorPickerOptions = null !== this._onChange ? { change: ( event, ui ) => this._onChange( event, ui ), } : {}; jQuery( this.input ).wpColorPicker( colorPickerOptions ); }js/src/web-components/frm-range-slider-component/frm-range-slider-component.js (3)
37-69: Consider adding validation to setters for consistency.The
onChangesetter validates its input and throws aTypeError, but other setters likehasMultipleValues(expects boolean) andsliderAvailableUnits(expects Array) accept any value without validation. Consider adding similar type checks for consistency and early error detection.
195-218: Hardcoded aria labels should be internationalized.
ariaLabel: 'Vertical value'andariaLabel: 'Horizontal value'are hardcoded English strings. Since the component imports__from@wordpress/i18nand uses it elsewhere (line 351), these should also be wrapped for consistency.♻️ Proposed fix
// Vertical slider (group) wrapper.append( this.createSliderGroup( { type: 'vertical', displaySliders: 'top,bottom', maxValue, units, value: defaultValues.vertical, iconSvgId: 'frm-margin-top-bottom', - ariaLabel: 'Vertical value', + ariaLabel: __( 'Vertical value', 'formidable' ), defaultValues: defaultValues, addHiddenInputValue: false } ) ); // Horizontal slider (group) wrapper.append( this.createSliderGroup( { type: 'horizontal', displaySliders: 'left,right', maxValue, units, value: defaultValues.horizontal, iconSvgId: 'frm-margin-left-right', - ariaLabel: 'Horizontal value', + ariaLabel: __( 'Horizontal value', 'formidable' ), defaultValues: defaultValues, addHiddenInputValue: false } ) );
264-275: Child slider aria labels should be internationalized.The dynamically generated
ariaLabel: \${ item } value`creates strings like "top value", "bottom value" without internationalization. Consider using__()` with a translatable format string.♻️ Proposed fix
slidersGroupItems.forEach( item => { slider.append( this.createSlider( { type: item, maxValue: options.maxValue, units: options.units, value: options.defaultValues[ item ], iconSvgId: `frm-margin-${ item }`, - ariaLabel: `${ item } value`, + ariaLabel: sprintf( __( '%s value', 'formidable' ), item ), hidden: true, addHiddenInputValue: false } ) ); } );Note: This requires importing
sprintffrom@wordpress/i18n.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
js/src/web-components/frm-border-radius-component/frm-border-radius-component.jsjs/src/web-components/frm-colorpicker-component/frm-colorpicker-component.jsjs/src/web-components/frm-dropdown-component/frm-dropdown-component.jsjs/src/web-components/frm-range-slider-component/frm-range-slider-component.jsjs/src/web-components/frm-typography-component/frm-typography-component.js
🚧 Files skipped from review as they are similar to previous changes (2)
- js/src/web-components/frm-typography-component/frm-typography-component.js
- js/src/web-components/frm-border-radius-component/frm-border-radius-component.js
🧰 Additional context used
🧬 Code graph analysis (3)
js/src/web-components/frm-range-slider-component/frm-range-slider-component.js (5)
js/src/web-components/frm-web-component.js (1)
frmWebComponent(1-157)js/admin/style.js (1)
style(1113-1113)webpack.config.js (1)
match(12-12)js/src/settings-components/components/unit-input.js (1)
unit(26-26)js/src/settings-components/components/slider-component.js (1)
frmSliderComponent(13-562)
js/src/web-components/frm-colorpicker-component/frm-colorpicker-component.js (1)
js/src/web-components/frm-web-component.js (1)
frmWebComponent(1-157)
js/src/web-components/frm-dropdown-component/frm-dropdown-component.js (1)
js/src/web-components/frm-web-component.js (1)
frmWebComponent(1-157)
⏰ 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). (3)
- GitHub Check: Cypress
- GitHub Check: PHP 8 tests in WP 6.9
- GitHub Check: PHP 7.4 tests in WP 6.9
🔇 Additional comments (22)
js/src/web-components/frm-dropdown-component/frm-dropdown-component.js (10)
1-10: LGTM!The constructor properly initializes the component with sensible defaults, including a no-op callback for
_onChangeto prevent null reference errors.
17-22: LGTM!Clean view initialization following the parent component's pattern.
29-33: LGTM!The select element is correctly configured with id and name attributes from the component's options.
40-42: LGTM!The override is intentional and well-documented, explaining that the dropdown doesn't require external dependencies.
49-58: LGTM!Good approach for migrating declaratively-defined options from the light DOM into the shadow DOM select element.
65-70: LGTM!The change listener is correctly set up to delegate to the configurable callback. Since the listener is attached to the component's own select element, it will be cleaned up when the component is removed.
94-96: LGTM!Simple and correct implementation.
104-110: LGTM!Good defensive programming with proper type validation and a descriptive error message.
118-123: LGTM!Clean implementation for programmatically setting the selected value.
127-133: LGTM!Useful factory function for programmatic component creation. Note: If you convert
addOptionsto a method per the earlier suggestion, update line 130 accordingly.js/src/web-components/frm-colorpicker-component/frm-colorpicker-component.js (6)
1-10: LGTM!Constructor properly extends the parent class, initializes the input element, and sets up defaults. The early creation of the input element is appropriate given its usage in
initView()andgetInput().
12-36: LGTM!The
initView()andgetInput()methods follow the parent class patterns correctly. The null checks for optional attributes are appropriate, and the input configuration is well-structured.
38-40: LGTM!Disabling Shadow DOM is the correct approach here since
wpColorPickercreates additional DOM elements that need to interact with the page's styles.
50-57: LGTM!The getter correctly uses the wpColorPicker API to retrieve the current color value.
69-81: Timing issue: onChange won't work if set after component renders.If
onChangeis set after the component is already connected andafterViewInit()has run, the wpColorPicker will have been initialized without the callback. Consider documenting thatonChangemust be set before the component is added to the DOM, or re-bind the change handler whenonChangeis updated.♻️ Potential fix to support late binding
set onChange( callback ) { if ( 'function' !== typeof callback ) { throw new TypeError( `Expected a function, but received ${ typeof callback }` ); } this._onChange = callback; + + // Re-bind if wpColorPicker is already initialized + const $input = jQuery( this.input ); + if ( $input.data( 'wpWpColorPicker' ) ) { + $input.wpColorPicker( 'option', 'change', ( event, ui ) => this._onChange( event, ui ) ); + } }Note: Verify the correct wpColorPicker data attribute name and API for updating options after initialization, as it may vary by WordPress version.
84-85: LGTM!The conditional export pattern allows Pro features to wrap the Lite component when available, providing a clean extension mechanism.
js/src/web-components/frm-range-slider-component/frm-range-slider-component.js (6)
1-15: LGTM!The imports are well-organized, and the constructor properly initializes instance properties with sensible defaults. The use of underscore-prefixed properties with public setters follows a reasonable encapsulation pattern.
71-73: LGTM!Correctly overrides the parent method to disable Shadow DOM for this component.
117-129: LGTM!The method correctly parses the
data-unitsattribute with proper fallback. Minor: typo "checke" → "check" in the JSDoc comment (line 118).
161-176: Regex does not support decimal or negative values.The regex
/^(\d+)(px|em|%)?$/only matches positive integers. Values like1.5em,-10px, or.5emwill fall back to{ value: 0, unit: 'px' }. If decimal or negative values are expected use cases, consider updating the pattern.♻️ Suggested fix to support decimals and negatives
- const match = valueStr.match( /^(\d+)(px|em|%)?$/ ); + const match = valueStr.match( /^(-?\d*\.?\d+)(px|em|%)?$/ );
178-185: LGTM!The method correctly checks both the attribute and the instance property with appropriate string comparison for the attribute.
375-379: LGTM!The method correctly initializes
frmSliderComponentwith the appropriate default values based on whether the component is in single or multiple value mode.
There was a problem hiding this comment.
Actionable comments posted: 12
🤖 Fix all issues with AI agents
In
`@js/src/web-components/frm-border-radius-component/frm-border-radius-component.js`:
- Around line 5-11: Remove the blank line immediately after the opening brace of
the class frmBorderRadiusComponent to satisfy the ESLint padded-blocks rule;
collapse the blank line so the first class field declaration (e.g., `#onChange` =
() => {}) follows directly after "export class frmBorderRadiusComponent extends
frmWebComponent {" without inserting or altering other private field
declarations (`#unitTypeOptions`, `#value`, `#defaultValue`, `#usesMultipleValues`).
- Around line 231-234: The setter borderRadiusDefaultValue currently treats
decimals as "multiple" because the regex /^(\d+)(px|em|%)?$/ only matches
integers; update the regex used in the assignment to `#usesMultipleValues` to
accept decimal numbers (e.g. change to something like /^\d*\.?\d+(px|em|%)?$/'
or /^\d+(\.\d+)?(px|em|%)?$/) so values like "1.5em" are considered a single
value, keeping the existing '' !== value check and retaining the assignment to
this.#defaultValue.
- Around line 54-63: parseValueUnit currently only matches integers; update its
regex to accept decimals and leading-dot numbers (e.g. "1.5", ".5") and switch
from parseInt to parseFloat. Locate the parseValueUnit function and replace the
pattern /^(\d+)(px|em|%)?$/ with one that matches decimals such as
/^(\d+(?:\.\d+)?|\.\d+)(px|em|%)?$/ and use parseFloat(match[1]) for the numeric
value; keep the fallback unit logic (match[2] || 'px') and return the numeric
value and unit as before.
In
`@js/src/web-components/frm-colorpicker-component/frm-colorpicker-component.js`:
- Line 7: The private field `#defaultValue` is never populated so getInput() reads
an empty value; change the code to use the parent's public defaultValue property
(set by initOptions()) instead of the private `#defaultValue`, i.e. replace
references to `#defaultValue` in getInput() and the other occurrences (around
lines 30-32) with this.defaultValue, or alternatively initialize `#defaultValue`
from this.defaultValue inside initOptions()/constructor so the private field
mirrors the attribute.
- Around line 4-7: Remove the extra blank line immediately after the class
opening brace in class frmColorpickerLiteComponent so the private field
declarations (`#onChange` and `#defaultValue`) directly follow the brace, satisfying
the ESLint block padding rule; update the class declaration to have no empty
line between "class frmColorpickerLiteComponent extends frmWebComponent {" and
the subsequent field definitions.
In `@js/src/web-components/frm-dropdown-component/frm-dropdown-component.js`:
- Around line 4-6: Remove the blank line after the class opening brace in
frmDropdownComponent to satisfy the padded-blocks ESLint rule: move the private
field declaration "#onChange = () => {};" immediately after "export class
frmDropdownComponent extends frmWebComponent {" so there is no empty line
between the brace and the "#onChange" field.
In
`@js/src/web-components/frm-range-slider-component/frm-range-slider-component.js`:
- Around line 6-7: The class frmRangeSliderComponent has an extra blank line
immediately after its opening brace which triggers the ESLint "Block must not be
padded by blank lines" rule; remove the empty line inside the class body (and
ensure no extra blank line immediately before the closing brace) so the
declaration reads directly after "export class frmRangeSliderComponent extends
frmWebComponent {" with members following on the next line.
- Line 246: The ESLint error is due to missing space before the closing brace in
the object literal passed to Object.assign; update the Object.assign call that
assigns to input (the one using fieldValue) so the object literal has a space
before the closing brace (i.e., change "{ type: 'hidden', value: fieldValue}" to
include a space before "}") ensuring the call remains Object.assign(input, {
type: 'hidden', value: fieldValue });.
- Around line 434-437: afterViewInit calls a nonexistent method
this.parseDefaultValues(); replace that call with the correct
this.parseDefaultMultipleValues() so the branch using hasMultipleSliderValues()
passes the parsed defaults; keep the existing fallback to
this.parseValueUnit(this.defaultValue) and the instantiation of new
frmSliderComponent(this.wrapper.querySelectorAll('.frm-slider-component'), {
defaultValues }).
- Around line 234-240: The JSDoc for createSliderHiddenInputValue uses the wrong
parameter name, causing an ESLint mismatch; update the JSDoc `@param` tag to match
the actual function signature by replacing the documented parameter name (e.g.,
options) with fieldValue and keep or adjust the type/description accordingly so
the JSDoc line (for createSliderHiddenInputValue) matches the function
parameter.
- Around line 424-426: The append call passes the entire options object to
createSliderHiddenInputValue but that helper expects a string fieldValue; fix
the call in the addHiddenInputValue branch so it supplies the actual field value
(e.g., the slider's current value or options.fieldValue) instead of options.
Locate where addHiddenInputValue is used and change valueContainer.append(
this.createSliderHiddenInputValue( options ) ) to pass the correct string (for
example this.createSliderHiddenInputValue(fieldValue) or
this.createSliderHiddenInputValue(options.fieldValue) or compute the value via
the component's current value getter) so the hidden input's value is a string,
not "[object Object]".
In `@js/src/web-components/frm-typography-component/frm-typography-component.js`:
- Around line 5-7: Remove the blank line after the class opening brace in
frmTypographyComponent so the private field declaration (`#onChange` = () => {};)
immediately follows the `{`; update the class declaration in
frmTypographyComponent to eliminate the empty line padding inside the class
block to satisfy the padded-blocks ESLint rule.
♻️ Duplicate comments (5)
js/src/web-components/frm-colorpicker-component/frm-colorpicker-component.js (2)
45-51: Dependency onwpColorPickermay not be loaded.This calls
jQuery().wpColorPicker()without ensuring the dependency is available. This was flagged in a previous review - please ensure'wp-color-picker'is added to the script's dependency array in PHP, or add a defensive check before calling the plugin.
68-70: Color setter doesn't update wpColorPicker UI.Setting
this.input.valuedirectly won't update the wpColorPicker's visual elements after initialization. This was flagged in a previous review - use the wpColorPicker API for consistency with the getter.🐛 Proposed fix
set color( value ) { - this.input.value = value; + jQuery( this.input ).wpColorPicker( 'color', value ); }js/src/web-components/frm-typography-component/frm-typography-component.js (1)
200-215: Potential TypeError if regex doesn't match.The fix at lines 202-204 handles falsy values, but
value.match(...)still returnsnullfor non-matching strings (e.g.,"abc"), causingnull[2]to throw aTypeError.🐛 Proposed fix
getUnitValue( value ) { const defaultValue = { value: 0, unit: 'px' }; if ( ! value ) { return defaultValue; } - const unitType = value.match( /^([\d.]+)(px|em|%)?$/ )[ 2 ] || 'px'; - - if ( ! unitType ) { - return defaultValue; - } - + const match = value.match( /^([\d.]+)(px|em|%)?$/ ); + if ( ! match ) { + return defaultValue; + } + const unitType = match[ 2 ] || 'px'; return { - value: parseInt( value ), + value: parseInt( match[ 1 ], 10 ), unit: unitType }; }js/src/web-components/frm-border-radius-component/frm-border-radius-component.js (1)
120-122: Unit change still doesn't trigger onChange callback.The change handler only updates
hiddenInput.valuebut doesn't callupdateValue(). Consumers relying on theonChangecallback won't be notified when the unit changes.🔧 Proposed fix
this.inputUnit.addEventListener( 'change', () => { - this.hiddenInput.value = this.inputValue.value + this.inputUnit.value; + const value = this.inputValue.value + this.inputUnit.value; + this.updateValue( value ); } );js/src/web-components/frm-range-slider-component/frm-range-slider-component.js (1)
50-58: Type mismatch:#sliderDefaultMultipleValuesis an object butparseDefaultMultipleValuescalls.split()on it.The default value for
#sliderDefaultMultipleValues(lines 10-17) is an object, but inparseDefaultMultipleValues(line 140),.split(' ')is called onvaluesAttrwhich may be this object when nodata-valuesattribute exists. This will throw aTypeError.Either:
- Change the default to a space-separated string (e.g.,
'0px 0px 0px 0px'), or- Add a type check in
parseDefaultMultipleValues🐛 Option 1: Change default to string
- `#sliderDefaultMultipleValues` = { - vertical: { value: 0, unit: 'px' }, - top: { value: 0, unit: 'px' }, - bottom: { value: 0, unit: 'px' }, - horizontal: { value: 0, unit: 'px' }, - left: { value: 0, unit: 'px' }, - right: { value: 0, unit: 'px' } - }; + `#sliderDefaultMultipleValues` = '0px 0px 0px 0px';And update JSDoc:
/** * A method to set the default multiple values. * - * `@param` {Object} value - The value to set. + * `@param` {string} value - Space-separated values (e.g., "10px 20px 10px 20px"). * `@return` {void} */
🧹 Nitpick comments (10)
js/src/web-components/frm-colorpicker-component/frm-colorpicker-component.js (1)
46-48: Conditional check is always true.
this.#onChangeis initialized as() => {}on line 6, so'function' === typeof this.#onChangeis alwaystrue. The ternary serves no purpose.♻️ Simplify by always passing the options object
afterViewInit() { - const colorPickerOptions = 'function' === typeof this.#onChange ? { + const colorPickerOptions = { change: ( event, ui ) => this.#onChange( event, ui ), - } : {}; + }; jQuery( this.input ).wpColorPicker( colorPickerOptions ); }js/src/web-components/frm-dropdown-component/frm-dropdown-component.js (1)
86-94: Consider adding input validation for the options array.If
optionsisnull,undefined, or not iterable, callingforEachwill throw aTypeError. Consider adding a guard clause.🔧 Proposed fix
set addOptions( options ) { + if ( ! Array.isArray( options ) ) { + return; + } options.forEach( option => {js/src/web-components/frm-typography-component/frm-typography-component.js (2)
8-13: Consider removing redundant field initialization.The
#defaultOptionsfield is initialized at declaration (lines 8-13) but immediately overwritten in the constructor (lines 21-46). The field-level initialization is unnecessary.♻️ Proposed fix
- `#onChange` = () => {}; - `#defaultOptions` = [ - { - value: '21px', - label: __( 'Default', 'formidable' ) - }, - ]; + `#onChange` = () => {}; + `#defaultOptions` = [];Also applies to: 21-46
127-127: Add radix parameter toparseInt.
parseIntshould include an explicit radix (base 10) to avoid unexpected behavior with strings that have leading zeros.♻️ Proposed fix
- this.unitValueInput.value = `${ parseInt( this.#defaultOptions.find( option => option.value === this.#defaultValue )?.value ) || 21 }`; + this.unitValueInput.value = `${ parseInt( this.#defaultOptions.find( option => option.value === this.#defaultValue )?.value, 10 ) || 21 }`;js/src/web-components/frm-border-radius-component/frm-border-radius-component.js (1)
145-198:parseDefaultValues()is called four times during initialization.Each
getBorderInput*method independently callsparseDefaultValues(), parsing the same string four times. Consider parsing once and reusing the result.♻️ Suggested approach
Parse default values once in
getBorderIndividualInputsWrapperand pass to each method:getBorderIndividualInputsWrapper() { this.borderIndividualInputsWrapper = document.createElement( 'div' ); this.borderIndividualInputsWrapper.classList.add( 'frm-border-individual-inputs-wrapper' ); if ( ! this.#usesMultipleValues ) { this.borderIndividualInputsWrapper.classList.add( 'frm_hidden' ); } + const defaultValues = this.parseDefaultValues(); this.borderIndividualInputsWrapper.append( - this.getBorderInputTop(), - this.getBorderInputRight(), - this.getBorderInputLeft(), - this.getBorderInputBottom() + this.getBorderInputTop( defaultValues ), + this.getBorderInputRight( defaultValues ), + this.getBorderInputLeft( defaultValues ), + this.getBorderInputBottom( defaultValues ) ); return this.borderIndividualInputsWrapper; } - getBorderInputTop() { - const defaultValues = this.parseDefaultValues(); + getBorderInputTop( defaultValues ) { // ... rest of method }js/src/web-components/frm-range-slider-component/frm-range-slider-component.js (5)
91-98: Dead code:fieldNamein config object is never used.The
fieldNameproperty at line 96 constructs an attribute-syntax string (name="..."), but this value is never actually used -createSliderHiddenInputValuenow readsthis.fieldNamedirectly from the instance. Consider removing this unused property from the config object.♻️ Proposed cleanup
const config = { maxValue: parseInt( this.getAttribute( 'data-max-value' ) || '100', 10 ), units: this.getAvailableUnits(), componentClass: this.getAttribute( 'data-component-class' ) || '', componentId: this.componentId, - fieldName: this.fieldName ? `name="${ this.fieldName }"` : '', fieldValue: this.defaultValue || this.#sliderDefaultValue };
337-339: Good fix:aria-label="undefined"is now prevented.The conditional check before setting
aria-labelprevents the previous bug. For improved accessibility, consider providing a sensible default whenariaLabelis not specified.♻️ Optional: Provide default aria-label
- if ( ariaLabel ) { - valueInput.setAttribute( 'aria-label', ariaLabel ); - } + valueInput.setAttribute( 'aria-label', ariaLabel || __( 'Slider value', 'formidable' ) );
185-185: ESLint: JSDoc block alignment warnings.ESLint reports alignment issues in JSDoc blocks at lines 185, 321, and 351. Consider aligning the JSDoc descriptions for consistency.
166-173: Consider: Regex only matches integer values.The pattern
/^(\d+)(px|em|%)?$/won't match decimal values like"1.5em"or"0.5px". If decimal support is needed, update the regex.♻️ Optional: Support decimal values
- const match = valueStr.match( /^(\d+)(px|em|%)?$/ ); + const match = valueStr.match( /^(\d+(?:\.\d+)?)(px|em|%)?$/ );Note: You'd also need to use
parseFloatinstead ofparseIntat line 171.
194-209: InternationalizeariaLabelstrings for accessibility.The
ariaLabelvalues are hardcoded in English. For proper internationalization, wrap them with__().♻️ Proposed fix
const groups = [ { type: 'vertical', displaySliders: 'top,bottom', iconSvgId: 'frm-margin-top-bottom', - ariaLabel: 'Vertical value', + ariaLabel: __( 'Vertical value', 'formidable' ), defaultValues: defaultValues.vertical, }, { type: 'horizontal', displaySliders: 'left,right', iconSvgId: 'frm-margin-left-right', - ariaLabel: 'Horizontal value', + ariaLabel: __( 'Horizontal value', 'formidable' ), defaultValues: defaultValues.horizontal, } ];Similarly update line 284:
- ariaLabel: `${ item } value`, + ariaLabel: __( `${ item } value`, 'formidable' ),
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
js/formidable-settings-components.js.mapis excluded by!**/*.map,!**/*.map
📒 Files selected for processing (6)
js/formidable-web-components.jsjs/src/web-components/frm-border-radius-component/frm-border-radius-component.jsjs/src/web-components/frm-colorpicker-component/frm-colorpicker-component.jsjs/src/web-components/frm-dropdown-component/frm-dropdown-component.jsjs/src/web-components/frm-range-slider-component/frm-range-slider-component.jsjs/src/web-components/frm-typography-component/frm-typography-component.js
🧰 Additional context used
🧬 Code graph analysis (4)
js/src/web-components/frm-dropdown-component/frm-dropdown-component.js (1)
js/src/web-components/frm-web-component.js (1)
frmWebComponent(1-157)
js/src/web-components/frm-colorpicker-component/frm-colorpicker-component.js (1)
js/src/web-components/frm-web-component.js (1)
frmWebComponent(1-157)
js/src/web-components/frm-range-slider-component/frm-range-slider-component.js (2)
js/src/web-components/frm-web-component.js (1)
frmWebComponent(1-157)js/src/settings-components/components/slider-component.js (1)
frmSliderComponent(13-562)
js/src/web-components/frm-typography-component/frm-typography-component.js (1)
js/src/web-components/frm-web-component.js (1)
frmWebComponent(1-157)
🪛 GitHub Actions: Inspections
js/src/web-components/frm-border-radius-component/frm-border-radius-component.js
[error] 5-5: ESLint: Block must not be padded by blank lines (padded-blocks).
🪛 GitHub Check: Run ESLint
js/src/web-components/frm-border-radius-component/frm-border-radius-component.js
[failure] 5-5:
Block must not be padded by blank lines
js/src/web-components/frm-dropdown-component/frm-dropdown-component.js
[failure] 4-4:
Block must not be padded by blank lines
js/src/web-components/frm-colorpicker-component/frm-colorpicker-component.js
[failure] 4-4:
Block must not be padded by blank lines
js/src/web-components/frm-range-slider-component/frm-range-slider-component.js
[warning] 351-351:
Expected JSDoc block lines to be aligned
[warning] 321-321:
Expected JSDoc block lines to be aligned
[failure] 246-246:
There must be a space before this paren
[failure] 237-237:
Expected @param names to be "fieldValue". Got "options"
[failure] 234-234:
Missing JSDoc @param "fieldValue" declaration
[warning] 185-185:
Expected JSDoc block lines to be aligned
[failure] 6-6:
Block must not be padded by blank lines
js/src/web-components/frm-typography-component/frm-typography-component.js
[failure] 5-5:
Block must not be padded by blank lines
⏰ 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). (3)
- GitHub Check: PHP 7.4 tests in WP 6.9
- GitHub Check: PHP 8 tests in WP 6.9
- GitHub Check: Cypress
🔇 Additional comments (13)
js/src/web-components/frm-colorpicker-component/frm-colorpicker-component.js (1)
1-3: LGTM on remaining segments.
- Imports, constructor setup,
initView(),getInput()structure,useShadowDom()returning false (correct for jQuery plugin integration),onChangesetter with proper validation, and the pro/lite export pattern all look appropriate.Also applies to: 9-13, 15-20, 22-39, 41-43, 72-88
js/src/web-components/frm-dropdown-component/frm-dropdown-component.js (5)
31-41: LGTM!The null checks for
componentIdandfieldNamehave been added, addressing the previous review concern about settingid="null"orname="null"on the select element.
73-78: LGTM!The previous concern about
_onChangebeing null has been addressed by initializing the private field#onChangeto an empty function() => {}. This ensures the callback is always safe to invoke.
112-118: LGTM!Good defensive validation - throwing a
TypeErrorwith a descriptive message when an invalid callback type is provided helps catch API misuse early.
126-131: LGTM!The
selectedValuesetter correctly usesArray.fromwithfindto locate and select the option, and gracefully handles the case where the value doesn't match any option.
134-141: LGTM!The global helper function provides a clean factory pattern for creating and configuring dropdown instances. The sequence of setting
id,addOptions, andonChangebefore the element is attached to the DOM is correct sinceconnectedCallbackwill properly initialize options when the element is eventually appended.js/src/web-components/frm-typography-component/frm-typography-component.js (4)
49-97: LGTM!The
initView,getSelect, andgetDefaultOptionsmethods are well-structured with proper conditional attribute assignment.
140-177: LGTM!The
getUnitTypeSelectandgetHiddenInputmethods are correctly implemented.
184-192: LGTM!The
#onChangecallback is now initialized to an empty function by default, addressing the previous null-check concern.
217-239: LGTM!The setters include proper type validation for
onChangeand clean assignment fortypographyDefaultValue.js/src/web-components/frm-border-radius-component/frm-border-radius-component.js (3)
17-33: LGTM!The
initViewmethod is well-structured with clear separation of concerns.
200-210: LGTM!The value building and update methods correctly construct CSS
border-radiussyntax and propagate changes via the callback.
212-221: LGTM!The button now has
type="button"to prevent form submission, and the toggle logic is clean.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
…eb-components-part-2
…eb-components-part-2
…-part-2' of github.com:Strategy11/formidable-forms into feature/make-style-elements-accesible-as-web-components-part-2
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
js/src/settings-components/components/slider-component.js (2)
40-52: Guard steps parsing and validate array shape.Untrusted dataset content can throw in JSON.parse or yield non-arrays, breaking slider init and later index/length usage.
🛠️ Suggested fix
- const steps = this.settings.steps || ( element.dataset.steps ? JSON.parse( element.dataset.steps ) : null ); + let steps = null; + if ( this.settings.steps ) { + steps = Array.isArray( this.settings.steps ) ? this.settings.steps : null; + } else if ( element.dataset.steps ) { + try { + const parsedSteps = JSON.parse( element.dataset.steps ); + steps = Array.isArray( parsedSteps ) ? parsedSteps : null; + } catch ( error ) { + steps = null; + } + }
189-203: Snap track position to stepped values.For stepped sliders, the value snaps but the track/handle stays at the raw pointer position, so visuals can diverge from the actual value (and child sliders inherit the unsnapped width).
🛠️ Suggested fix
- const deltaX = event.clientX - sliderRect.left - this.sliderBulletWidth; + let deltaX = event.clientX - sliderRect.left - this.sliderBulletWidth; const unit = element.querySelector( 'select' ).value; - const value = this.calculateValue( sliderWidth, deltaX, this.getMaxValue( unit, index ), this.options[ index ].steps ); + const steps = this.options[ index ].steps; + const value = this.calculateValue( sliderWidth, deltaX, this.getMaxValue( unit, index ), steps ); + if ( steps && steps.length > 0 ) { + deltaX = this.calculateDeltaXFromSteps( value, steps, sliderWidth ); + }- const sliderWidth = element.querySelector( '.frm-slider' ).offsetWidth; + const sliderWidth = element.querySelector( '.frm-slider' ).offsetWidth; + const trackWidth = sliderWidth - this.sliderBulletWidth; ... - const value = this.calculateValue( sliderWidth, deltaX, this.getMaxValue( unit, index ), this.options[ index ].steps ); + const steps = this.options[ index ].steps; + const value = this.calculateValue( steps && steps.length ? trackWidth : sliderWidth, deltaX, this.getMaxValue( unit, index ), steps ); + if ( steps && steps.length > 0 ) { + deltaX = this.calculateDeltaXFromSteps( value, steps, trackWidth ); + }Also applies to: 393-418
🤖 Fix all issues with AI agents
In `@js/src/settings-components/components/slider-component.js`:
- Around line 342-351: calculateDeltaXFromSteps currently divides by
(steps.length - 1) which is zero when steps has a single element, causing NaN;
update calculateDeltaXFromSteps to early-handle the single-step case (when
steps.length === 1) by returning 0 (or sliderWidth if you want a full track) so
no division occurs, and keep the existing logic for other lengths (also ensure
snapToStep usage remains unchanged when value is not exactly in steps).
♻️ Duplicate comments (4)
js/src/web-components/frm-colorpicker-component/frm-colorpicker-component.js (3)
6-47: Honor thevalueattribute fromfrmWebComponentinstead of a hardcoded default.
frmWebComponent.initOptions()setsthis.defaultValue, butgetInput()/afterViewInit()use#defaultValueonly. This ignores attribute-provided defaults and forces#ffffffunless the setter is used.🛠️ Proposed fix
- `#defaultValue` = '#ffffff'; + `#defaultValue` = '#ffffff'; @@ - if ( null !== this.#defaultValue ) { - this.input.value = this.#defaultValue; - } + const defaultValue = this.defaultValue ?? this.#defaultValue; + if ( null !== defaultValue ) { + this.input.value = defaultValue; + } @@ - const colorPickerOptions = { - defaultColor: this.#defaultValue, - }; + const colorPickerOptions = { + defaultColor: this.defaultValue ?? this.#defaultValue, + };
44-53: Guard wpColorPicker initialization or ensure the dependency is loaded.If
wpColorPickerisn’t available yet, this will throw at runtime. Consider a defensive check or ensurewp-color-pickeris a registered dependency for the bundle.🛠️ Defensive init
- jQuery( this.input ).wpColorPicker( colorPickerOptions ); + const $input = jQuery( this.input ); + if ( 'function' !== typeof $input.wpColorPicker ) { + return; + } + $input.wpColorPicker( colorPickerOptions );
70-73: Keep the wpColorPicker UI in sync when settingcolor.Updating
input.valuealone doesn’t update the picker UI after initialization. Use the picker API when available.🛠️ Sync setter with wpColorPicker
set color( value ) { this.#defaultValue = value; this.input.value = value; + if ( window.jQuery && typeof jQuery.fn.wpColorPicker === 'function' ) { + jQuery( this.input ).wpColorPicker( 'color', value ); + } }js/src/web-components/frm-typography-component/frm-typography-component.js (1)
199-213: Guard the regex match to avoid a runtime TypeError.
value.match(...)can returnnullfor unexpected values, causingmatch[2]to throw.🛠️ Proposed fix
getUnitValue( value ) { const defaultValue = { value: 0, unit: 'px' }; if ( ! value ) { return defaultValue; } - const unitType = value.match( /^([\d.]+)(px|em|%)?$/ )[ 2 ] || 'px'; + const match = value.match( /^([\d.]+)(px|em|%)?$/ ); + if ( ! match ) { + return defaultValue; + } + const unitType = match[ 2 ] || 'px'; @@ return { - value: parseInt( value ), + value: parseInt( match[ 1 ], 10 ), unit: unitType }; }
|
Here's the code health analysis summary for commits Analysis Summary
|
|
Here's the code health analysis summary for commits Analysis Summary
|
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Fix all issues with AI agents
In `@classes/controllers/FrmAppController.php`:
- Around line 1698-1701: The code currently makes a hard static call to
FrmProFormsController::enqueue_pro_web_components_script(), which triggers
PHPStan since the Pro class may be absent; instead keep the is_callable check
but replace the direct static invocation with a dynamic call such as
call_user_func(array('FrmProFormsController',
'enqueue_pro_web_components_script')) (or
call_user_func('FrmProFormsController::enqueue_pro_web_components_script')) so
the runtime behavior is preserved without a direct static class reference;
update the body of enqueue_components_scripts to use that dynamic callable
invocation referencing the same method name.
- Around line 689-693: The dependency array in FrmAppController::admin_js
currently uses FrmAppHelper::meets_min_pro_version('x.x') alone, which can
return true when Pro isn’t installed; update the condition to require Pro
actually be installed as well (e.g., call a pro_is_installed() or
FrmAppHelper::pro_is_installed()) so the 'formidable-pro-web-components'
dependency is only added when both pro_is_installed() and
FrmAppHelper::meets_min_pro_version('x.x') are true, preventing Lite from
depending on the Pro script.
In `@js/formidable-settings-components.js`:
- Line 1: The h class constructor currently may leave this.data undefined when
JSON parsing fails or when initPropagationList returns an empty array; ensure
the constructor assigns safe defaults (e.g., this.data = { propagateInputs: [],
changeEvent: new Event("change",{bubbles:true}) }) in the catch block and after
parsing so propagateInputs always exists, and update updateAllDependentElements
to guard on this.data && this.data.propagateInputs.length > 0 (and that
propagateInputs[0] is defined) before accessing or dispatching the changeEvent.
In `@js/src/settings-components/components/slider-component.js`:
- Line 42: The current expression that computes steps (const steps =
this.settings.steps || ( element.dataset.steps ? JSON.parse(
element.dataset.steps ) : null );) can throw if element.dataset.steps contains
invalid JSON; wrap the JSON.parse call in a try/catch (or use a safeParse
helper) and fall back to null or a sensible default on parse error, optionally
logging a warning, so slider initialization continues without throwing.
In `@js/src/web-components/frm-dropdown-component/frm-dropdown-component.css`:
- Line 1: The CSS includes a redundant background-position declaration that gets
overridden by the following background shorthand in the .frm-dropdown-component
select rule; remove the standalone background-position: center 5px to avoid dead
code and keep the intended position from the shorthand (background:
rgba(0,0,0,0) url(...) no-repeat right 8px top 50%).
In `@js/src/web-components/frm-typography-component/frm-typography-component.js`:
- Around line 150-154: The unit value change handler computes the new combined
value but doesn't update the form's hidden input, leaving submissions stale;
inside the this.unitValueInput 'change' listener (the block that currently calls
this.changeSelectValue(value) and this.#onChange(value)) also set the hidden
input to the new value—either by calling the existing routine that updates the
hidden input (e.g., this.updateHiddenInput(value) if present) or directly
assigning this.hiddenInput.value = value (ensuring this.hiddenInput is non-null)
so the hidden input stays in sync with changeSelectValue and `#onChange`.
- Around line 147-149: The initialization uses parseInt and can produce NaN or
drop decimals; replace the parseInt approach and initialize unitValueInput.value
using the same parsing routine as getUnitValue so non-numeric defaults (e.g.,
"custom") yield an empty string and numeric defaults preserve decimals—e.g.,
call the component's getUnitValue (or reuse its logic) with this.#defaultValue
and assign its result to this.unitValueInput.value, removing parseInt and
guarding against NaN.
- Around line 164-180: The unit-type select created in getUnitTypeSelect does
not update component state or notify listeners on change; attach a change event
listener to this.unitTypeSelect inside getUnitTypeSelect that (1) sets the
corresponding hidden/field value (e.g., update the hidden input property used
for form submission such as this.hiddenInput or the field keyed by
this.fieldName + '[unit-type]') and (2) emits a component-level notification
(dispatch a CustomEvent or call the component's existing change handler) with
the new unit type so external listeners are notified; ensure you reference
this.unitTypeSelect and this.fieldName in the handler so the value and event
payload remain consistent.
🧹 Nitpick comments (2)
js/src/web-components/frm-dropdown-component/frm-dropdown-component.css (1)
1-1: Consider reducing!importantusage in component label styles.The
.frm-component-labelstyles use multiple!importantdeclarations (font-size,color,width,display,margin-right). This pattern often indicates specificity conflicts that could be addressed by restructuring selectors or increasing selector specificity naturally, improving long-term maintainability.js/src/web-components/frm-border-radius-component/frm-border-radius-component.js (1)
144-156: Consider caching parsed default values.
parseDefaultValues()is called separately in each of the fourgetBorderInput*methods, redundantly parsing the same string four times. Consider parsing once and reusing.♻️ Proposed optimization
Cache the parsed values at the start of
getBorderIndividualInputsWrapper()and pass them to each input method:getBorderIndividualInputsWrapper() { this.borderIndividualInputsWrapper = document.createElement( 'div' ); this.borderIndividualInputsWrapper.classList.add( 'frm-border-individual-inputs-wrapper' ); if ( ! this.#usesMultipleValues ) { this.borderIndividualInputsWrapper.classList.add( 'frm_hidden' ); } + const defaultValues = this.parseDefaultValues(); + this.borderIndividualInputsWrapper.append( - this.getBorderInputTop(), - this.getBorderInputRight(), - this.getBorderInputLeft(), - this.getBorderInputBottom() + this.getBorderInputTop( defaultValues ), + this.getBorderInputRight( defaultValues ), + this.getBorderInputLeft( defaultValues ), + this.getBorderInputBottom( defaultValues ) ); return this.borderIndividualInputsWrapper; } -getBorderInputTop() { - const defaultValues = this.parseDefaultValues(); +getBorderInputTop( defaultValues ) { // ...rest unchanged }
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@css/admin/frm-settings-components.css`:
- Line 1: The rule ".frm-style-component .frm-radio-container
input{display:none}" makes radio inputs inaccessible; replace that display:none
with a visually-hidden accessible pattern for ".frm-style-component
.frm-radio-container input" (keep the inputs in the DOM and focus/order) by
applying off-screen/clip or inset-based hiding while preserving focusability and
screen-reader access, and add a visible focus style (e.g., outline or box-shadow
on :focus or :focus-visible) to the same selector so keyboard users can see
focus.
In
`@js/src/web-components/frm-border-radius-component/frm-border-radius-component.scss`:
- Around line 46-58: The current rule under the input selector removes keyboard
focus styling by using "&:focus { outline: none; }" which is an accessibility
issue; restore a visible focus indicator by removing the bare outline:none and
replacing it with an accessible focus style (for example use :focus or
:focus-visible on the input and select selectors to apply a high-contrast
outline or box-shadow and maintain existing layout via box-sizing), and apply
the same change to the select rules referenced around lines 103-126 so keyboard
users get a clear visible focus state for both input and select elements.
- Around line 18-24: The background shorthand in
frm-border-radius-component.scss (the block that currently sets background:
white; then background: url(...)) is overwriting the white fill; fix by keeping
the white background-color and applying the SVG as an image instead of using a
full shorthand—either replace the second background: with a background-image:
url(...) plus background-repeat/position, or combine into a single shorthand
that includes the color (e.g., background: white url(...) no-repeat center
center) and keep background-size: 20px; ensure background-color remains white so
the SVG overlay doesn’t make the element transparent.
🧹 Nitpick comments (1)
js/src/web-components/frm-colorpicker-component/frm-colorpicker-component.js (1)
48-48: Redundant type check for#onChange.The check
'function' === typeof this.#onChangeis always true because#onChangeis initialized to() => {}and the setter throws if a non-function is passed.♻️ Simplified version
- if ( 'function' === typeof this.#onChange ) { - // Use the setTimeout when calling the callback for onChange to make sure the latest color is fetched. - colorPickerOptions.change = ( event, ui ) => setTimeout( () => this.#onChange( event, ui ), 20 ); - } + // Use the setTimeout when calling the callback for onChange to make sure the latest color is fetched. + colorPickerOptions.change = ( event, ui ) => setTimeout( () => this.#onChange( event, ui ), 20 );
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2711 +/- ##
============================================
- Coverage 27.06% 27.04% -0.03%
- Complexity 8866 8873 +7
============================================
Files 145 145
Lines 29868 29885 +17
============================================
- Hits 8085 8083 -2
- Misses 21783 21802 +19 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
js/src/web-components/frm-web-component.js (1)
55-70:⚠️ Potential issue | 🟠 MajorPass the wrapper to
afterViewInitfor shadow DOM components.
afterViewInitis documented to receive the wrapper, but currently receives the host. For shadow DOM components, host queries won’t see shadow content. Please passwrapperinstead.✅ Suggested fix
- this.whenElementBecomesVisible().then( () => this.afterViewInit( this ) ); + this.whenElementBecomesVisible().then( () => this.afterViewInit( wrapper ) );
🤖 Fix all issues with AI agents
In `@stubs.php`:
- Around line 537-541: The stub class FrmProFormsController currently lives in
the Elementor namespace but is invoked unqualified from the global scope via
FrmProFormsController::enqueue_pro_web_components_script(); fix by either moving
the stub declaration into the global namespace (declare class
FrmProFormsController { public static function
enqueue_pro_web_components_script() {} }) or by updating the call site to
reference the fully-qualified class name
\Elementor\FrmProFormsController::enqueue_pro_web_components_script(); ensure
the referenced symbol (FrmProFormsController and its method
enqueue_pro_web_components_script) match exactly between the stub and the call
site.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@js/src/web-components/frm-typography-component/frm-typography-component.js`:
- Around line 219-233: The getUnitValue static method currently uses parseInt on
the original value which truncates decimals; update getUnitValue to parse the
numeric portion from the regex match (match[1]) with parseFloat instead of
parseInt and return that parsed number along with match[2] (or default 'px') so
inputs like "1.5em" preserve the decimal part; ensure you still return
defaultValue when there is no match or no value.
🧹 Nitpick comments (5)
js/src/web-components/frm-range-slider-component/frm-range-slider-component.js (5)
91-93: Missing JSDoc forstepssetter.For consistency with other setters in this class, consider adding a JSDoc comment documenting the parameter type and purpose.
106-113: Dead code:fieldNamewith attribute syntax is unused.The
fieldNameproperty at line 111 constructs an HTML attribute string (name="...") but is never destructured or used increateMultipleValuesSlider(line 208 only extractsmaxValue, units, componentClass, fieldValue, defaultValues). Consider removing this unused property from the config object.🧹 Proposed fix
const config = { maxValue: parseInt( this.getAttribute( 'data-max-value' ) || this.#sliderMaxValue, 10 ), units: this.getAvailableUnits(), componentClass: this.getAttribute( 'data-component-class' ) || '', componentId: this.componentId, - fieldName: this.fieldName ? `name="${ this.fieldName }"` : '', fieldValue: this.defaultValue || this.#sliderDefaultValue };
209-224: Internationalize aria labels for accessibility.The
ariaLabelvalues at lines 214 and 221 are hardcoded English strings. For consistency with other i18n usage (e.g., line 357), wrap them with__()for translation support.♻️ Proposed fix
const groups = [ { type: 'vertical', displaySliders: 'top,bottom', iconSvgId: 'frm-margin-top-bottom', - ariaLabel: 'Vertical value', + ariaLabel: __( 'Vertical value', 'formidable' ), defaultValues: defaultValues.vertical, }, { type: 'horizontal', displaySliders: 'left,right', iconSvgId: 'frm-margin-left-right', - ariaLabel: 'Horizontal value', + ariaLabel: __( 'Horizontal value', 'formidable' ), defaultValues: defaultValues.horizontal, } ];
292-303: Internationalize aria labels in slider group items.Similar to the previous comment, the aria label at line 299 should use i18n. Note that the template literal approach complicates translation, so consider using a mapping or sprintf-style translation.
♻️ Proposed fix
+ const ariaLabels = { + top: __( 'Top value', 'formidable' ), + bottom: __( 'Bottom value', 'formidable' ), + left: __( 'Left value', 'formidable' ), + right: __( 'Right value', 'formidable' ) + }; + slidersGroupItems.forEach( item => { slider.append( this.createSlider( { type: item, maxValue: options.maxValue, units: options.units, value: options.defaultValues[ item ], iconSvgId: `frm-margin-${ item }`, - ariaLabel: `${ item } value`, + ariaLabel: ariaLabels[ item ], hidden: true, addHiddenInputValue: false } ) ); } );
359-359: Avoid implicit return inforEachcallback.The arrow function implicitly returns the result of
unitSelect.append(). WhileforEachignores return values, this triggers a static analysis warning. Use braces to make the intent explicit.🧹 Proposed fix
- units.forEach( unit => unitSelect.append( frmRangeSliderComponent.createDropdownOption( unit, unit, value.unit === unit ) ) ); + units.forEach( unit => { + unitSelect.append( frmRangeSliderComponent.createDropdownOption( unit, unit, value.unit === unit ) ); + } );
…eb-components-part-2
…er, switch to href from xlink:href on svgs
…eb-components-part-2
Crabcyborg
deleted the
feature/make-style-elements-accesible-as-web-components-part-2
branch
…accesible-as-web-components-part-2 Feature/make style elements accesible as web components part 2
2 participants
Summary by CodeRabbit
New Features
Refactor
Style
Chores