[7418] FE2- Checkbox control #4658
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
WalkthroughCheckbox control now computes readonly using Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant UI as Checkbox Component
participant Field as Field Meta
participant Form as FormsEngine State
Note over UI,Field: Render / init
UI->>Field: read defaultValue and metadata
UI->>UI: compute readonly = isFieldReadOnly(field, formReadonly)
alt valueProp present (nnou)
UI->>UI: localValue = valueProp
else
UI->>UI: localValue = field.defaultValue (if non-empty)
UI->>Form: useEffect -> setValue(defaultValue) (when initialized)
end
UI->>Form: onChange -> setValue(e.target.checked)
sequenceDiagram
autonumber
participant Src as Source Value
participant VR as booleanFieldExtractor
Src-->>VR: value (boolean | string | undefined | '')
VR-->>Src: process per JSDoc and return boolean (implementation unchanged)
sequenceDiagram
autonumber
participant Validator as validatorsMap
participant CheckboxVal as checkboxValidator
participant Field as Field Meta
Validator->>CheckboxVal: invoke (field, currentValue as boolean, messages)
alt field.required and currentValue === false
CheckboxVal->>Validator: push message, return false
else
CheckboxVal->>Validator: return true
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (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). (2)
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
ui/app/src/components/FormsEngine/controls/Checkbox.tsx(1 hunks)ui/app/src/components/FormsEngine/lib/valueRetrievers.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
ui/app/src/components/FormsEngine/controls/Checkbox.tsx (2)
ui/app/src/components/FormsEngine/types.ts (1)
ControlProps(23-30)ui/app/src/utils/object.ts (1)
nnou(141-143)
ui/app/src/components/FormsEngine/lib/valueRetrievers.ts (1)
ui/app/src/utils/object.ts (1)
nnou(141-143)
⏰ 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)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Travis CI - Pull Request
…o feature/checkbox # Conflicts: # ui/app/src/components/FormsEngine/lib/valueRetrievers.ts
…FieldValue to omit empty strings as defaultValue
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
ui/app/src/components/FormsEngine/lib/valueRetrievers.ts (1)
195-198: Fix the return type mismatch in booleanFieldExtractor.Line 196 declares the return type as
boolean, but line 197 returnsnullwhen the value is empty or not set. This creates a type mismatch that will fail strict null checks.Apply this diff to fix the return type:
-export function booleanFieldExtractor(value: unknown): boolean { +export function booleanFieldExtractor(value: unknown): boolean | null { return nnou(value) && value !== '' ? value === true || value === 'true' : null; }
🧹 Nitpick comments (1)
ui/app/src/components/FormsEngine/lib/validators.ts (1)
35-35: Consider updating the type cast to reflect nullable boolean values.The cast
currentValue as booleanis misleading sincebooleanFieldExtractorreturnsboolean | null(null when the value is empty or not set). While the validator logic correctly handles null via!currentValue, the type annotation doesn't reflect reality.Apply this diff to make the type more accurate:
- checkbox: (field, currentValue, messages) => checkboxValidator(field, currentValue as boolean, messages), + checkbox: (field, currentValue, messages) => checkboxValidator(field, currentValue as boolean | null, messages),And update the validator signature:
-export function checkboxValidator(field: ContentTypeField, currentValue: boolean, messages: FieldValidityMessage[]) { +export function checkboxValidator(field: ContentTypeField, currentValue: boolean | null, messages: FieldValidityMessage[]) {
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
ui/app/src/components/FormsEngine/controls/Checkbox.tsx(1 hunks)ui/app/src/components/FormsEngine/lib/validators.ts(2 hunks)ui/app/src/components/FormsEngine/lib/valueRetrievers.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
ui/app/src/components/FormsEngine/lib/valueRetrievers.ts (1)
ui/app/src/utils/object.ts (1)
nnou(141-143)
ui/app/src/components/FormsEngine/controls/Checkbox.tsx (2)
ui/app/src/components/FormsEngine/types.ts (1)
ControlProps(23-30)ui/app/src/components/FormsEngine/lib/formUtils.tsx (1)
isFieldReadOnly(764-766)
ui/app/src/components/FormsEngine/lib/validators.ts (1)
ui/app/src/models/ContentType.ts (1)
ContentTypeField(64-142)
⏰ 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)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Travis CI - Pull Request
🔇 Additional comments (2)
ui/app/src/components/FormsEngine/lib/validators.ts (1)
107-116: LGTM! Checkbox validation logic is correct.The validator correctly enforces that required checkboxes must be checked (truthy). The condition
!currentValueappropriately handlesnull(from booleanFieldExtractor when no value is set) andfalse(unchecked).ui/app/src/components/FormsEngine/lib/valueRetrievers.ts (1)
154-154: LGTM! Default value handling is now more precise.The updated logic correctly falls back to
defaultValueonly when it's non-null/non-undefined and not an empty string, otherwiseundefined. This prevents empty strings from being treated as meaningful defaults.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
craftercms/craftercms#7418
Summary by CodeRabbit
New Features
Bug Fixes