[Fixes]: Working on image editor stuff. #64
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis pull request introduces several modifications across different modules within the application. In the editor service, adjustments have been made to cursor handling and shape dimension calculations, including scaling changes and default attribute settings. Several helper functions in the blur drawer were made publicly accessible, while the shape drawing process was enhanced with additional mouse event listeners and initialization steps. Minor whitespace and placeholder modifications were introduced in the tools panel, and clarifying comments were added in the editor screen without altering its functionality. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant SD as ShapeDrawer (drawShapeMouseDownListener)
participant PT as PointerTransformer
participant EL as EventListeners
participant BD as BlurDrawer Functions
participant Stage as Canvas/Stage
U->>SD: MouseDown Event
SD->>PT: Initialize Pointer Transformer
SD->>EL: Setup Event Listeners (initEventListeners)
EL->>BD: Invoke drawBlurMouseMoveListener
EL->>BD: Invoke drawBlurMouseUpListener
EL->>BD: Invoke drawBlurDragmoveListener
BD->>Stage: Update shape dimensions & position
Stage-->>EL: Render updated shape
U->>SD: MouseUp Event
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
|
Sir @evereq Don't merge this PR I'll create this PR for backup so that I can work more effectively and create a track record |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
apps/portal/pages/edit/[id].tsx (3)
614-614: Improve comment specificity.The comment "This is checking shape type color size etc" is too vague. Consider providing more detailed information about what types of shapes are being checked and how this affects the application.
1090-1091: Remove TODO-style comment.This comment appears to be a placeholder or reminder. If this code is complete, the comment should be removed. If additional work is needed, use a proper TODO format with specific details.
1192-1192: Remove TODO-style comment.Similar to the previous comment, this appears to be a placeholder. Either remove it if the implementation is complete or replace it with a proper documentation comment.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/portal/app/services/editor/editor.ts(4 hunks)apps/portal/components/pagesComponents/_editorScreen/editorHelpers/blurDrawer.ts(4 hunks)apps/portal/components/pagesComponents/_editorScreen/editorHelpers/shapeDrawer.ts(4 hunks)apps/portal/components/pagesComponents/_editorScreen/toolsPanel/ToolsPanel.tsx(1 hunks)apps/portal/pages/edit/[id].tsx(6 hunks)
✅ Files skipped from review due to trivial changes (1)
- apps/portal/components/pagesComponents/_editorScreen/toolsPanel/ToolsPanel.tsx
🔇 Additional comments (8)
apps/portal/components/pagesComponents/_editorScreen/editorHelpers/shapeDrawer.ts (3)
68-69: Good addition of descriptive comment and scaling variable.The comment clarifies the purpose of this code section, and the new
blurScalevariable helps with understanding the calculation.
73-76: Improved shape initialization.Setting initial dimensions to 1 and using the exact pointer position without offset is a better approach for consistent shape creation.
177-193: New functionality integrates blur effect with shape transformation.This is a significant enhancement that ties shape drawing with blur effects. The code:
- Gets the appropriate layer
- Sets up the transformer
- Attaches event listeners for mouse movement, mouse up, and transformation
- Initializes event listeners for the shape
This implements the previously commented "Add draw shape logic" from the editor screen.
apps/portal/app/services/editor/editor.ts (4)
479-480: Improved emoji size for better visibility.Doubling the emoji dimensions from 64 to 128 will make them more visible and easier to interact with.
520-521: Refined marker cursor scaling.Changing the scale factor from 2 to 1.1 makes the marker cursor more proportionate and less overwhelming.
527-529: Adjusted padding for better cursor positioning.Reducing the padding by 6 units helps maintain proper cursor position while still providing adequate spacing.
533-535: Added fallback values for cursor text and reduced font size.Adding fallback values (|| 0) for x and y coordinates prevents errors when pointer position is undefined. Reducing font size from 44 to 20 makes the text more readable and appropriately sized.
Also applies to: 540-540
apps/portal/components/pagesComponents/_editorScreen/editorHelpers/blurDrawer.ts (1)
153-165: Good refactoring: exported blur drawer functions for reuse.Making these functions public by adding the
exportkeyword improves code reusability, allowing them to be imported in other files likeshapeDrawer.ts. This promotes code modularity and reduces duplication, which is particularly valuable for complex event handling logic.Also applies to: 167-178, 180-183, 185-202, 204-222
apps/portal/pages/edit/[id].tsx
Outdated
| //dependencies.some((tool) => compareTools(activeTool, tool)) && | ||
| // setActiveTool(Object.assign({}, activeTool)); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Commented code should be explained or removed.
The active tool update logic has been commented out without explanation. If this change is intentional, please add a comment explaining why this functionality has been disabled.
🏁 Script executed:
#!/bin/bash
# Checking if this is an intentional change or if it's causing issues elsewhere
# Find where updateDependedActiveTool is called
echo "References to updateDependedActiveTool:"
rg "updateDependedActiveTool" -A 1 -B 1Length of output: 5318
Action Required: Clarify Disabled Active Tool Update Logic
- In apps/portal/pages/edit/[id].tsx (lines 467-468), the code that would update the active tool is commented out:
//dependencies.some((tool) => compareTools(activeTool, tool)) && // setActiveTool(Object.assign({}, activeTool));
- Similar functionality is actively used elsewhere in the codebase (for example, in apps/portal/pages/whiteboard/[whiteboardID].tsx and apps/extensions/src/content/panel/screens/editorScreen/EditorScreen.tsx).
- If disabling this logic in the current context is intentional, please add a comment explaining:
- Why the active tool update is disabled in this file.
- Under what conditions (or in which future scenario) it might be re-enabled.
- If the code is no longer needed, please remove the commented lines to keep the codebase clean.
| @@ -77,6 +86,7 @@ const drawShapeMouseDownListener = ({ | |||
| pixelRatio: 1, | |||
| }; | |||
|
|
|||
| console.log('shapeOptions:', shapeOptions); | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove debug console.log statement.
This debug statement should be removed before merging to production. Debug logs add noise to the browser console and can impact performance if called frequently.
- console.log('shapeOptions:', shapeOptions);📝 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.
| console.log('shapeOptions:', shapeOptions); |
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| @@ -561,6 +569,7 @@ export class EditorService { | |||
| cursorgroup.add(cursortext); | |||
| } | |||
| cursormarker.setAttr('text', Number(cursormarker.getAttr('text')) + 1); | |||
| cursorgroup.to; | |||
There was a problem hiding this comment.
Fix incomplete method call.
This line contains an incomplete method call cursorgroup.to; which will have no effect and may indicate a mistake. Either complete the method call or remove this line.
- cursorgroup.to;📝 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.
| cursorgroup.to; |
|
@Anishali2 if it's not ready yet, you can use "Draft" PR... If it's ready for Review, please remove [DO NOT MERGE] from title |
Anishali2
changed the title
Summary by CodeRabbit