[Grida Canvas] RC (daily) #374
Conversation
Review or Edit in CodeSandboxOpen the branch in Web Editor • VS Code • Insiders |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
6 Skipped Deployments
|
WalkthroughThis update replaces all usages of the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant InputPropertyNumber
participant ParentComponent
User->>InputPropertyNumber: Types or changes number
InputPropertyNumber->>ParentComponent: onValueChange (immediate, if in auto mode)
User->>InputPropertyNumber: Presses Enter/Arrow or blurs input
InputPropertyNumber->>ParentComponent: onValueCommit (committed value)
Poem
✨ Finishing Touches
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. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. 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 (2)
editor/scaffolds/sidecontrol/ui/number.tsx (1)
162-162: Type assertions are necessary but consider defensive checks.While the discriminated union makes these type assertions safe, consider adding runtime checks for extra safety in development mode.
+// Add development-time safety check +if (process.env.NODE_ENV === 'development') { + if (mode === "auto" && typeof onValueCommit !== 'function') { + console.warn('onValueCommit should be provided in auto mode'); + } +}Also applies to: 168-168, 231-231, 237-237, 286-286, 292-292
editor/scaffolds/sidecontrol/controls/opacity.tsx (1)
42-45: Consider event handling consistency between input and sliderThe slider now only calls
onValueCommitwhile the input field calls bothonValueChangeandonValueCommit. This might create inconsistent behavior if the parent component expects live updates (onValueChange) from both controls.Consider whether the slider should also call
onValueChangefor consistency, or if the commit-only approach is intentionally different for slider interactions.If both should behave consistently for live updates:
- // use commit for slider - onValueCommit?.({ type: "set", value }); + onValueChange?.({ type: "set", value }); + onValueCommit?.({ type: "set", value });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
editor/scaffolds/sidecontrol/controls/box-shadow.tsx(4 hunks)editor/scaffolds/sidecontrol/controls/corner-radius.tsx(5 hunks)editor/scaffolds/sidecontrol/controls/font-size.tsx(1 hunks)editor/scaffolds/sidecontrol/controls/gap.tsx(1 hunks)editor/scaffolds/sidecontrol/controls/gradient.tsx(2 hunks)editor/scaffolds/sidecontrol/controls/length-percentage.tsx(4 hunks)editor/scaffolds/sidecontrol/controls/letter-spacing.tsx(1 hunks)editor/scaffolds/sidecontrol/controls/line-height.tsx(1 hunks)editor/scaffolds/sidecontrol/controls/maxlength.tsx(1 hunks)editor/scaffolds/sidecontrol/controls/opacity.tsx(3 hunks)editor/scaffolds/sidecontrol/controls/padding.tsx(2 hunks)editor/scaffolds/sidecontrol/controls/positioning.tsx(3 hunks)editor/scaffolds/sidecontrol/controls/rotate.tsx(1 hunks)editor/scaffolds/sidecontrol/controls/stroke-width.tsx(1 hunks)editor/scaffolds/sidecontrol/sidecontrol-node-selection.tsx(13 hunks)editor/scaffolds/sidecontrol/ui/index.tsx(2 hunks)editor/scaffolds/sidecontrol/ui/number.tsx(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (6)
editor/scaffolds/sidecontrol/controls/letter-spacing.tsx (3)
editor/scaffolds/sidecontrol/controls/utils/types.ts (1)
TMixed(3-3)editor/grida-canvas/index.ts (1)
NumberChange(1815-1815)editor/scaffolds/sidecontrol/ui/number.tsx (1)
InputPropertyNumber(128-298)
editor/scaffolds/sidecontrol/controls/stroke-width.tsx (3)
editor/scaffolds/sidecontrol/controls/utils/types.ts (1)
TMixed(3-3)editor/grida-canvas/index.ts (1)
NumberChange(1815-1815)editor/scaffolds/sidecontrol/ui/number.tsx (1)
InputPropertyNumber(128-298)
editor/scaffolds/sidecontrol/controls/positioning.tsx (2)
editor/scaffolds/sidecontrol/ui/number.tsx (1)
InputPropertyNumber(128-298)editor/components/lib/utils/index.ts (1)
cn(4-6)
editor/scaffolds/sidecontrol/controls/line-height.tsx (3)
editor/scaffolds/sidecontrol/controls/utils/types.ts (1)
TMixed(3-3)editor/grida-canvas/index.ts (1)
NumberChange(1815-1815)editor/scaffolds/sidecontrol/ui/number.tsx (1)
InputPropertyNumber(128-298)
editor/scaffolds/sidecontrol/ui/index.tsx (1)
editor/components/lib/utils/index.ts (1)
cn(4-6)
editor/scaffolds/sidecontrol/controls/corner-radius.tsx (4)
packages/grida-canvas-schema/grida.ts (1)
IRectangleCorner(1072-1074)editor/scaffolds/sidecontrol/ui/number.tsx (1)
InputPropertyNumber(128-298)packages/grida-canvas-cg/lib.ts (1)
CornerRadius4(428-428)editor/scaffolds/sidecontrol/ui/index.tsx (1)
PropertyInputContainer(50-66)
🔇 Additional comments (39)
editor/scaffolds/sidecontrol/controls/stroke-width.tsx (4)
3-3: LGTM! Clean refactor to new InputPropertyNumber component.The changes correctly replace
PropertyNumberwithInputPropertyNumberand add theonValueCommitcallback for commit-based value handling. The component interface and usage are consistent with the broader refactor pattern.Also applies to: 8-8, 12-12, 15-15, 21-21
3-3: LGTM: Import updated correctlyThe import has been properly updated to use the new
InputPropertyNumbercomponent.
8-8: LGTM: Props interface updated consistentlyThe addition of the optional
onValueCommitprop maintains backward compatibility while enabling the new commit-based event handling pattern.Also applies to: 12-12
15-22: LGTM: Component usage follows the new patternThe component correctly uses
InputPropertyNumberwith bothonValueChangeandonValueCommithandlers. The defaultmode="auto"is appropriate for this stroke width control, providing both live updates and commit events.editor/scaffolds/sidecontrol/controls/box-shadow.tsx (3)
19-19: LGTM! Consistent usage of InputPropertyNumber with fixed mode.The refactor correctly uses
mode="fixed"for all box shadow numeric inputs, which is appropriate since these components handle direct numeric values rather than change objects. The event handlers properly handle the numeric values with fallbacks.Also applies to: 118-127, 131-140, 144-150, 154-160
18-19: LGTM: Imports updated correctlyThe import statements have been properly updated to use the new
InputPropertyNumbercomponent.
118-127: LGTM: Consistent use of commit-only pattern for box shadow valuesThe use of
mode="fixed"with onlyonValueCommit(noonValueChange) is appropriate for box shadow properties, as it prevents expensive live updates during typing and only commits final values. The callback signature correctly expects anumberparameter in fixed mode.Also applies to: 131-140, 144-150, 154-161
editor/scaffolds/sidecontrol/controls/font-size.tsx (4)
7-7: LGTM! Proper integration with auto mode for NumberChange handling.The refactor correctly uses
mode="auto"which is appropriate since this component expectsNumberChangeobjects. BothonValueChangeandonValueCommitare properly passed through, maintaining compatibility with the existing Select dropdown logic.Also applies to: 16-16, 20-20, 24-32
7-7: LGTM: Import updated correctlyThe import has been properly updated to use the new
InputPropertyNumbercomponent.
16-16: LGTM: Props interface updated consistentlyThe addition of the optional
onValueCommitprop maintains backward compatibility while enabling commit-based event handling.Also applies to: 20-20
24-33: LGTM: Component configuration is appropriate for font size controlThe use of
mode="auto"withtype="integer"is perfect for font size input, providing both live preview updates (onValueChange) and committed changes (onValueCommit). The min/step constraints ensure valid font size values.editor/scaffolds/sidecontrol/controls/opacity.tsx (4)
3-3: LGTM! Good use of commit semantics for slider interactions.The refactor correctly uses
mode="auto"and addstype="number"for opacity values. The change to useonValueCommitfor slider interactions is logical since slider changes represent committed user actions, and the comment clearly explains this reasoning.Also applies to: 11-11, 15-15, 25-34, 43-44
3-3: LGTM: Import updated correctlyThe import has been properly updated to use the new
InputPropertyNumbercomponent.
11-11: LGTM: Props interface updated consistentlyThe addition of the optional
onValueCommitprop maintains backward compatibility while enabling commit-based event handling.Also applies to: 15-15
25-34: LGTM: InputPropertyNumber configuration is appropriate for opacityThe use of
mode="auto"withtype="number"is correct for opacity values (0-1 range with decimal precision). The min/max/step constraints ensure valid opacity values.editor/scaffolds/sidecontrol/controls/letter-spacing.tsx (1)
3-3: LGTM! Clean refactor to commit-based input handling.The changes correctly implement the new commit-based pattern:
- Import updated to use the new
InputPropertyNumbercomponent- Added
onValueCommitprop to handle committed value changes- Uses
mode="auto"which is appropriate for theeditor.api.NumberChangecallback type- Maintains all existing functionality while adding enhanced keyboard interactions
Also applies to: 8-8, 12-12, 15-16, 23-23
editor/scaffolds/sidecontrol/controls/line-height.tsx (1)
3-3: LGTM! Consistent with the refactor pattern.The implementation correctly follows the established pattern:
- Updated import and component usage
- Added
onValueCommitcallback for committed value changes- Uses
mode="auto"consistently with other text-related controls- Preserves the minimum value constraint (
min={1}) for line height validationAlso applies to: 8-8, 12-12, 15-16, 23-23
editor/scaffolds/sidecontrol/sidecontrol-node-selection.tsx (1)
313-313: LGTM! Systematic prop updates align with the commit-based pattern.All the numeric control components have been correctly updated to use
onValueCommitinstead ofonValueChange. This ensures:
- Values are committed when user finishes input (blur, Enter, arrow keys) rather than on every keystroke
- Consistent behavior across all numeric inputs in the side panel
- Better performance by reducing unnecessary intermediate updates
The commented-out line at 527-528 clearly shows the transition pattern being applied.
Also applies to: 330-330, 337-337, 401-401, 408-408, 415-415, 527-529, 535-535, 539-539, 608-608, 919-919, 926-926, 933-933, 957-957, 1029-1029, 1043-1043, 1058-1058, 1066-1066, 1108-1108, 1247-1247, 1272-1272, 1279-1279
editor/scaffolds/sidecontrol/controls/padding.tsx (1)
1-1: LGTM! Correct implementation with appropriate mode selection.The changes properly replace the generic
Inputcomponent with the specializedInputPropertyNumber:
- Uses
mode="fixed"which is appropriate since the callback expects rawPaddingvalues rather thanNumberChangeobjects- Preserves the existing value handling logic for both numeric and object padding values
- Maintains styling and validation constraints (
min={0},step={1})The different mode usage (
"fixed"vs"auto"in other controls) is intentional based on the callback type requirements.Also applies to: 9-9, 12-12, 15-16, 28-28
editor/scaffolds/sidecontrol/controls/gap.tsx (3)
1-1: LGTM: Clean import migration to new component.The import correctly references the new
InputPropertyNumbercomponent.
5-10: LGTM: Proper interface update for commit-based handling.The interface correctly updates from
onValueChangetoonValueCommit, aligning with the new commit-based input pattern. This change improves UX by only triggering updates when the user commits the value rather than on every keystroke.
13-22: LGTM: Proper component migration with appropriate constraints.The migration to
InputPropertyNumberis well-executed:
mode="fixed"is appropriate for gap values that should be explicit numbersmin={0}prevents negative gap values, which makes logical senseonValueCommitcorrectly replaces the previous change handlereditor/scaffolds/sidecontrol/controls/gradient.tsx (2)
8-8: LGTM: Correct import for new numeric input component.The import properly references the new specialized
InputPropertyNumbercomponent.
32-48: LGTM: Excellent migration with clear behavioral documentation.The migration to
InputPropertyNumberis well-implemented:
mode="fixed"is appropriate for angle values- The comment "change on commit" clearly documents the behavioral change
- Commit-based handling improves UX by preventing excessive re-renders during angle adjustment
- The transform calculation logic remains correctly unchanged
editor/scaffolds/sidecontrol/controls/maxlength.tsx (3)
2-2: LGTM: Proper import update.Correctly imports the new
InputPropertyNumbercomponent.
7-12: LGTM: Interface properly updated for commit-based handling.The prop signature correctly changes from
onValueChangetoonValueCommit, maintaining type safety and consistency with the new input pattern.
16-25: LGTM: Well-configured numeric input with appropriate constraints.The
InputPropertyNumberconfiguration is optimal:
mode="fixed"enforces explicit numeric values for maxlengthmin={0}prevents negative values, which is logically correctonValueCommitprovides better UX by committing only final valueseditor/scaffolds/sidecontrol/controls/rotate.tsx (3)
3-3: LGTM: Import updated correctly.Properly imports the new
InputPropertyNumbercomponent.
8-12: LGTM: Dual-callback interface for enhanced UX.The interface wisely maintains both
onValueChangeandonValueCommitcallbacks, likely to support live preview during rotation (immediate feedback) while still providing commit semantics for final value persistence.
15-22: LGTM: Well-configured component with dual event handling.The
InputPropertyNumberconfiguration is appropriate:
mode="auto"likely supports mixed values or auto-completion for rotation- Both
onValueChangeandonValueCommitenable live preview with commit finalization- This provides optimal UX for rotation controls where users expect real-time visual feedback
editor/scaffolds/sidecontrol/controls/positioning.tsx (1)
2-2: Excellent systematic refactor to specialized numeric inputs!The replacement of generic input components with
InputPropertyNumberprovides several improvements:
- Enhanced keyboard navigation with arrow keys for increment/decrement
- Commit-based value handling for better UX (values only update on blur/Enter)
- Better handling of mixed/undefined states
- Consistent
mode="fixed"configuration appropriate for positioning valuesThe implementation is consistent across all four positioning controls (top, left, right, bottom) with proper callback handling.
Also applies to: 45-57, 60-72, 87-99, 102-114
editor/scaffolds/sidecontrol/ui/index.tsx (1)
15-15: Well-designed container component with comprehensive styling!The new
PropertyInputContainerprovides excellent styling infrastructure:
- Comprehensive state management (focus, hover, disabled, invalid)
- Proper accessibility support with ARIA attributes
- Consistent design system integration with Tailwind classes
- Good separation of concerns between container styling and input logic
The type-only import optimization for
TMixedis also a good practice.Also applies to: 50-66
editor/scaffolds/sidecontrol/controls/length-percentage.tsx (1)
3-3: Clean architectural separation with conditional input rendering!The refactor introduces excellent separation of concerns:
- Main component focuses on mode logic and state management
- New
__Inputcomponent handles conditional rendering based on value type- Numeric values leverage
InputPropertyNumberwith commit-based handling- Non-numeric values (auto, mixed) use appropriately disabled text inputs
- Proper mode-specific handling in commit callbacks (fixed vs percentage)
The transition from
onValueChangetoonValueCommitaligns with the commit-based UX pattern throughout the codebase.Also applies to: 29-29, 32-32, 46-55, 59-59, 82-121
editor/scaffolds/sidecontrol/controls/corner-radius.tsx (1)
2-2: Comprehensive implementation with excellent UX improvements!The corner radius controls demonstrate excellent application of the new component architecture:
- Proper separation between unified and individual corner radius modes
- Individual corner inputs wrapped in
PropertyInputContainerwith clear icon labeling- Consistent commit-based handling throughout both control variants
- Logic for detecting identical corner values and collapsing to single value is preserved
- All four corner controls maintain consistent patterns and styling
The icon placement within input containers significantly improves visual clarity and user experience.
Also applies to: 18-18, 23-23, 27-30, 42-51, 73-80, 88-88, 91-92, 95-100, 105-142
editor/scaffolds/sidecontrol/ui/number.tsx (5)
1-15: LGTM! Clean imports and solid helper function.The imports are well-organized and the
roundToStephelper function correctly handles floating-point precision by counting decimal places in the step value and rounding accordingly.
17-50: Excellent type design with discriminated unions.The
NumericPropertyControlPropstype effectively uses discriminated unions to provide type safety for different operation modes. The separation between "auto" mode (witheditor.api.NumberChangeobjects) and "fixed" mode (with direct numbers) is clean and well-structured.
52-127: Exceptional documentation quality.The JSDoc documentation is comprehensive, providing clear usage examples and detailed behavioral specifications. This level of documentation significantly improves maintainability and developer experience.
281-281: Review the fallback behavior for empty input.The
|| 0fallback means empty strings become0, which may not be the intended behavior for all use cases. Consider if this should be|| NaNor handled explicitly.Should empty input values default to
0or remain asNaN? This could affect how the component behaves with optional numeric fields.
128-298: Excellent implementation of commit-based value handling.The component successfully addresses the PR objective of fixing "value change on commit" for side controls. Key strengths:
- Proper separation between
onValueChange(during typing) andonValueCommit(on Enter/arrow keys)- Comprehensive keyboard interactions with shift modifiers
- Elegant handling of mixed states for multi-selection scenarios
- Auto-selection and focus management enhance UX
- Proper constraint enforcement with min/max values
This implementation provides a solid foundation for replacing the previous
PropertyNumbercomponent throughout the codebase.
| : type === "integer" | ||
| ? parseInt(internalValue as string) | ||
| : parseFloat(internalValue as string); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor duplicated parsing logic.
The same parsing logic is repeated in three places. Consider extracting this into a helper function to improve maintainability and reduce code duplication.
+const parseNumericValue = (value: string | number, type: "integer" | "number"): number => {
+ if (typeof value === "number") return value;
+ return type === "integer" ? parseInt(value) : parseFloat(value);
+};
// Then replace the duplicated logic:
-const currentValue =
- typeof internalValue === "number"
- ? internalValue
- : type === "integer"
- ? parseInt(internalValue as string)
- : parseFloat(internalValue as string);
+const currentValue = parseNumericValue(internalValue, type);Also applies to: 252-254, 267-269
🤖 Prompt for AI Agents
In editor/scaffolds/sidecontrol/ui/number.tsx around lines 214-216, 252-254, and
267-269, the parsing logic for converting internalValue to integer or float is
duplicated. Extract this logic into a single helper function that takes the type
and internalValue as parameters and returns the parsed number. Replace all
occurrences of the duplicated parsing code with calls to this new helper
function to improve maintainability and reduce code duplication.
Summary by CodeRabbit
onValueCommitinstead ofonValueChangefor numeric inputs.