show css vars for fields values that are changed later via css_var_prepare_value#2456
show css vars for fields values that are changed later via css_var_prepare_value#2456Liviu-p wants to merge 1 commit into
Conversation
WalkthroughThe Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~7 minutes Possibly related PRs
Suggested labels
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 PHPStan (2.1.17)Note: Using configuration file /phpstan.neon. Note 🔌 MCP (Model Context Protocol) integration is now available in Early Access!Pro users can now connect to remote MCP servers under the Integrations page to get reviews and chat conversations that understand additional development context. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
🧹 Nitpick comments (3)
classes/helpers/FrmStylesHelper.php (3)
446-450: Avoid rebuilding $show_fields on each iterationMinor perf/readability win: make it static within the method so it’s allocated once per request.
Apply:
- // These fields value may be changed later in css_var_prepare_value() as $settings[ $var ] will be the same as $defaults[ $var ] and the condition will be false - $show_fields = array( 'field_border_width', 'border_radius', 'border_width_error', 'box_shadow', 'font' ); + // These fields' values may be changed later in css_var_prepare_value(), even when $settings[ $var ] === $defaults[ $var ]. + static $show_fields = array( 'field_border_width', 'border_radius', 'border_width_error', 'box_shadow', 'font' );
446-450: Validate and echo the prepared value, not the raw setting (optional hardening)Right now the validity check uses the raw $settings[ $var ]. In edge cases, css_var_prepare_value() can sanitize/normalize a value into a safe one that fails the raw check. Consider:
- Compute $prepared = self::css_var_prepare_value( $settings, $var )
- Validate $prepared with css_value_is_valid()
- Echo $prepared
For clarity (non-diff, spans adjacent lines):
$prepared = self::css_var_prepare_value( $settings, $var ); if ( $show && self::css_value_is_valid( $prepared ) ) { echo '--' . esc_html( self::clean_var_name( str_replace( '_', '-', $var ) ) ) . ':' . $prepared . ';'; // phpcs:ignore WordPress.Security.EscapeOutput.OutputNotEscaped }
447-449: Comment grammar/clarityTighten the comment.
Apply:
- // These fields value may be changed later in css_var_prepare_value() as $settings[ $var ] will be the same as $defaults[ $var ] and the condition will be false + // These fields' values may be changed later in css_var_prepare_value(), even when $settings[ $var ] === $defaults[ $var ].
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
classes/helpers/FrmStylesHelper.php(1 hunks)
🔇 Additional comments (1)
classes/helpers/FrmStylesHelper.php (1)
446-450: Correct: always emit CSS vars for values transformed in css_var_prepare_valueThis directly addresses the PR goal. The list covers all keys that css_var_prepare_value can change independently of defaults (font, field_border_width, border_width_error, box_shadow, border_radius). Behavior for other keys remains unchanged.
|
Closing this since #2482 covers the issue. |
2 participants
No description provided.