Use more css vars in single theme css#2646
Conversation
WalkthroughReplaces many PHP-injected CSS values in Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code graph analysis (1)css/_single_theme.css.php (1)
⏰ 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)
🔇 Additional comments (11)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
css/_single_theme.css.php (1)
247-255: Unclosedif ( empty( $submit_bg_img ) )will cause a PHP parse error
if ( empty( $submit_bg_img ) ) {on Line 247 opens a PHP block that is never closed in PHP—the}on Line 255 is CSS, and there is only a single closing}on Line 299, which is already needed forif ( ! $submit_style ). This leaves oneifwithout a matching}and will result in a parse error when this file is loaded.You can fix this by explicitly closing the inner
ifright after the hover rule block:- <?php if ( empty( $submit_bg_img ) ) { ?> -.<?php echo esc_html( $style_class ); ?> .frm-edit-page-btn:hover, + <?php if ( empty( $submit_bg_img ) ) { ?> +.<?php echo esc_html( $style_class ); ?> .frm-edit-page-btn:hover, .<?php echo esc_html( $style_class ); ?> input[type=submit]:hover, .<?php echo esc_html( $style_class ); ?> .frm_submit input[type=button]:hover, .<?php echo esc_html( $style_class ); ?> .frm_submit button:hover{ background: var(--submit-hover-bg-color)<?php echo esc_html( $important ); ?>; border-color: var(--submit-hover-border-color)<?php echo esc_html( $important ); ?>; color: var(--submit-hover-color)<?php echo esc_html( $important ); ?>; } - -.<?php echo esc_html( $style_class ); ?>.frm_center_submit .frm_submit .frm_ajax_loading{ + <?php } ?> + +.<?php echo esc_html( $style_class ); ?>.frm_center_submit .frm_submit .frm_ajax_loading{Also applies to: 297-300
🧹 Nitpick comments (1)
css/_single_theme.css.php (1)
325-337: Plugin-conditional CSS is good; minor consistency/typo nitsConditionally outputting the Math Captcha and Signatures-specific rules only when
FrmCptController/FrmSigFieldexist is a nice optimization to avoid unnecessary selectors when those add-ons are absent.Two small nits you may want to address:
Line 325 comment has a typo:
inclide→include.Line 368 hard-codes
!importanton the signatures error border:
border-color:var(--border-color-error) !important;
If you want this to behave like the other error-related declarations, consider using$importantinstead for consistency:
- border-color:var(--border-color-error) !important;
- border-color:var(--border-color-error);
Also applies to: 365-370 </blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used**: CodeRabbit UI **Review profile**: CHILL **Plan**: Pro <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 70eddfce98ff97fd80b40c5385c46f4aa02cc196 and e643a5b2f3c2fc2ef0cb531c2a582f2f3d65ac6b. </details> <details> <summary>📒 Files selected for processing (1)</summary> * `css/_single_theme.css.php` (12 hunks) </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>🧬 Code graph analysis (1)</summary> <details> <summary>css/_single_theme.css.php (1)</summary><blockquote> <details> <summary>classes/helpers/FrmAppHelper.php (2)</summary> * `FrmAppHelper` (6-5084) * `kses_echo` (1075-1077) </details> </blockquote></details> </details> </details> <details> <summary>⏰ 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). (2)</summary> * GitHub Check: Cypress * GitHub Check: Cypress </details> <details> <summary>🔇 Additional comments (1)</summary><blockquote> <details> <summary>css/_single_theme.css.php (1)</summary><blockquote> `22-24`: **CSS custom property refactor looks consistent and preserves existing semantics** The switch from hard-coded values to CSS vars for form width/direction, field margins, label/placeholder/disabled colors, active/hover/loading states, error/invalid styling, and inline submit labels all looks consistent. `$important` is correctly appended everywhere in these hunks, so the existing “Important” style toggle behavior should be preserved while gaining more flexible theming via the new `--*` variables. Also applies to: 39-42, 120-123, 147-162, 164-182, 251-254, 267-270, 281-285, 288-296, 308-313, 331-333, 358-363, 372-383, 392-394 </blockquote></details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
css/_single_theme.css.php (1)
386-394: Missing $important flag on margin-bottom (previously flagged).Lines 387-391 correctly migrate to CSS variables with proper
$importantflag handling. However, Line 393'smargin-bottom:var(--field-margin);is missing the$importantflag, making it inconsistent with all other properties in the.frm_error_styleblock.This issue was flagged in a previous review and has not been addressed yet. When the "Important" style toggle is enabled, all error box properties except margin-bottom will have
!important, which could lead to unexpected spacing behavior.Apply this diff to add the missing
$importantflag:.<?php echo esc_html( $style_class ); ?> .frm_error_style{ background-color:var(--error-bg)<?php echo esc_html( $important ); ?>; border:1px solid var(--error-border)<?php echo esc_html( $important ); ?>; border-radius:var(--border-radius)<?php echo esc_html( $important ); ?>; color: var(--error-text)<?php echo esc_html( $important ); ?>; font-size:var(--error-font-size)<?php echo esc_html( $important ); ?>; margin:0; - margin-bottom:var(--field-margin); + margin-bottom:var(--field-margin)<?php echo esc_html( $important ); ?>; }
🧹 Nitpick comments (1)
css/_single_theme.css.php (1)
369-378: Signature field error styling correctly gated, but consider $important consistency.The conditional block appropriately uses
class_exists('FrmSigField')to gate signature-specific styling. The migration to--border-color-erroris correct.However, Line 374 uses a hardcoded
!importantinstead of the$importantvariable used throughout the rest of the file. Consider whether this should be:- border-color:var(--border-color-error) !important; + border-color:var(--border-color-error)<?php echo esc_html( $important ); ?>;This would maintain consistency with the pattern used elsewhere, unless the hardcoded
!importantis intentionally required for signature fields.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
css/_single_theme.css.php(12 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
css/_single_theme.css.php (1)
classes/helpers/FrmAppHelper.php (1)
FrmAppHelper(6-5084)
⏰ 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 (13)
css/_single_theme.css.php (13)
23-24: LGTM! Form-level properties migrated to CSS variables.The conversion to
--form-widthand--directionCSS variables is correct, and the$importantflag is properly preserved.
41-41: LGTM! Field spacing and width migrated to CSS variables.The conversion to
--field-marginand--widthis correct and maintains the$importantflag handling.Also applies to: 122-122
149-149: LGTM! Placeholder styling migrated to CSS variables.The use of
--text-color-disabledfor placeholder text is semantically appropriate and correctly preserves the$importantflag.Also applies to: 161-161
180-181: LGTM! Active field state styling migrated to CSS variables.The conversion to
--bg-color-activeand--border-color-activeis correct with proper$importantflag handling.
251-253: LGTM! Submit button hover state migrated to CSS variables.The conversion to
--submit-hover-*variables is consistent and correctly maintains the$importantflag for all three properties.
267-269: LGTM! Submit button active/focus state migrated to CSS variables.The conversion to
--submit-active-*variables is correct and properly preserves the$importantflag.
281-283: LGTM! Loading state styling migrated to CSS variables.The use of
--submit-bg-colorfor both background and border during loading creates the intended uniform appearance. The$importantflag is correctly preserved.
288-295: LGTM! Loading spinner styling migrated to CSS variables.The conversion to
--submit-text-color,--submit-height, and--submit-widthis correct, and the conditional checks for 'auto' values are properly preserved.
308-312: LGTM! Inline submit alignment styling migrated to CSS variables.The conversion to
--font-size,--label-color,--weight, and--label-paddingcorrectly ensures the invisible pseudo-element matches label dimensions for proper inline form alignment.
325-340: LGTM! Math captcha styling correctly gated by plugin check.The conditional block appropriately uses
class_exists('FrmCptController')to prevent generating unnecessary CSS when the plugin is inactive. The migration to--font-size,--label-color, and--weightvariables is consistent with the rest of the file.
362-366: LGTM! Error field styling migrated to CSS variables.The conversion to
--text-color-error,--bg-color-error,--border-color-error,--border-width-error, and--border-style-erroris comprehensive and consistent, with proper$importantflag handling.
382-383: LGTM! Error message text styling migrated to CSS variables.The conversion to
--weightand--border-color-erroris correct, ensuring error messages match the error border color for visual consistency.
401-401: LGTM! Progress bar styling migrated to CSS variables.The conversion to
--bg-coloris correct and properly preserves the$importantflag.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
css/_single_theme.css.php (1)
386-394: Apply$importantto.frm_error_stylemargin-bottomfor consistency
margin-bottomon.frm_error_styleis still the only property in this rule not respecting the$importanttoggle, so enabling “Important” won’t affect the error block’s bottom spacing. This is inconsistent with nearby declarations and the rest of the theme controls.Recommend aligning it with the other properties:
.<?php echo esc_html( $style_class ); ?> .frm_error_style{ background-color:var(--error-bg)<?php echo esc_html( $important ); ?>; border:1px solid var(--error-border)<?php echo esc_html( $important ); ?>; border-radius:var(--border-radius)<?php echo esc_html( $important ); ?>; color: var(--error-text)<?php echo esc_html( $important ); ?>; font-size:var(--error-font-size)<?php echo esc_html( $important ); ?>; margin:0; - margin-bottom:var(--field-margin); + margin-bottom:var(--field-margin)<?php echo esc_html( $important ); ?>; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
.github/workflows/syntax.ymlis excluded by!**/*.yml
📒 Files selected for processing (1)
css/_single_theme.css.php(12 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
css/_single_theme.css.php (1)
classes/helpers/FrmAppHelper.php (2)
FrmAppHelper(6-5084)kses_echo(1075-1077)
⏰ 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 trunk
- GitHub Check: PHP 7.4 tests in WP trunk
🔇 Additional comments (2)
css/_single_theme.css.php (2)
23-24: CSS variable substitutions look consistent and preserve$importantbehaviorThe conversions to
var(--…)<?php echo esc_html( $important ); ?>across these rules (form width/direction, field margins, right position width, placeholders, focus/active states, submit hover/active/loading, inline labels, error colors, and progress bar) look coherent and match the previous semantics while centralizing styling in CSS variables. No functional concerns from these changes.Also applies to: 41-41, 122-122, 149-149, 161-161, 180-181, 251-253, 267-269, 281-283, 288-295, 308-310, 312-312, 362-366, 382-383, 387-391, 401-401
325-340: Math captcha CSS gating behindFrmCptControllerlooks goodThe new guard around
#frm_field_cptch_number_containerensures this block only outputs when the related plugin class exists, and the switch tovar(--font-size),var(--label-color), andvar(--weight)with$importantmatches the pattern used elsewhere. This aligns with the PR goal of avoiding unnecessary styles for inactive add-ons.
…theme_css Use more css vars in single theme css
1 participant
In addtion