Added Text Styling: Opacity, Position (X/Y), Bold, Italic, Text & Background Color#351
Added Text Styling: Opacity, Position (X/Y), Bold, Italic, Text & Background Color#351AasheeshLikePanner wants to merge 3 commits intoOpenCut-app:mainfrom AasheeshLikePanner:main
Conversation
👷 Deploy request for appcut pending review.Visit the deploys page to approve it
|
|
@AasheeshLikePanner is attempting to deploy a commit to the OpenCut OSS Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughA new color picker dependency was added to the web app. Two new React dialog components were introduced for changing text and background colors. The text properties panel was extensively expanded, adding controls for font color, background color, opacity, rotation, position, weight, style, and decoration, with integrated color picker dialogs. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant TextPropertiesPanel
participant ChangeTextColorDialog
participant ChangeTextBackgroundColorDialog
User->>TextPropertiesPanel: Opens text properties
User->>TextPropertiesPanel: Clicks "Change Text Color"
TextPropertiesPanel->>ChangeTextColorDialog: Open dialog
User->>ChangeTextColorDialog: Picks color
ChangeTextColorDialog->>TextPropertiesPanel: setColor(newColor)
TextPropertiesPanel->>TextPropertiesPanel: updateTextElement(color)
User->>TextPropertiesPanel: Clicks "Change Background Color"
TextPropertiesPanel->>ChangeTextBackgroundColorDialog: Open dialog
User->>ChangeTextBackgroundColorDialog: Picks color
ChangeTextBackgroundColorDialog->>TextPropertiesPanel: setColor(newColor)
TextPropertiesPanel->>TextPropertiesPanel: updateTextElement(backgroundColor)
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: 4
🧹 Nitpick comments (3)
apps/web/src/components/change-font-background-color-dialog.tsx (1)
14-44: Consider refactoring to reduce code duplicationThis component is nearly identical to
ChangeTextColorDialog. Consider creating a single reusable color picker dialog component that accepts a title prop.Would you like me to create a unified color picker dialog component that can be reused for both text and background color selection?
apps/web/src/components/editor/properties-panel/text-properties.tsx (2)
299-307: Simplify text decoration logicThe text decoration toggle group logic is complex and could be simplified.
<ToggleGroup type="multiple" - value={element.textDecoration === "underline" ? ["underline"] : element.textDecoration === "line-through" ? ["line-through"]:[]} + value={element.textDecoration !== "none" ? [element.textDecoration] : []} onValueChange={(value: ("underline" | "line-through" | "none")[]) => { - if (value.length > 0){ - updateTextElement(trackId, element.id, { textDecoration: value[0]}); - }else{ - updateTextElement(trackId, element.id, { textDecoration: "none" }); - } + updateTextElement(trackId, element.id, { + textDecoration: value.length > 0 ? value[0] : "none" + }); }}
93-95: Consider validating color input valuesThe color input fields accept any text value without validation. Invalid color values could cause rendering issues.
Consider adding validation for hex color format or using a more controlled input approach.
Also applies to: 122-124
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
apps/web/package.json(1 hunks)apps/web/src/components/change-font-background-color-dialog.tsx(1 hunks)apps/web/src/components/change-font-color-dialog.tsx(1 hunks)apps/web/src/components/editor/properties-panel/text-properties.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: alamshafil
PR: OpenCut-app/OpenCut#325
File: apps/web/src/components/ui/font-picker.tsx:27-27
Timestamp: 2025-07-18T15:22:59.061Z
Learning: In the OpenCut video export system, the FontPicker component uses getGoogleFonts() instead of FONT_OPTIONS to limit font selection to Google Fonts only. This is because the video export functionality requires actual font files to be loaded into FFmpeg's virtual filesystem through the FontManager, rather than CSS-based fonts. System fonts like Arial and Helvetica are not available as downloadable files in the browser environment, making Google Fonts the appropriate choice for video export compatibility.
apps/web/src/components/editor/properties-panel/text-properties.tsx (1)
Learnt from: simonorzel26
PR: OpenCut-app/OpenCut#324
File: apps/web/src/components/editor/snap-indicator.tsx:43-43
Timestamp: 2025-07-17T08:26:10.929Z
Learning: In the timeline refactor PR #324, the snap indicator component in apps/web/src/components/editor/snap-indicator.tsx requires the hard-coded `ml-48` class in addition to the calculated `leftPosition` for proper alignment. This is intentional and needed for the new timeline layout.
🔇 Additional comments (4)
apps/web/package.json (1)
47-47: LGTM!The addition of
react-colorfulis appropriate for implementing the color picker functionality in the new dialog components.apps/web/src/components/change-font-color-dialog.tsx (1)
14-44: Component implementation looks good!The dialog properly handles state management, prevents auto-focus issues, and provides a clean interface for color selection.
apps/web/src/components/editor/properties-panel/text-properties.tsx (2)
207-208: Verify position range limits are sufficientThe position sliders are limited to -1000 to 1000. Depending on the canvas/viewport size, these limits might be too restrictive for positioning text elements.
Consider making these limits dynamic based on the viewport dimensions or increasing the range.
Also applies to: 237-238
20-321: Great implementation of comprehensive text styling features!The component successfully implements all the promised features from the PR objectives: opacity, position (X/Y), bold, italic, and color controls. The UI is well-structured using consistent patterns for different control types.
| onChange={(e) => | ||
| updateTextElement(trackId, element.id, { | ||
| fontSize: parseInt(e.target.value), | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for numeric inputs
The parseInt calls could result in NaN if the input is invalid. Consider adding validation or using a fallback value.
Example fix for one of the occurrences:
onChange={(e) =>
updateTextElement(trackId, element.id, {
- fontSize: parseInt(e.target.value),
+ fontSize: parseInt(e.target.value) || element.fontSize,
})
}Apply similar validation to rotation, x, and y position handlers.
Also applies to: 189-191, 219-221, 249-251
🤖 Prompt for AI Agents
In apps/web/src/components/editor/properties-panel/text-properties.tsx around
lines 69 to 72, the parseInt usage on input values can produce NaN if the input
is invalid. To fix this, add validation to check if the parsed value is a valid
number and provide a fallback default value if not. Apply this validation
pattern similarly to the handlers for rotation, x, and y positions at lines
189-191, 219-221, and 249-251 to ensure all numeric inputs are safely handled.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
apps/web/src/components/editor/properties-panel/text-properties.tsx (4)
155-158: Add validation for opacity inputThe
parseFloatcall could result inNaNif the input is invalid. Add validation similar to the font size handler.onChange={(e) => updateTextElement(trackId, element.id, { - opacity: parseFloat(e.target.value) / 100, + opacity: (parseFloat(e.target.value) || element.opacity * 100) / 100, }) }
188-191: Add validation for rotation inputThe
parseIntcall lacks validation, which could result inNaNas mentioned in the previous review.onChange={(e) => updateTextElement(trackId, element.id, { - rotation: parseInt(e.target.value), + rotation: parseInt(e.target.value) || element.rotation, }) }
218-221: Add validation for X position inputThe
parseIntcall needs validation to handle invalid input.onChange={(e) => updateTextElement(trackId, element.id, { - x: parseInt(e.target.value), + x: parseInt(e.target.value) || element.x, }) }Also, consider if the -1000 to 1000 range is sufficient for all canvas sizes.
248-251: Add validation for Y position inputThe
parseIntcall needs validation to handle invalid input.onChange={(e) => updateTextElement(trackId, element.id, { - y: parseInt(e.target.value), + y: parseInt(e.target.value) || element.y, }) }
🧹 Nitpick comments (1)
apps/web/src/components/editor/properties-panel/text-properties.tsx (1)
300-300: Improve readability with proper spacingThe conditional expression lacks spaces around operators, making it hard to read.
-value={element.textDecoration === "underline" ? ["underline"] : element.textDecoration === "line-through" ? ["line-through"]:[]} +value={element.textDecoration === "underline" ? ["underline"] : element.textDecoration === "line-through" ? ["line-through"] : []}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/web/src/components/change-font-background-color-dialog.tsx(1 hunks)apps/web/src/components/change-font-color-dialog.tsx(1 hunks)apps/web/src/components/editor/properties-panel/text-properties.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/web/src/components/change-font-color-dialog.tsx
- apps/web/src/components/change-font-background-color-dialog.tsx
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: alamshafil
PR: OpenCut-app/OpenCut#325
File: apps/web/src/components/ui/font-picker.tsx:27-27
Timestamp: 2025-07-18T15:22:59.061Z
Learning: In the OpenCut video export system, the FontPicker component uses getGoogleFonts() instead of FONT_OPTIONS to limit font selection to Google Fonts only. This is because the video export functionality requires actual font files to be loaded into FFmpeg's virtual filesystem through the FontManager, rather than CSS-based fonts. System fonts like Arial and Helvetica are not available as downloadable files in the browser environment, making Google Fonts the appropriate choice for video export compatibility.
apps/web/src/components/editor/properties-panel/text-properties.tsx (1)
Learnt from: simonorzel26
PR: OpenCut-app/OpenCut#324
File: apps/web/src/components/editor/snap-indicator.tsx:43-43
Timestamp: 2025-07-17T08:26:10.929Z
Learning: In the timeline refactor PR #324, the snap indicator component in apps/web/src/components/editor/snap-indicator.tsx requires the hard-coded `ml-48` class in addition to the calculated `leftPosition` for proper alignment. This is intentional and needed for the new timeline layout.
🔇 Additional comments (1)
apps/web/src/components/editor/properties-panel/text-properties.tsx (1)
69-72: Good validation implementation!The parseInt validation with fallback to
element.fontSizeproperly handles invalid input, addressing the previous concern about NaN values.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
apps/web/src/components/editor/properties-panel/text-properties.tsx (3)
202-206: Add validation for rotation parseInt.The rotation input needs the same validation pattern as fontSize to prevent NaN values.
onChange={(e) => updateTextElement(trackId, element.id, { - rotation: parseInt(e.target.value), + rotation: parseInt(e.target.value) || element.rotation, }) }
232-236: Add validation for position X parseInt.The position X input needs validation to prevent NaN values.
onChange={(e) => updateTextElement(trackId, element.id, { - x: parseInt(e.target.value), + x: parseInt(e.target.value) || element.x, }) }
262-266: Add validation for position Y parseInt.The position Y input needs validation to prevent NaN values.
onChange={(e) => updateTextElement(trackId, element.id, { - y: parseInt(e.target.value), + y: parseInt(e.target.value) || element.y, }) }
🧹 Nitpick comments (3)
apps/web/src/components/editor/properties-panel/text-properties.tsx (3)
278-289: Consider using "single" type for font weight toggle.Since there's only one option (bold), consider using
type="single"instead oftype="multiple"for clearer semantics.<ToggleGroup - type="multiple" - value={element.fontWeight === "bold" ? ["bold"] : []} - onValueChange={(value: ("normal" | "bold")[]) => { - updateTextElement(trackId, element.id, { fontWeight: value.includes("bold") ? "bold" : "normal" }); + type="single" + value={element.fontWeight === "bold" ? "bold" : ""} + onValueChange={(value: "bold" | "") => { + updateTextElement(trackId, element.id, { fontWeight: value || "normal" }); }}
295-306: Consider using "single" type for font style toggle.Similar to font weight, using
type="single"would be more semantically appropriate.<ToggleGroup - type="multiple" - value={element.fontStyle === "italic" ? ["italic"] : []} - onValueChange={(value: ("normal" | "italic")[]) => { - updateTextElement(trackId, element.id, { fontStyle: value.includes("italic") ? "italic" : "normal" }); + type="single" + value={element.fontStyle === "italic" ? "italic" : ""} + onValueChange={(value: "italic" | "") => { + updateTextElement(trackId, element.id, { fontStyle: value || "normal" }); }}
312-331: Simplify text decoration toggle with "single" type.The current logic only uses the first selected value, making
type="single"more appropriate and simpler.<ToggleGroup - type="multiple" - value={element.textDecoration === "underline" ? ["underline"] : element.textDecoration === "line-through" ? ["line-through"] : []} - onValueChange={(value: ("underline" | "line-through" | "none")[]) => { - if (value.length > 0) { - updateTextElement(trackId, element.id, { textDecoration: value[0] }); - } else { - updateTextElement(trackId, element.id, { textDecoration: "none" }); - } + type="single" + value={element.textDecoration === "none" ? "" : element.textDecoration} + onValueChange={(value: "underline" | "line-through" | "") => { + updateTextElement(trackId, element.id, { textDecoration: value || "none" }); }}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/web/src/components/editor/properties-panel/text-properties.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: alamshafil
PR: OpenCut-app/OpenCut#325
File: apps/web/src/components/ui/font-picker.tsx:27-27
Timestamp: 2025-07-18T15:22:59.061Z
Learning: In the OpenCut video export system, the FontPicker component uses getGoogleFonts() instead of FONT_OPTIONS to limit font selection to Google Fonts only. This is because the video export functionality requires actual font files to be loaded into FFmpeg's virtual filesystem through the FontManager, rather than CSS-based fonts. System fonts like Arial and Helvetica are not available as downloadable files in the browser environment, making Google Fonts the appropriate choice for video export compatibility.
apps/web/src/components/editor/properties-panel/text-properties.tsx (1)
Learnt from: simonorzel26
PR: OpenCut-app/OpenCut#324
File: apps/web/src/components/editor/snap-indicator.tsx:43-43
Timestamp: 2025-07-17T08:26:10.929Z
Learning: In the timeline refactor PR #324, the snap indicator component in apps/web/src/components/editor/snap-indicator.tsx requires the hard-coded `ml-48` class in addition to the calculated `leftPosition` for proper alignment. This is intentional and needed for the new timeline layout.
🔇 Additional comments (2)
apps/web/src/components/editor/properties-panel/text-properties.tsx (2)
1-30: Component setup looks good.The imports, TypeScript interfaces, and component structure are well-organized. The local state management for dialog visibility follows React best practices.
69-72: Validation improvement implemented correctly.Good fix! The fallback to
element.fontSizeprevents NaN values when parseInt fails, addressing the previous validation concern.
| onChange={(e) => { | ||
| { | ||
| const value = e.target.value.toLowerCase(); | ||
| if (!value || value === 'transparent' || /^#([0-9A-Fa-f]{3}){1,2}$/.test(value)) { | ||
| updateTextElement(trackId, element.id, { | ||
| backgroundColor: value || 'transparent' | ||
| }); | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix syntax errors in background color onChange handler.
Similar syntax issue with extra curly braces in the background color handler.
onChange={(e) => {
- {
const value = e.target.value.toLowerCase();
if (!value || value === 'transparent' || /^#([0-9A-Fa-f]{3}){1,2}$/.test(value)) {
updateTextElement(trackId, element.id, {
backgroundColor: value || 'transparent'
});
}
- }
}
- }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| onChange={(e) => { | |
| { | |
| const value = e.target.value.toLowerCase(); | |
| if (!value || value === 'transparent' || /^#([0-9A-Fa-f]{3}){1,2}$/.test(value)) { | |
| updateTextElement(trackId, element.id, { | |
| backgroundColor: value || 'transparent' | |
| }); | |
| } | |
| } | |
| } | |
| } | |
| onChange={(e) => { | |
| const value = e.target.value.toLowerCase(); | |
| if ( | |
| !value || | |
| value === 'transparent' || | |
| /^#([0-9A-Fa-f]{3}){1,2}$/.test(value) | |
| ) { | |
| updateTextElement(trackId, element.id, { | |
| backgroundColor: value || 'transparent' | |
| }); | |
| } | |
| }} |
🤖 Prompt for AI Agents
In apps/web/src/components/editor/properties-panel/text-properties.tsx around
lines 128 to 138, the onChange handler for backgroundColor contains unnecessary
extra curly braces causing syntax errors. Remove the redundant braces wrapping
the handler body so that the function has a single block scope with the logic
inside it, ensuring proper syntax and execution.
| onChange={(e) => { | ||
| { | ||
| const value = e.target.value; | ||
| if (!value || /^#([0-9A-Fa-f]{3}){1,2}$/.test(value)) { | ||
| updateTextElement(trackId, element.id, { color: value || element.color }); | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix syntax errors in onChange handler.
There are extra curly braces causing syntax errors in the onChange handler.
onChange={(e) => {
- {
const value = e.target.value;
if (!value || /^#([0-9A-Fa-f]{3}){1,2}$/.test(value)) {
updateTextElement(trackId, element.id, { color: value || element.color });
}
- }
}
- }Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In apps/web/src/components/editor/properties-panel/text-properties.tsx around
lines 94 to 102, remove the unnecessary extra curly braces inside the onChange
handler to fix the syntax errors. Ensure the handler function has a single block
with the logic for extracting the value, validating it, and calling
updateTextElement without redundant braces.
|
fix AI comments and open into staging not main |
|
@izadoesdev can you check this pr now i open the pr into staging #363 |
2 participants
Description
Implemented a set of text styling features including opacity adjustment, X and Y position control, text decorations like bold and italic, as well as custom text and background color support. These enhancements give users more flexibility to customize and fine-tune text appearance directly within the editor.
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Test Configuration:
Screenshots (if applicable)
Add screenshots to help explain your changes.
text.editing.mp4
Checklist:
Additional context
Add any other context about the pull request here.
Summary by CodeRabbit
New Features
Chores