Updated component accordingly #4070
Conversation
WalkthroughRemoved per-element style and search-related props from the phone number component API, introduced a consolidated styling hook and unified style prop, added a styles module and initialValue wiring, updated settings JSON to match consolidated styles, and added a simple component test. Changes
Sequence Diagram(s)sequenceDiagram
participant Designer as Designer UI / Model
participant PhoneComp as PhoneNumberInputComponent
participant ConfigItem as ConfigurableFormItem
participant PhoneInput as PhoneInput (ui)
Designer->>PhoneComp: provide model (includes styles, initialValue, enableArrow)
PhoneComp->>PhoneComp: useStyles() & compute componentStyle (useMemo)
PhoneComp->>ConfigItem: render with model + initialValue
ConfigItem->>PhoneInput: render child with style={componentStyle}
PhoneInput-->>ConfigItem: user input / validation
ConfigItem-->>PhoneComp: value/validation events
PhoneComp-->>Designer: propagate changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ 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: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
shesha-reactjs/src/designer-components/phoneNumber/interface.ts(1 hunks)shesha-reactjs/src/designer-components/phoneNumber/phoneNumber.tsx(7 hunks)shesha-reactjs/src/designer-components/phoneNumber/settingsForm.json(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
shesha-reactjs/src/designer-components/phoneNumber/phoneNumber.tsx (1)
shesha-reactjs/src/providers/form/utils.ts (1)
getStyle(1527-1536)
🔇 Additional comments (7)
shesha-reactjs/src/designer-components/phoneNumber/interface.ts (1)
27-27: LGTM! Consolidation of style props.The unified
stylesprop effectively replaces multiple individual style properties, simplifying the interface and aligning with the consolidated styling approach implemented in the component.shesha-reactjs/src/designer-components/phoneNumber/settingsForm.json (2)
221-221: LGTM! Improved UX with expanded behavior panel.Setting
defaultValue: trueensures the behavior panel is expanded by default, improving discoverability of configuration options.
615-664: LGTM! Well-documented unified styles configuration.The refactored styles editor provides clear documentation through exposed variables and helpful placeholder examples. The consolidation from individual style props to a unified
stylesobject with element-specific keys improves maintainability.shesha-reactjs/src/designer-components/phoneNumber/phoneNumber.tsx (4)
153-156: LGTM! Good use of memoization.The memoization of
componentStylewith proper dependencies prevents unnecessary re-evaluations and improves performance.
376-396: LGTM! Well-implemented unified styles evaluation.The
evaluatedStylesmemoization correctly handles both unified style objects (applied to all elements) and element-specific style objects. The fallback logic ensures consistent styling behavior.
493-493: LGTM! Proper initialValue propagation.Passing
initialValue={model.initialValue}toConfigurableFormItemensures the component correctly initializes with default values from the form configuration.
21-28: CSS sanitization may be too aggressive.Removing all braces
{}will break valid CSS values that use parentheses in different contexts. For example:
rgb(255, 0, 0)would becomergb255, 0, 0calc(100% - 20px)would becomecalc100% - 20pxurl(...)patterns would be brokenConsider a more targeted approach that validates against a whitelist of safe CSS properties/values or uses a CSS sanitization library:
const sanitizeCssValue = (value: any): string => { if (typeof value !== 'string') return String(value); - // Remove potentially dangerous characters: braces, comments, semicolons outside of expected contexts return value - .replace(/[{}]/g, '') // Remove braces .replace(/\/\*[\s\S]*?\*\//g, '') // Remove comments .replace(/<!--[\s\S]*?-->/g, '') // Remove HTML comments + .replace(/[{}]/g, '') // Remove curly braces only (keep parentheses) .trim(); };Or better yet, consider using a dedicated CSS sanitization library like
sanitize-cssor implementing a whitelist-based approach.Likely an incorrect or invalid review comment.
shesha-reactjs/src/designer-components/phoneNumber/phoneNumber.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
shesha-reactjs/src/designer-components/phoneNumber/phoneNumber.tsx (1)
103-103: Fix inconsistent enableArrow defaults (reminder).The
enableArrowproperty still has conflicting defaults:
- Line 103: Defaults to
falsein the component destructuring- Line 506: Set to
trueininitModel- Line 515: Set to
truein the migratorThis inconsistency will cause unpredictable behavior depending on whether a component is newly created or migrated. As discussed in previous review, line 506 (initModel) should be updated to
falsefor consistency.Apply this diff to fix the inconsistency:
initModel: (model) => ({ ...model, valueFormat: model.valueFormat || 'string', stripCountryCode: model.stripCountryCode !== undefined ? model.stripCountryCode : false, defaultCountry: model.defaultCountry || 'za', - enableArrow: model.enableArrow !== undefined ? model.enableArrow : true, + enableArrow: model.enableArrow !== undefined ? model.enableArrow : false, labelAlign: model.labelAlign || 'left', }),Also applies to: 506-506, 515-515
🧹 Nitpick comments (1)
shesha-reactjs/src/designer-components/phoneNumber/phoneNumber.tsx (1)
18-29: Consider additional CSS sanitization patterns.The current sanitization covers common attack vectors effectively. For comprehensive protection, consider also filtering:
behavior:and-moz-binding:(browser-specific script execution)vbscript:protocoldata:protocol (can contain encoded malicious content)Apply this diff to enhance sanitization:
const sanitizeCssValue = (value: any): string => { if (typeof value !== 'string') return String(value); return value .replace(/[{}]/g, '') // Remove braces .replace(/\/\*[\s\S]*?\*\//g, '') // Remove comments .replace(/<!--[\s\S]*?-->/g, '') // Remove HTML comments .replace(/url\s*\(/gi, '') // Remove url() functions .replace(/@import/gi, '') // Remove @import .replace(/expression\s*\(/gi, '') // Remove expression() .replace(/javascript:/gi, '') // Remove javascript: protocol + .replace(/vbscript:/gi, '') // Remove vbscript: protocol + .replace(/data:/gi, '') // Remove data: protocol + .replace(/behavior\s*:/gi, '') // Remove behavior: + .replace(/-moz-binding\s*:/gi, '') // Remove -moz-binding: .trim(); };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
shesha-reactjs/src/designer-components/phoneNumber/phoneNumber.tsx(6 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-06-12T16:55:57.638Z
Learnt from: teboho
PR: shesha-io/shesha-framework#3397
File: shesha-reactjs/src/designer-components/charts/bar.tsx:49-52
Timestamp: 2025-06-12T16:55:57.638Z
Learning: For the chart components’ migrators (e.g., BarChartComponent in shesha-reactjs/src/designer-components/charts/bar.tsx), the version 5 step intentionally spreads `{ ...prev, ...defaultConfigFiller }` so that values from `defaultConfigFiller` override any existing properties in `prev`. This reset to new defaults is by design and should not be flagged as an issue.
Applied to files:
shesha-reactjs/src/designer-components/phoneNumber/phoneNumber.tsx
🧬 Code graph analysis (1)
shesha-reactjs/src/designer-components/phoneNumber/phoneNumber.tsx (1)
shesha-reactjs/src/providers/form/utils.ts (1)
getStyle(1527-1536)
🔇 Additional comments (2)
shesha-reactjs/src/designer-components/phoneNumber/phoneNumber.tsx (2)
152-155: Excellent unified styling approach with proper memoization.The refactoring consolidates per-element style props into a unified
stylesobject with intelligent handling:
componentStyleserves as base styles applied to all elements (lines 152-155)evaluatedStylessmartly detects whether the styles prop contains element-specific keys or should be applied universally (lines 374-395)- Proper memoization prevents unnecessary recalculations (dependencies correctly specified at lines 154, 395, 435)
- CSS generation efficiently combines base and override styles
This is a clean architectural improvement that enhances both maintainability and performance.
Also applies to: 374-395, 400-435
492-492: Good addition of initialValue propagation.Passing
initialValue={model.initialValue}toConfigurableFormItemensures proper form initialization and aligns with the component's updated public API.
IvanIlyichev
left a comment
IvanIlyichev
left a comment
There was a problem hiding this comment.
Hi @WillemBarendKruger. During the initial review of PR #4012, I overlooked an important architectural guideline. We should avoid creating component-level CSS tools and manually generating <style> tags. Instead, please refactor this to use antd-style, our standard library for styling.
For reference, please follow the implementation patterns used in other components.
|
@IvanIlyichev , The problem is that when I don't target the specific classnames, it doesn't style correctly, only certain parts of the component are styled, as you can see in the image. If there is an alternative way please do share. I did find a way around when I made the 2 nested components transparent, which gives the idea that the styling is applied to all components |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
shesha-reactjs/src/designer-components/phoneNumber/__tests__/index.test.tsx (1)
1-14: Test file doesn't cover the phone number component.This test renders a
SimpleComponentwith "Hello React" but doesn't test any functionality of the phone number component that was modified in this PR. Consider adding meaningful tests for the new unified styling approach, initialValue support, or validation behavior.Example test structure:
import React from 'react'; import { render, screen } from '@testing-library/react'; import PhoneNumberInputComponent from '../phoneNumber'; describe('PhoneNumberInputComponent', () => { test('renders with unified styling classes', () => { const model = { id: 'test-phone', type: 'phoneNumberInput', propertyName: 'phone', label: 'Phone Number', }; const { container } = render( <PhoneNumberInputComponent.Factory model={model} /> ); expect(container.querySelector('.sha-phone-number-wrapper')).toBeInTheDocument(); }); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
shesha-reactjs/src/designer-components/phoneNumber/__tests__/index.test.tsx(1 hunks)shesha-reactjs/src/designer-components/phoneNumber/interface.ts(0 hunks)shesha-reactjs/src/designer-components/phoneNumber/phoneNumber.tsx(6 hunks)shesha-reactjs/src/designer-components/phoneNumber/settingsForm.json(4 hunks)shesha-reactjs/src/designer-components/phoneNumber/styles.ts(1 hunks)
💤 Files with no reviewable changes (1)
- shesha-reactjs/src/designer-components/phoneNumber/interface.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-06-12T16:55:57.638Z
Learnt from: teboho
PR: shesha-io/shesha-framework#3397
File: shesha-reactjs/src/designer-components/charts/bar.tsx:49-52
Timestamp: 2025-06-12T16:55:57.638Z
Learning: For the chart components’ migrators (e.g., BarChartComponent in shesha-reactjs/src/designer-components/charts/bar.tsx), the version 5 step intentionally spreads `{ ...prev, ...defaultConfigFiller }` so that values from `defaultConfigFiller` override any existing properties in `prev`. This reset to new defaults is by design and should not be flagged as an issue.
Applied to files:
shesha-reactjs/src/designer-components/phoneNumber/phoneNumber.tsx
🧬 Code graph analysis (2)
shesha-reactjs/src/designer-components/phoneNumber/styles.ts (1)
shesha-reactjs/src/styles/index.ts (2)
createStyles(61-61)css(61-61)
shesha-reactjs/src/designer-components/phoneNumber/phoneNumber.tsx (2)
shesha-reactjs/src/designer-components/phoneNumber/styles.ts (1)
useStyles(3-54)shesha-reactjs/src/providers/form/utils.ts (1)
getStyle(1527-1536)
🔇 Additional comments (8)
shesha-reactjs/src/designer-components/phoneNumber/settingsForm.json (2)
221-221: Clarify the purpose of defaultValue: true on the Behavior panel.Setting
defaultValue: trueon thephone_behavior_panel(line 221) appears to expand the panel by default. Verify this is intentional and consistent with the UX design for the settings form.
592-607: LGTM: Unified style editor aligns with the new styling approach.The consolidation from per-element style editors to a single unified style editor is consistent with the new styling architecture introduced in styles.ts. The updated description and placeholder clearly communicate that inheritable properties cascade to all child elements.
shesha-reactjs/src/designer-components/phoneNumber/phoneNumber.tsx (5)
14-14: LGTM: Unified styling hook imported.The import of
useStylesfrom the new styles module supports the transition to consolidated styling.
96-96: LGTM: Style classes hook usage.The
useStyleshook is correctly invoked to obtain the style classes for the wrapper and validation message.
124-127: LGTM: Memoized style computation.The
componentStyleis properly memoized with dependencies onstyle,formData, andglobalState, preventing unnecessary recalculations.
349-376: LGTM: Unified styling implementation.The rendering now uses the consolidated styling approach:
- Wrapper div with
shaPhoneNumberWrapperclass provides full-width layout and transparent backgrounds- Single
styleprop passed toPhoneInputfor dynamic styling- Validation message styled with
shaPhoneNumberValidationMessageclassThis implementation aligns with the PR objectives and removes the previous per-element styling complexity.
391-391: LGTM: Initial value support added.The
initialValue={model.initialValue}prop correctly wires up default value support for the phone number component.shesha-reactjs/src/designer-components/phoneNumber/styles.ts (1)
43-48: Add theme token to phoneNumber validation message styles.The
useStylesfunction inshesha-reactjs/src/designer-components/phoneNumber/styles.tsmust destructure thetokenparameter and replace the hardcoded error color with Ant Design's theme token. Update line 2:export const useStyles = createStyles(({ css, cx, token }) => {Then update line 44:
color: ${token.colorError};This aligns with the codebase pattern used in other components (e.g.,
imageAnnotation/styles.ts,text/styles/styles.ts) and ensures the validation message respects theme customization.⛔ Skipped due to learnings
Learnt from: teboho PR: shesha-io/shesha-framework#3926 File: shesha-reactjs/src/designer-components/dataTable/tableContext/styles.ts:138-166 Timestamp: 2025-10-02T11:25:33.370Z Learning: In the shesha-framework repository, the hint popover components (sha-quick-search-hint-popover, sha-table-view-selector-hint-popover, sha-table-pager-hint-popover) in shesha-reactjs/src/designer-components/dataTable/tableContext/styles.ts intentionally use hard-coded color `rgb(214, 214, 214)` with `!important` to impose a specific consistent color across themes, overriding theme tokens by design.
2 participants
Updated component according to request: #3898
Summary by CodeRabbit
Refactor
Settings
Tests