feat(image): add right-click menu to save and copy image #196
Conversation
WalkthroughThis pull request introduces a new image bar feature for the Quill editor, enhancing image interaction capabilities. The changes include creating a new Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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 (
|
WalkthroughThis pull request introduces a new feature that adds a right-click menu to images, allowing users to save or copy images directly from the editor. This enhancement improves user interaction with images within the editor. Changes
|
![petercat-assistant[bot]](https://avatars.githubusercontent.com/in/875736?s=60&v=4){kind=small}
| } | ||
| else if (operate === 'copy') { | ||
| const imageUrl = this.image.src | ||
| const response = await fetch(imageUrl) |
There was a problem hiding this comment.
Using fetch to retrieve the image data and then writing it to the clipboard could lead to potential security issues if the image source is not trusted. Ensure that the image source is validated or sanitized to prevent any malicious content from being executed.
|
![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 (8)
packages/fluent-editor/src/modules/custom-image/image-bar.ts (4)
1-6: Use explicit typings for maintainability and clarity.Both
quill: anyandimage: anycould benefit from more specific types for better readability and IDE assistance.
22-43: Add cleanup logic for event listeners.Events are attached to
imageDownloadandimageCopybut there is no direct removal of these listeners on destruction. Consider removing them explicitly indestroy()to avoid memory leaks in large or long-lived sessions.
84-94: Ensure toolbar remains visible within the viewport.If the image is near the right edge or bottom of the screen, the requested position could extend beyond the viewport. Consider clamping the position to ensure the toolbar is fully visible.
96-112: Favor built-in typing for style properties.When iterating over the
rulesobject, consider refining the property type. Using more specific CSS styling patterns (likePartial<CSSStyleDeclaration>) might help reduce type mismatches.packages/fluent-editor/src/modules/custom-image/BlotFormatter.ts (2)
22-22: Add docstring forimageBar.A short docstring clarifying when and how the
imageBaris used would make code navigation smoother for new contributors.
158-163: Consolidate destruction logic.When hiding the formatter (
this.hide()), consider integrating theimageBarteardown there as well to avoid multiple method calls inonClick. This can reduce code duplication.packages/fluent-editor/src/assets/imageBar.scss (2)
18-26: Consider a single utility class for vertical dividers.You might define a global
.vertical-dividerand reuse it across multiple components instead of repeatedly declaring a.ql-splitblock.
50-53: Ensure color transitions extend across variants.For consistent user experience, unify the transition property set across
.ql-image-preview,.ql-image-download, and.ql-image-copy.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/fluent-editor/src/assets/imageBar.scss(1 hunks)packages/fluent-editor/src/assets/style.scss(1 hunks)packages/fluent-editor/src/modules/custom-image/BlotFormatter.ts(4 hunks)packages/fluent-editor/src/modules/custom-image/image-bar.ts(1 hunks)
🔇 Additional comments (7)
packages/fluent-editor/src/modules/custom-image/image-bar.ts (1)
7-20: Consider adding a guard for initialization consistency.You might want to validate that the passed
targetis indeed an<img>element. This helps avoid errors in cases where the context menu is triggered on a non-image element.packages/fluent-editor/src/modules/custom-image/BlotFormatter.ts (3)
7-7: Inline import is acceptable.The import for
ImageBarlooks good. It’s straightforward and aligned with standard ES6 practices.
44-44: Great approach for context menu integration.Using an event listener on
contextmenuensures minimal intrusion into other editor functionality. Good job.
164-179: Validate presence ofclosestmethod.
Element.closestis not supported in some older browsers, so if there's a need for IE11 or lower support, consider a polyfill.packages/fluent-editor/src/assets/style.scss (1)
22-22: Confirm that the import path matches repo structure.Ensure
@import './imageBar';does not conflict with names or paths in different modules.packages/fluent-editor/src/assets/imageBar.scss (2)
1-16: Good use of CSS variables for theming.Leveraging
var(--devui-base-bg, #fff)etc. offers flexibility for theming. Nice approach to dynamic styling support.
127-130: Coordinate hover styling with the design system.If there's a design standard or pattern library, confirm that the hover color
#aaaaafor the variable used is consistent throughout.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
packages/fluent-editor/src/modules/custom-image/image-bar.ts (3)
11-17: Move SVG content to a separate asset fileThe SVG content should be moved to a separate asset file and imported, improving maintainability and reusability.
Consider creating an
assets/iconsdirectory and moving the SVG there, then importing it:import copyIcon from '../assets/icons/copy-icon.svg' this.template = [ `<a class="ql-image-download"><i class="icon-download"></i></a>`, `<a class="ql-image-copy">${copyIcon}</a>`, ].join('')
89-99: Improve positioning logic
- Extract magic numbers into named constants
- Add window resize handling
- Consider removing IE11 compatibility if not required
+ private static readonly TOOLBAR_WIDTH = 92 + + private handleResize = () => { + this.setPosition() + } + setPosition() { if (this.domNode && this.image) { const parent = this.quill.root.parentNode const containerRect = parent.getBoundingClientRect() const imageRect = this.image.getBoundingClientRect() this.css(this.domNode, { - left: `${imageRect.left + imageRect.width - containerRect.left - 92}px`, + left: `${imageRect.left + imageRect.width - containerRect.left - ImageBar.TOOLBAR_WIDTH}px`, top: `${imageRect.top - containerRect.top}px`, }) } } + + createImageBar() { + // ... existing code ... + window.addEventListener('resize', this.handleResize) + } + + destroy() { + // ... existing code ... + window.removeEventListener('resize', this.handleResize) + }
101-117: Consider modernizing the CSS application methodThe current implementation includes IE11 compatibility code. Consider using modern approaches if IE11 support is not required.
- css(domNode, rules) { - if (typeof rules === 'object') { - for (const prop in rules) { - if (prop) { - if (Array.isArray(rules[prop])) { - // 兼容IE11浏览器 - rules[prop].forEach((val) => { - domNode.style[prop] = val - }) - } - else { - domNode.style[prop] = rules[prop] - } - } - } - } - } + css(domNode: HTMLElement, rules: Record<string, string>) { + Object.assign(domNode.style, rules) + }
| destroy() { | ||
| if (this.domNode) { | ||
| this.domNode.remove() | ||
| this.domNode = null | ||
| this.image = null | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve cleanup process
The destroy method should remove event listeners and handle edge cases more robustly.
+ private handleDownload: (event: MouseEvent) => void
+ private handleCopy: (event: MouseEvent) => void
+
destroy() {
if (this.domNode) {
+ const imageDownload = this.domNode.querySelector('a.ql-image-download')
+ const imageCopy = this.domNode.querySelector('a.ql-image-copy')
+
+ if (imageDownload && this.handleDownload) {
+ imageDownload.removeEventListener('click', this.handleDownload)
+ }
+ if (imageCopy && this.handleCopy) {
+ imageCopy.removeEventListener('click', this.handleCopy)
+ }
+
this.domNode.remove()
this.domNode = null
this.image = null
}
}Committable suggestion skipped: line range outside the PR's diff.
Signed-off-by: wangry <[email protected]>
qwangry
force-pushed
the
wangry/feat-image
branch
from
b356cf5 to
1e56bf0
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
packages/fluent-editor/src/modules/custom-image/image-bar.ts (2)
13-19: Move SVG to assets and clean up commented code.
- The SVG content should be moved to a separate assets file for better maintainability.
- Remove the commented preview functionality if it's not planned for implementation.
99-127: Improve positioning logic.
- The hard-coded value
92in positioning calculation should be derived from actual element dimensions.- Add window resize event handling to update the position when the window is resized.
setPosition() { if (this.domNode && this.image) { + const updatePosition = () => { const parent = this.quill.root.parentNode const containerRect = parent.getBoundingClientRect() const imageRect = this.image.getBoundingClientRect() + const barWidth = this.domNode.offsetWidth this.css(this.domNode, { - left: `${imageRect.left + imageRect.width - containerRect.left - 92}px`, + left: `${imageRect.left + imageRect.width - containerRect.left - barWidth}px`, top: `${imageRect.top - containerRect.top}px`, }) + } + updatePosition() + window.addEventListener('resize', updatePosition) + // Clean up the event listener in destroy method } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/fluent-editor/src/assets/imageBar.scss(1 hunks)packages/fluent-editor/src/assets/style.scss(1 hunks)packages/fluent-editor/src/modules/custom-image/BlotFormatter.ts(4 hunks)packages/fluent-editor/src/modules/custom-image/image-bar.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/fluent-editor/src/assets/style.scss
- packages/fluent-editor/src/assets/imageBar.scss
🔇 Additional comments (6)
packages/fluent-editor/src/modules/custom-image/image-bar.ts (4)
1-8: LGTM! Well-structured class with proper TypeScript types.The class properties are properly typed, making the code type-safe and maintainable.
24-45: Clean up event listeners and translate comments to English.Previous review comments about event listener cleanup and English comments are still applicable.
47-53: Improve cleanup process.Previous review comments about improving the cleanup process are still applicable.
55-97: Improve image operations implementation and security.Previous review comments about improving image operations implementation are still applicable.
Let's verify the image sources that can be handled:
#!/bin/bash # Search for image source patterns in the codebase rg -g '!*.{jpg,png,svg}' -A 2 'src\s*=|<img.*src=|imageUrl\s*='packages/fluent-editor/src/modules/custom-image/BlotFormatter.ts (2)
7-7: LGTM! Clean import and property declaration.The ImageBar integration is properly set up with correct typing.
Also applies to: 22-22
158-162: LGTM! Proper cleanup of ImageBar instance.The click handler correctly destroys and nullifies the ImageBar instance.
| onContextmenu = (event) => { | ||
| this.hide() | ||
| event.preventDefault() | ||
| const target = event.target | ||
| const imageDom = target.closest('img') | ||
|
|
||
| if (imageDom) { | ||
| if (this.imageBar) { | ||
| this.imageBar.destroy() | ||
| } | ||
| this.imageBar = new ImageBar(this.quill, imageDom) | ||
| } | ||
| else if (this.imageBar && !target.closest('.ql-image-bar')) { | ||
| this.imageBar.destroy() | ||
| this.imageBar = null | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for edge cases.
The context menu handler should include error handling for potential edge cases:
- Invalid target element
- ImageBar creation failure
onContextmenu = (event) => {
this.hide()
event.preventDefault()
- const target = event.target
- const imageDom = target.closest('img')
+ try {
+ const target = event.target
+ if (!target || !(target instanceof Element)) {
+ return
+ }
+ const imageDom = target.closest('img')
if (imageDom) {
if (this.imageBar) {
this.imageBar.destroy()
}
- this.imageBar = new ImageBar(this.quill, imageDom)
+ try {
+ this.imageBar = new ImageBar(this.quill, imageDom)
+ } catch (error) {
+ console.error('Failed to create ImageBar:', error)
+ }
}
else if (this.imageBar && !target.closest('.ql-image-bar')) {
this.imageBar.destroy()
this.imageBar = null
}
+ } catch (error) {
+ console.error('Error handling context menu:', error)
+ }
}📝 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.
| onContextmenu = (event) => { | |
| this.hide() | |
| event.preventDefault() | |
| const target = event.target | |
| const imageDom = target.closest('img') | |
| if (imageDom) { | |
| if (this.imageBar) { | |
| this.imageBar.destroy() | |
| } | |
| this.imageBar = new ImageBar(this.quill, imageDom) | |
| } | |
| else if (this.imageBar && !target.closest('.ql-image-bar')) { | |
| this.imageBar.destroy() | |
| this.imageBar = null | |
| } | |
| onContextmenu = (event) => { | |
| this.hide() | |
| event.preventDefault() | |
| try { | |
| const target = event.target | |
| if (!target || !(target instanceof Element)) { | |
| return | |
| } | |
| const imageDom = target.closest('img') | |
| if (imageDom) { | |
| if (this.imageBar) { | |
| this.imageBar.destroy() | |
| } | |
| try { | |
| this.imageBar = new ImageBar(this.quill, imageDom) | |
| } catch (error) { | |
| console.error('Failed to create ImageBar:', error) | |
| } | |
| } | |
| else if (this.imageBar && !target.closest('.ql-image-bar')) { | |
| this.imageBar.destroy() | |
| this.imageBar = null | |
| } | |
| } catch (error) { | |
| console.error('Error handling context menu:', error) | |
| } | |
| } |
| @@ -39,6 +41,7 @@ | |||
| // disable native image resizing on firefox | |||
| document.execCommand('enableObjectResizing', false, 'false') // eslint-disable-next-line-line no-undef | |||
| this.quill.root.addEventListener('click', this.onClick) | |||
| this.quill.root.addEventListener('contextmenu', event => this.onContextmenu(event)) | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add event listener cleanup.
The context menu event listener should be cleaned up when the BlotFormatter is destroyed.
constructor(quill: any, options: any = {}) {
// ... existing code ...
this.quill.root.addEventListener('click', this.onClick)
- this.quill.root.addEventListener('contextmenu', event => this.onContextmenu(event))
+ this.onContextmenu = this.onContextmenu.bind(this)
+ this.quill.root.addEventListener('contextmenu', this.onContextmenu)
// ... existing code ...
}
+destroy() {
+ this.quill.root.removeEventListener('contextmenu', this.onContextmenu)
+ if (this.imageBar) {
+ this.imageBar.destroy()
+ this.imageBar = null
+ }
+}Committable suggestion skipped: line range outside the PR's diff.
|
@qwangry 放在右键菜单里有点太隐藏了，建议 hover 时显示，这个工具栏后续可以扩展，比如做大图预览、删除图片等，不过可以后续优化，我先合入。 |
|
@qwangry 下次提交 PR 可以在 PR 描述里的 issue 号前面加上 clsoe，这样 PR 合入之后，issue 会自动关闭。 |
好的，get了 |
Signed-off-by: wangry <[email protected]>
PR
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Issue Number: #124
What is the new behavior?
Does this PR introduce a breaking change?
Other information
Summary by CodeRabbit
New Features
Style