feat(cloud-editor): added image mask in the cloud image editor #779
Conversation
WalkthroughThe pull request introduces a new configuration option Changes
Sequence DiagramsequenceDiagram
participant Config as Configuration
participant CropFrame as CropFrame Component
participant SVG as SVG Element
Config->>CropFrame: Set cloudImageEditorMaskHref
CropFrame->>CropFrame: _createMask(href)
CropFrame->>SVG: Create/Update mask image
CropFrame->>CropFrame: _updateMask()
CropFrame->>SVG: Adjust mask position and dimensions
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (6)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (4)
⏰ Context from checks skipped due to timeout of 90000ms (1)
🪧 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 (
|
![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)
blocks/Config/initialConfig.js (1)
76-77: LGTM! Consider fixing the typo inmediaRecorerOptionsThe initialization of
maskHrefCloudEditoras null is appropriate. However, I noticed an existing typo in the property name above it:mediaRecorerOptionsshould bemediaRecorderOptions.- mediaRecorerOptions: null, + mediaRecorderOptions: null,types/exported.d.ts (1)
270-271: Add JSDoc documentation for the new propertyPlease add JSDoc documentation for the
maskHrefCloudEditorproperty to maintain consistency with other properties in the interface.+ /** + * URL of the mask image to be applied in the cloud editor. + * @default null + */ maskHrefCloudEditor: string | null;blocks/CloudImageEditor/src/CropFrame.js (1)
475-490: Add error handling for SVG attribute updates.The
_updateMaskmethod should handle potential failures when updating SVG attributes.Apply this diff to improve error handling:
_updateMask() { let cropBox = this.$['*cropBox']; if (!cropBox || !this._frameImage) { return; } let { x, y, width, height } = cropBox; + try { setSvgNodeAttrs(this._frameImage, { x, y, height, width, }); + } catch (error) { + console.error('Failed to update mask attributes:', error); + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
blocks/CloudImageEditor/src/CropFrame.js(2 hunks)blocks/CloudImageEditor/src/css/common.css(1 hunks)blocks/Config/Config.js(2 hunks)blocks/Config/initialConfig.js(1 hunks)blocks/Config/normalizeConfigValue.js(1 hunks)demo/cloud-image-editor.html(1 hunks)types/exported.d.ts(1 hunks)
🔇 Additional comments (4)
blocks/Config/normalizeConfigValue.js (1)
175-175: LGTM!The mapping of
maskHrefCloudEditortoasStringis appropriate for handling string | null type values.blocks/CloudImageEditor/src/CropFrame.js (1)
496-496: LGTM!The addition of
_updateMask()to the render cycle ensures the mask stays synchronized with other UI updates.demo/cloud-image-editor.html (1)
33-33: LGTM!Setting
height: 100vhensures the editor container fills the viewport height correctly.blocks/CloudImageEditor/src/css/common.css (1)
1194-1196: LGTM!Setting
pointer-events: nonecorrectly prevents the mask from intercepting mouse events, allowing interactions with underlying elements.
blocks/Config/Config.js
Outdated
| * 'fileValidators', | ||
| * 'collectionValidators', | ||
| * 'mediaRecorerOptions', | ||
| * 'maskHrefCloudEditor', |
There was a problem hiding this comment.
Remove maskHrefCloudEditor from complexConfigKeys
The maskHrefCloudEditor property is of type string | null, which can be safely passed as an attribute. The complexConfigKeys array is specifically for properties that are objects or functions and cannot be passed as attributes.
* @type {[
* 'metadata',
* 'localeDefinitionOverride',
* 'secureUploadsSignatureResolver',
* 'secureDeliveryProxyUrlResolver',
* 'iconHrefResolver',
* 'fileValidators',
* 'collectionValidators',
-* 'mediaRecorerOptions',
-* 'maskHrefCloudEditor',
+* 'mediaRecorerOptions'
* ]}
*/
export const complexConfigKeys = [
'metadata',
'localeDefinitionOverride',
'secureUploadsSignatureResolver',
'secureDeliveryProxyUrlResolver',
'iconHrefResolver',
'fileValidators',
'collectionValidators',
- 'mediaRecorerOptions',
- 'maskHrefCloudEditor',
+ 'mediaRecorerOptions'
];Also applies to: 37-37
| this.subConfigValue('maskHrefCloudEditor', (maskHref) => { | ||
| if (maskHref) { | ||
| this._createMask(maskHref); | ||
| } | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle mask removal when maskHref becomes null.
The subscription should clean up the mask when the maskHref config is cleared.
Apply this diff to handle mask removal:
this.subConfigValue('maskHrefCloudEditor', (maskHref) => {
if (maskHref) {
this._createMask(maskHref);
+ } else if (this._frameImage) {
+ this._frameImage.remove();
+ this._frameImage = null;
}
});Committable suggestion skipped: line range outside the PR's diff.
| /** | ||
| * @private | ||
| * @param {String} href | ||
| */ | ||
| _createMask(href) { | ||
| if (this._frameImage) { | ||
| this._frameImage.setAttribute('href', href); | ||
| return; | ||
| } | ||
|
|
||
| let svg = this.ref['svg-el']; | ||
| let fr = document.createDocumentFragment(); | ||
|
|
||
| let imageNode = createSvgNode('image', { | ||
| href, | ||
| }); | ||
|
|
||
| imageNode.setAttribute('class', 'uc-cloud-mask'); | ||
|
|
||
| fr.appendChild(imageNode); | ||
|
|
||
| svg.appendChild(fr); | ||
|
|
||
| this._frameImage = imageNode; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation and error handling.
The _createMask method should validate the href parameter and handle potential SVG operation failures.
Apply this diff to improve the implementation:
_createMask(href) {
+ if (!href || typeof href !== 'string') {
+ console.warn('Invalid href provided to _createMask');
+ return;
+ }
+
if (this._frameImage) {
+ // Clean up old image resources before updating href
+ this._frameImage.removeAttribute('href');
this._frameImage.setAttribute('href', href);
return;
}
let svg = this.ref['svg-el'];
+ if (!svg) {
+ console.error('SVG element not found');
+ return;
+ }
let fr = document.createDocumentFragment();
let imageNode = createSvgNode('image', {
href,
});
imageNode.setAttribute('class', 'uc-cloud-mask');
fr.appendChild(imageNode);
svg.appendChild(fr);
this._frameImage = imageNode;
}📝 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.
| /** | |
| * @private | |
| * @param {String} href | |
| */ | |
| _createMask(href) { | |
| if (this._frameImage) { | |
| this._frameImage.setAttribute('href', href); | |
| return; | |
| } | |
| let svg = this.ref['svg-el']; | |
| let fr = document.createDocumentFragment(); | |
| let imageNode = createSvgNode('image', { | |
| href, | |
| }); | |
| imageNode.setAttribute('class', 'uc-cloud-mask'); | |
| fr.appendChild(imageNode); | |
| svg.appendChild(fr); | |
| this._frameImage = imageNode; | |
| } | |
| /** | |
| * @private | |
| * @param {String} href | |
| */ | |
| _createMask(href) { | |
| if (!href || typeof href !== 'string') { | |
| console.warn('Invalid href provided to _createMask'); | |
| return; | |
| } | |
| if (this._frameImage) { | |
| // Clean up old image resources before updating href | |
| this._frameImage.removeAttribute('href'); | |
| this._frameImage.setAttribute('href', href); | |
| return; | |
| } | |
| let svg = this.ref['svg-el']; | |
| if (!svg) { | |
| console.error('SVG element not found'); | |
| return; | |
| } | |
| let fr = document.createDocumentFragment(); | |
| let imageNode = createSvgNode('image', { | |
| href, | |
| }); | |
| imageNode.setAttribute('class', 'uc-cloud-mask'); | |
| fr.appendChild(imageNode); | |
| svg.appendChild(fr); | |
| this._frameImage = imageNode; | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
blocks/CloudImageEditor/src/CropFrame.js (2)
449-473:⚠️ Potential issueAdd input validation and error handling.
The
_createMaskmethod needs improved validation and error handling.Apply this diff to improve the implementation:
_createMask(href) { + if (!href || typeof href !== 'string') { + console.warn('Invalid href provided to _createMask'); + return; + } + if (this._frameImage) { + // Clean up old image resources before updating href + this._frameImage.removeAttribute('href'); this._frameImage.setAttribute('href', href); return; } let svg = this.ref['svg-el']; + if (!svg) { + console.error('SVG element not found'); + return; + } let fr = document.createDocumentFragment(); let imageNode = createSvgNode('image', { href, }); imageNode.setAttribute('class', 'uc-cloud-mask'); fr.appendChild(imageNode); svg.appendChild(fr); this._frameImage = imageNode; }
538-542:⚠️ Potential issueHandle mask removal when maskHref becomes null.
The subscription should clean up the mask when the maskHref config is cleared.
Apply this diff to handle mask removal:
this.subConfigValue('cloudImageEditorMaskHref', (maskHref) => { if (maskHref) { this._createMask(maskHref); + } else if (this._frameImage) { + this._frameImage.remove(); + this._frameImage = null; } });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
blocks/CloudImageEditor/src/CropFrame.js(2 hunks)blocks/Config/Config.js(2 hunks)blocks/Config/initialConfig.js(1 hunks)blocks/Config/normalizeConfigValue.js(1 hunks)types/exported.d.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- types/exported.d.ts
- blocks/Config/initialConfig.js
- blocks/Config/Config.js
- blocks/Config/normalizeConfigValue.js
🔇 Additional comments (1)
blocks/CloudImageEditor/src/CropFrame.js (1)
491-496: LGTM!The addition of
_updateMask()to the render cycle is appropriate and follows the existing pattern.
egordidenko
force-pushed
the
feat/cloud-editor-implement-mask
branch
from
5fabc2c to
0652cf9
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
blocks/CloudImageEditor/src/CropFrame.js (1)
Line range hint
566-570: Add cleanup for mask resources.The
destroyCallbackshould clean up mask-related resources to prevent memory leaks.destroyCallback() { super.destroyCallback(); + if (this._frameImage) { + this._frameImage.remove(); + this._frameImage = null; + } + document.removeEventListener('pointermove', this._handlePointerMove); document.removeEventListener('pointerup', this._handlePointerUp); }
🧹 Nitpick comments (1)
blocks/CloudImageEditor/src/CropFrame.js (1)
482-489: Maintain consistent property order.The property order in
setSvgNodeAttrsdiffers from the object destructuring order. Maintain consistency for better code readability.- setSvgNodeAttrs(this._frameImage, { - x, - y, - height, - width, - }); + setSvgNodeAttrs(this._frameImage, { + x, + y, + width, + height, + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
blocks/CloudImageEditor/src/CropFrame.js(2 hunks)blocks/CloudImageEditor/src/css/common.css(1 hunks)blocks/Config/initialConfig.js(1 hunks)blocks/Config/normalizeConfigValue.js(1 hunks)demo/cloud-image-editor.html(1 hunks)types/exported.d.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- demo/cloud-image-editor.html
- types/exported.d.ts
- blocks/Config/initialConfig.js
- blocks/CloudImageEditor/src/css/common.css
- blocks/Config/normalizeConfigValue.js
🔇 Additional comments (4)
blocks/CloudImageEditor/src/CropFrame.js (4)
449-473: Add input validation and error handling.The
_createMaskmethod needs validation and error handling improvements.Previous review comment still applies. The implementation should:
- Validate the
hrefparameter- Handle potential SVG operation failures
- Clean up old image resources before updates
- Validate SVG element existence
475-490: Add validation and optimize mask updates.The
_updateMaskmethod needs property validation and performance optimization.Previous review comment still applies. The implementation should:
- Validate cropBox properties
- Cache current values to avoid unnecessary updates
496-496: LGTM!The
_updateMaskcall is appropriately placed in the render cycle.
538-542:⚠️ Potential issueHandle mask cleanup and validate maskHref.
The subscription needs improvements:
- Clean up the mask when maskHref becomes null
- Add validation for maskHref value
this.subConfigValue('cloudImageEditorMaskHref', (maskHref) => { + if (typeof maskHref !== 'string' && maskHref !== null) { + console.warn('Invalid maskHref value:', maskHref); + return; + } if (maskHref) { this._createMask(maskHref); + } else if (this._frameImage) { + this._frameImage.remove(); + this._frameImage = null; } });Likely invalid or redundant comment.
egordidenko
force-pushed
the
feat/cloud-editor-implement-mask
branch
from
0652cf9 to
ea48c88
Compare
egordidenko
force-pushed
the
feat/cloud-editor-implement-mask
branch
from
ea48c88 to
d1173ba
Compare
egordidenko
changed the title
Description
Checklist
Summary by CodeRabbit
New Features
Style
Documentation